New toggle button on First-run page

Implementing the new toggle button on first-run page.

This issue also contains a fix for the issue that our social media widgets were not shown when the "Remove Social Media Buttons" feature was enabled. In that case clicking on the button will open our social media page rather than opening the share popup.

[Thomas Greiner, 2013-06-07 12:24:21]

Implementing the new toggle button on first-run page.

This issue also contains a fix for the issue that our social media widgets were
not shown when the "Remove Social Media Buttons" feature was enabled. In that
case clicking on the button will open our social media page rather than opening
the share popup.

[Wladimir Palant, 2013-06-07 14:00:04]

LGTM (feel free to push) if my comments below are addressed.

http://codereview.adblockplus.org/10860047/diff/1/chrome/content/ui/firstRun.js
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10860047/diff/1/chrome/content/ui/firstRun....
chrome/content/ui/firstRun.js:237: }
Could you split out that change and only leave the button changes there? I don't
really like how this makes assumptions that the social subscription (and only
the social subscription) will block the social buttons instead of implementing
it as a fallback if things fails.

http://codereview.adblockplus.org/10860047/diff/1/chrome/content/ui/firstRun....
chrome/content/ui/firstRun.js:294: button.className =
button.className.replace("disable", "enable");
Frankly, one class name should be sufficient here:

  if (isEnabled)
    button.classList.add("enabled");
  else
    button.classList.remove("enabled");

[Thomas Greiner, 2013-06-11 12:23:01]

Changed button to have dynamic width.

Note: Reverted the code parts regarding the social subscription fix. You can
ignore those in firstRun.html and in firstRun.js.

[Wladimir Palant, 2013-07-08 09:37:29]

These styles make my head spin, I guess I'll try to make sense of them when I
see them live in the extension.

http://codereview.adblockplus.org/10860047/diff/4001/chrome/content/ui/firstR...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10860047/diff/4001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:277: button.classList.add("off");
classList.remove() and classList.add() already do classList.contains()
internally - the explicit checks really aren't necessary.

http://codereview.adblockplus.org/10860047/diff/4001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/10860047/diff/4001/chrome/skin/firstRun.css...
chrome/skin/firstRun.css:309: z-index: 1;
Are we supporting IE6 now? In all other browsers that property should be
pointless.

http://codereview.adblockplus.org/10860047/diff/4001/chrome/skin/firstRun.css...
chrome/skin/firstRun.css:507: z-index: 1;
Same here, why do we need that?

[Thomas Greiner, 2013-07-08 10:30:19]

http://codereview.adblockplus.org/10860047/diff/4001/chrome/content/ui/firstR...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10860047/diff/4001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:277: button.classList.add("off");
On 2013/07/08 09:37:29, Wladimir Palant wrote:
> classList.remove() and classList.add() already do classList.contains()
> internally - the explicit checks really aren't necessary.

Done.

http://codereview.adblockplus.org/10860047/diff/4001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/10860047/diff/4001/chrome/skin/firstRun.css...
chrome/skin/firstRun.css:309: z-index: 1;
On 2013/07/08 09:37:29, Wladimir Palant wrote:
> Are we supporting IE6 now? In all other browsers that property should be
> pointless.

Done.

http://codereview.adblockplus.org/10860047/diff/4001/chrome/skin/firstRun.css...
chrome/skin/firstRun.css:507: z-index: 1;
On 2013/07/08 09:37:29, Wladimir Palant wrote:
> Same here, why do we need that?

Done.

[Wladimir Palant, 2013-07-08 11:03:31]

LGTM