First run page

This code review is based on revision #3593 because I couldn't update to default - hg failed on jshydra dependency.

I've made only Typo Correction feature to support activation because I didn't find other feature switches in preferences.

I think it would be better not to simply hide Activate button, but to put some sort of a tick mark instead, but wanted to consult you first.

I didn't know how to address openFilters() if it is enclosed in anonymous closure, so I've left it alone.

I didn't remove init() because I was not sure if it will be not needed anymore.

I didn't implement clicks on social buttons because I need initial assistance from Felix on that.

[Andrey Novikov, 2012-12-28 11:21:07]

See issue description for comments.

[Wladimir Palant, 2013-05-07 15:01:12]

Obviously, this needs to be properly localizable before it can land. Also, we
need to consider the scenario where some features are already activated when
this page is displayed, e.g. because the extension was preconfigured by an
admin. Showing features that are already activated makes little sense. Finally,
the images need to be combined into sprites.

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

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.j...
chrome/content/ui/firstRun.js:22: var right;
The variables left and right seem to be unused.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.j...
chrome/content/ui/firstRun.js:48: shade.style.opacity = (contentHeight >
document.documentElement.clientHeight) ? "0.5" : "0.0";
Why do we have different logic here and in onScroll? This function should simply
call onScroll. No point caching contentHeight, onScroll can simply look it up
every time.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.j...
chrome/content/ui/firstRun.js:54: scrollTimer = setTimeout(scrollPage, 20);
What if mouseover event fires twice for some reason (e.g. because you catch the
event from a child element)? Cleaner solution:

function scrollPage()
{
  if (scrollTimer)
    stopScroll();

  scrollTimer = setInterval(function()
  {
    window.scrollBy(0, 5);
  }, 20);
}

function stopScroll()
{
  clearTimeout(scrollTimer);
  scrollTimer = null;
}

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.j...
chrome/content/ui/firstRun.js:62: function scrollContent(direction)
This function seems to be unused code.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
File chrome/content/ui/firstRun.xhtml (right):

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:38: <a href="#"
onclick="UI.loadDocLink('acceptable_ads', 'criteria'); return false;">strict
guidelines</a>
In fact, that's something we've done wrong in Chrome - anchors should also be
set on the server side, not hardcoded in the extensions. I added
acceptable_ads_criteria redirect, please use it instead of changing
loadDocLink().

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:43: <h2>Adblock Plus can do more then block
ads:</h2>
s/then/than/

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:46: <div class="feature">
Wrong indentation here.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:47: <img
src="chrome://adblockplus/skin/features/typo.png" alt="Typo Correction"/>
I would suggest going with alt="" here - the alt text doesn't provide any
additional information and localizing it is pointless. Also, image paths really
belong into skin (CSS code).

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:54: <button
onclick="TypoActions.setEnabled(true); this.setAttribute('hidden', 'true');
return false;">Activate</button>
We shouldn't be using inline JavaScript if we want to use this page in Chrome
later - inline JavaScript is forbidden via CSP there. Not using it is better
style anyway.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:63: <button>Activate</button>
This is obviously a TODO.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:73: <button>Activate</button>
This is obviously a TODO.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:81: <button>Activate</button>
This is obviously a TODO.

http://codereview.adblockplus.org/9078049/diff/1/chrome/content/ui/firstRun.x...
chrome/content/ui/firstRun.xhtml:89: <img
src="chrome://adblockplus/skin/social/facebook.png" width="82" height="82"
alt="Facebook logo"/>
Alt text should be "Share on Facebook" here - otherwise a blind person will have
no idea what this is. A different accessibility approach will be necessary once
the paths are moved to CSS however.

These buttons shouldn't be links, they should work the same as Chrome's
first-run page.