First-run page (revisited)

Follow-up to http://codereview.adblockplus.org/9078049/ (however, based on revision 3652)

First patch deliberately doesn't address comments about image paths and inline JavaScript due to the merge with the other version.

[Wladimir Palant, 2013-05-15 12:12:02]

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

http://codereview.adblockplus.org/10585038/diff/1/chrome/content/ui/firstRun....
chrome/content/ui/firstRun.js:31: document.addEventListener('scroll', onScroll,
false);
Nit: double quotation marks here and above please.

[Thomas Greiner, 2013-05-23 09:17:20]

Merged with existing version and made it work on Firefox and Chrome.

Still to do:
- make remaining buttons functional
- get rid of error in typoSettings.js
- remove any IE specific stuff (in case it won't be ready for IE yet)

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

http://codereview.adblockplus.org/10585038/diff/1/chrome/content/ui/firstRun....
chrome/content/ui/firstRun.js:31: document.addEventListener('scroll', onScroll,
false);
On 2013/05/15 12:12:02, Wladimir Palant wrote:
> Nit: double quotation marks here and above please.

Done.

[Thomas Greiner, 2013-05-23 14:39:18]

Implemented behavior of remaining buttons on Firefox

[Thomas Greiner, 2013-05-24 10:19:12]

Implemented behavior of remaining buttons on Chrome
Added changelog and data corruption warning

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:105: // TODO: check in Firefox
ignore the TODO here :)

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/typoS...
File chrome/content/ui/typoSettings.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/typoS...
chrome/content/ui/typoSettings.js:111: // only interact with UI if loaded in the
context of filter settings dialog
tried to put the whole block under the if but resulted in an error in the filter
settings on Firefox

[Wladimir Palant, 2013-05-27 11:02:00]

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/featu...
File chrome/content/ui/featureSubscriptions.xml (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/featu...
chrome/content/ui/featureSubscriptions.xml:30: author="Hubird"/>
This should be Fanboy's Social Blocking List:

<subscription feature="social"
              title="Fanboy's Social Blocking List"
              url="https://easylist-downloads.adblockplus.org/fanboy-social.txt"
              author="fanboy, Famlam, MonztA"/>

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
File chrome/content/ui/firstRun.html (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:21: <meta http-equiv="Content-Type"
content="text/html; charset=UTF-8">
<meta charset="utf-8"> please.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:23: <script type="text/javascript"
src="firstRun.js" charset="utf-8"></script>
Please don't define the charset of scripts - it's inherited from the page
charset (and irrelevant for our scripts anyway, particularly because Chrome
seems to ignore it sometimes).

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:33: document.documentElement.className =
userAgent;
Please never do user agent detection, use feature detection. Either way, you
don't need it. Simply include utils.js in the HTML code and add utils.js in
Chrome with the following code:

var backgroundPage = chrome.extension.getBackgroundPage();
var require = backgroundPage.require;

You can also add require() calls for whatever API you need here.

If you still need to check application then, check require("info").application.
Normally you shouldn't need that however.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:52: 
How about hardcoding the value of featureSubscriptions here instead of doing a
synchronous XMLHttpRequest (very bad)? subscriptions.xml is auto-generated, your
featureSubscriptions.xml isn't.

var featureSubscriptions = [
  {
    feature: "malware",
    url: "...",
    title: "..."
  },
  {
    ...
  }
];

Note that this is a list above, you don't really need a lookup table.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:64: scripts.push("i18n.js");
This shouldn't be necessary. As I said above, just load utils.js and i18n.js
every time - the contents of these files will be different depending on the
application however.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:79: || (userAgent == "firefox" &&
!UI.firstRunDone);
I think we can safely drop the update scenario now - if we are showing the first
run page then it's a first install. Consequently, you don't need to set the page
title dynamically.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:92: window.Filter =
require("filterClasses").Filter;
This should be in the Chrome variant of utils.js.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:102:
document.getElementById("title-changelog").style.display = "none";
See above, the update case is irrelevant by now and hence that link can go.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:106: if (userAgent == "chrome" &&
backgroundPage.seenDataCorruption)
Please change that into (typeof backgroundPage != "undefined" &&
backgroundPage.seenDataCorruption) - Chrome isn't the only application with a
background page.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:133: if (userAgent == "firefox")
Please don't assume that only Firefox has the typo correction feature - just
check whether it's there, |if (require("typoBootstrap"))| should do. Also, don't
"reuse" an unrelated script - add the two lines to enable/disable type
correction to this script.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:156: setToggleSubscriptionButton("tracking");
How about:

featureSubscriptions.forEach(setToggleSubscriptionButton);

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:193: return subscription != null &&
!subscription.disabled;
FilterStorage keeps a lookup table for listed subscriptions, please use it:

return
  featureSubscription.url in FilterStorage.knownSubscriptions &&
  !Subscription.fromURL(featureSubscription.url).disabled;

getSubscription() shouldn't be necessary.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:205: updateToggleButton(feature,
isSubscriptionEnabled(featureSubscription));
How about adding a single listener that will update all subscription buttons
rather than one per subscription? Also, this listener needs to be removed when
the page unloads. And finally, why do you care about filter notifications?

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:223: subscription.disabled = false;
The logic should be simpler here, either it's currently enabled or it isn't:

    var subscription = Subscription.fromURL(featureSubscription.url);
    if (isSubscriptionEnabled(featureSubscription))
      FilterStorage.removeSubscription(subscription);
    else
    {
      subscription.disabled = false;
      subscription.title = featureSubscription.title;
      subscription.homepage = featureSubscription.homepage;
      FilterStorage.addSubscription(subscription);
      if (!subscription.lastDownload)
        Synchronizer.execute(subscription);
    }

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:336: }
The anchor parameter is unnecessary.

This is duplicating some code from ui.js in Firefox. Please add
Utils.getDocLink() to utils.js and make UI.loadDocLink() use it. In Chrome you
can add it to lib/utils.js as well.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:344: backgroundPage.openOptions();
Again, please don't go by user agents here:

if (typeof UI != "undefined")
  UI.openFiltersDialog();
else
  backgroundPage.openOptions();

In the long term we need a UI module for Chrome that's compatible with Firefox.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n.js
File chrome/content/ui/i18n.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:24: {
Please add a comment noting that this is the Firefox branch.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:26: var stringBundle =
Services.strings.createBundle("chrome://" + require("info").addonName +
"/locale/firstRun.properties?" + Math.random());
A relative URL should work here: "/locale/firstRun.properties?" + Math.random().
However, we shouldn't hardcode the page name, the same localization approach
will likely be used for other pages as well in future. So maybe:

var pageName = location.pathname.replace(/.*\//, '').replace(/\..*?$/, '');
var stringBundle = Services.strings.createBundle("/locale/" + pageName +
".properties?" + Math.random());

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:68: return "Missing translation: " + key;
This won't work - GetStringForName throws on missing translations. So you want
to use a try..catch block instead and return this string on exceptions.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:110: // Script could be injected after
DOMContentLoaded fired
Nope, it could not - if firstRun.js is coded cleanly ;)
Please just use DOMContentLoaded.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/typoS...
File chrome/content/ui/typoSettings.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/typoS...
chrome/content/ui/typoSettings.js:111: // only interact with UI if loaded in the
context of filter settings dialog
Please revert these changes, this script should only be used in the context of
the filter settings dialog.

[Thomas Greiner, 2013-05-27 13:03:16]

Done everything except those comments linked to the creation of a
Chrome-specific utils.js due to embedding issues with the JavaScript version.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/featu...
File chrome/content/ui/featureSubscriptions.xml (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/featu...
chrome/content/ui/featureSubscriptions.xml:30: author="Hubird"/>
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> This should be Fanboy's Social Blocking List:
> 
> <subscription feature="social"
>               title="Fanboy's Social Blocking List"
>              
url="https://easylist-downloads.adblockplus.org/fanboy-social.txt"
>               author="fanboy, Famlam, MonztA"/>

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
File chrome/content/ui/firstRun.html (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:21: <meta http-equiv="Content-Type"
content="text/html; charset=UTF-8">
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> <meta charset="utf-8"> please.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:23: <script type="text/javascript"
src="firstRun.js" charset="utf-8"></script>
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Please don't define the charset of scripts - it's inherited from the page
> charset (and irrelevant for our scripts anyway, particularly because Chrome
> seems to ignore it sometimes).

Just copied those from our previous first-run page in Chrome so no objections.
Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:33: document.documentElement.className =
userAgent;
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Please never do user agent detection, use feature detection. Either way, you
> don't need it. Simply include utils.js in the HTML code and add utils.js in
> Chrome with the following code:
> 
> var backgroundPage = chrome.extension.getBackgroundPage();
> var require = backgroundPage.require;
> 
> You can also add require() calls for whatever API you need here.
> 
> If you still need to check application then, check
require("info").application.
> Normally you shouldn't need that however.

I wanted to but the problem is that Firefox' utils.js uses JavaScript version
1.7 which - when declared in the <script> tag - is not being executed in Chrome.

Not declaring type="application/x-javascript;version=1.7" means that Firefox
won't treat utils.js as intended. Haven't found a good solution for dealing with
that, unfortunately.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:52: 
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> How about hardcoding the value of featureSubscriptions here instead of doing a
> synchronous XMLHttpRequest (very bad)? subscriptions.xml is auto-generated,
your
> featureSubscriptions.xml isn't.
> 
> var featureSubscriptions = [
>   {
>     feature: "malware",
>     url: "...",
>     title: "..."
>   },
>   {
>     ...
>   }
> ];
> 
> Note that this is a list above, you don't really need a lookup table.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:79: || (userAgent == "firefox" &&
!UI.firstRunDone);
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> I think we can safely drop the update scenario now - if we are showing the
first
> run page then it's a first install. Consequently, you don't need to set the
page
> title dynamically.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:102:
document.getElementById("title-changelog").style.display = "none";
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> See above, the update case is irrelevant by now and hence that link can go.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:106: if (userAgent == "chrome" &&
backgroundPage.seenDataCorruption)
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Please change that into (typeof backgroundPage != "undefined" &&
> backgroundPage.seenDataCorruption) - Chrome isn't the only application with a
> background page.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:133: if (userAgent == "firefox")
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Please don't assume that only Firefox has the typo correction feature - just
> check whether it's there, |if (require("typoBootstrap"))| should do. Also,
don't
> "reuse" an unrelated script - add the two lines to enable/disable type
> correction to this script.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:156: setToggleSubscriptionButton("tracking");
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> How about:
> 
> featureSubscriptions.forEach(setToggleSubscriptionButton);

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:193: return subscription != null &&
!subscription.disabled;
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> FilterStorage keeps a lookup table for listed subscriptions, please use it:
> 
> return
>   featureSubscription.url in FilterStorage.knownSubscriptions &&
>   !Subscription.fromURL(featureSubscription.url).disabled;
> 
> getSubscription() shouldn't be necessary.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:205: updateToggleButton(feature,
isSubscriptionEnabled(featureSubscription));
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> How about adding a single listener that will update all subscription buttons
> rather than one per subscription? Also, this listener needs to be removed when
> the page unloads. And finally, why do you care about filter notifications?

Not removing the listeners was a clear oversight, I agree.

I personally don't mind not having this listener but I thought it'd make it more
consistent with the settings in case they change and especially because it
didn't require any effort to add it. But I see why you'd want to remove it which
is why I removed it now.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:223: subscription.disabled = false;
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> The logic should be simpler here, either it's currently enabled or it isn't:
> 
>     var subscription = Subscription.fromURL(featureSubscription.url);
>     if (isSubscriptionEnabled(featureSubscription))
>       FilterStorage.removeSubscription(subscription);
>     else
>     {
>       subscription.disabled = false;
>       subscription.title = featureSubscription.title;
>       subscription.homepage = featureSubscription.homepage;
>       FilterStorage.addSubscription(subscription);
>       if (!subscription.lastDownload)
>         Synchronizer.execute(subscription);
>     }

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:344: backgroundPage.openOptions();
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Again, please don't go by user agents here:
> 
> if (typeof UI != "undefined")
>   UI.openFiltersDialog();
> else
>   backgroundPage.openOptions();
> 
> In the long term we need a UI module for Chrome that's compatible with
Firefox.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n.js
File chrome/content/ui/i18n.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:24: {
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Please add a comment noting that this is the Firefox branch.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:26: var stringBundle =
Services.strings.createBundle("chrome://" + require("info").addonName +
"/locale/firstRun.properties?" + Math.random());
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> A relative URL should work here: "/locale/firstRun.properties?" +
Math.random().
> However, we shouldn't hardcode the page name, the same localization approach
> will likely be used for other pages as well in future. So maybe:
> 
> var pageName = location.pathname.replace(/.*\//, '').replace(/\..*?$/, '');
> var stringBundle = Services.strings.createBundle("/locale/" + pageName +
> ".properties?" + Math.random());

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:68: return "Missing translation: " + key;
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> This won't work - GetStringForName throws on missing translations. So you want
> to use a try..catch block instead and return this string on exceptions.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:110: // Script could be injected after
DOMContentLoaded fired
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Nope, it could not - if firstRun.js is coded cleanly ;)
> Please just use DOMContentLoaded.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/typoS...
File chrome/content/ui/typoSettings.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/typoS...
chrome/content/ui/typoSettings.js:111: // only interact with UI if loaded in the
context of filter settings dialog
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> Please revert these changes, this script should only be used in the context of
> the filter settings dialog.

Done.

[Wladimir Palant, 2013-05-27 14:10:37]

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:33: document.documentElement.className =
userAgent;
Feel free to replace let by var everywhere in utils.js in Firefox - that should
make it usable without version=1.7.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:40: url:
"https://easylist-downloads.adblockplus.org/malwaredomains_full.txt"
Please add the homepage field as well - I noticed that you are using it below
(and it's actually necessary).

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:95:
document.getElementById("dataCorruptionWarning").removeAttribute("hidden");
utils.js in Firefox defines a E("dataCorruptionWarning") shortcut - feel free to
define it in Chrome as well and use it here.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:128: });
This listener needs to be removed on unload.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:131:
setTypoCorrectionEnabled(!Prefs.correctTypos);
How about Prefs.correctTypos = !Prefs.correctTypos? The function seems to be
pointless :)

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:300: }
Comment not addressed (in addition to utils.js & Co.):

The anchor parameter is unnecessary. This is duplicating some code from ui.js in
Firefox. Please add Utils.getDocLink() to utils.js and make UI.loadDocLink() use
it. In Chrome you can add it to lib/utils.js as well.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/i18n.js
File chrome/content/ui/i18n.js (right):

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:77: return "Missing translation: " + key;
Cu.reportError(e) before returning please - a missing translation is always an
error in Firefox, with the build tools filling up translations. So we shouldn't
be sweeping the error under the carpet here.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/typoS...
File chrome/content/ui/typoSettings.js (right):

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/typoS...
chrome/content/ui/typoSettings.js:111: // only interact with UI if loaded in the
context of filter settings dialog
Please remove that comment as well. There should be no changes in this file.

[Wladimir Palant, 2013-05-27 15:17:28]

I also noticed that you removed the FilterNotifier listener completely - this
wasn't really the idea...

[Thomas Greiner, 2013-05-27 16:39:14]

All comments addressed now (including the one about the removal of
FilterNotifier) and tested on both Chrome and Firefox.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:33: document.documentElement.className =
userAgent;
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> Feel free to replace let by var everywhere in utils.js in Firefox - that
should
> make it usable without version=1.7.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:64: scripts.push("i18n.js");
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> This shouldn't be necessary. As I said above, just load utils.js and i18n.js
> every time - the contents of these files will be different depending on the
> application however.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:92: window.Filter =
require("filterClasses").Filter;
On 2013/05/27 11:02:00, Wladimir Palant wrote:
> This should be in the Chrome variant of utils.js.

Done.

http://codereview.adblockplus.org/10585038/diff/27001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:205: updateToggleButton(feature,
isSubscriptionEnabled(featureSubscription));
On 2013/05/27 13:03:16, Thomas Greiner wrote:
> On 2013/05/27 11:02:00, Wladimir Palant wrote:
> > How about adding a single listener that will update all subscription buttons
> > rather than one per subscription? Also, this listener needs to be removed
when
> > the page unloads. And finally, why do you care about filter notifications?
> 
> Not removing the listeners was a clear oversight, I agree.
> 
> I personally don't mind not having this listener but I thought it'd make it
more
> consistent with the settings in case they change and especially because it
> didn't require any effort to add it. But I see why you'd want to remove it
which
> is why I removed it now.

Readded with unload.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:40: url:
"https://easylist-downloads.adblockplus.org/malwaredomains_full.txt"
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> Please add the homepage field as well - I noticed that you are using it below
> (and it's actually necessary).

Done.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:95:
document.getElementById("dataCorruptionWarning").removeAttribute("hidden");
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> utils.js in Firefox defines a E("dataCorruptionWarning") shortcut - feel free
to
> define it in Chrome as well and use it here.

Done.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:128: });
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> This listener needs to be removed on unload.

Done.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:131:
setTypoCorrectionEnabled(!Prefs.correctTypos);
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> How about Prefs.correctTypos = !Prefs.correctTypos? The function seems to be
> pointless :)

True! :)

Done.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:300: }
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> Comment not addressed (in addition to utils.js & Co.):
> 
> The anchor parameter is unnecessary. This is duplicating some code from ui.js
in
> Firefox. Please add Utils.getDocLink() to utils.js and make UI.loadDocLink()
use
> it. In Chrome you can add it to lib/utils.js as well.

Done.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/i18n.js
File chrome/content/ui/i18n.js (right):

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:77: return "Missing translation: " + key;
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> Cu.reportError(e) before returning please - a missing translation is always an
> error in Firefox, with the build tools filling up translations. So we
shouldn't
> be sweeping the error under the carpet here.

Done.

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/typoS...
File chrome/content/ui/typoSettings.js (right):

http://codereview.adblockplus.org/10585038/diff/51001/chrome/content/ui/typoS...
chrome/content/ui/typoSettings.js:111: // only interact with UI if loaded in the
context of filter settings dialog
On 2013/05/27 14:10:37, Wladimir Palant wrote:
> Please remove that comment as well. There should be no changes in this file.

Done.

http://codereview.adblockplus.org/10585038/diff/41006/chrome/content/ui/i18n.js
File chrome/content/ui/i18n.js (right):

http://codereview.adblockplus.org/10585038/diff/41006/chrome/content/ui/i18n....
chrome/content/ui/i18n.js:29: var stringBundle =
Services.strings.createBundle("chrome://adblockplus/locale/" + pageName +
After testing it in Firefox it turned out that /locale/* didn't work.

[Wladimir Palant, 2013-05-28 13:39:01]

LGTM, please fix the nits above and push.

http://codereview.adblockplus.org/10585038/diff/41006/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/10585038/diff/41006/chrome/content/ui/first...
chrome/content/ui/firstRun.js:86: }, false);
If you want a function then please don't wrap it in a pointless callback:

  E("toggle-typo").addEventListener("click", toggleTypoCorrectionEnabled,
false);

Note that you can remove the toggleTypo variable, IMHO this is the only place it
is used.

http://codereview.adblockplus.org/10585038/diff/41006/chrome/content/ui/first...
chrome/content/ui/firstRun.js:95: if
(/^(filter|subscription)\.(added|removed|disabled)$/.test(action))
This regexp should be: /^subscription\.(added|removed|disabled)$/ (we don't care
about filter changes). Also, I don't think you want to run that loop for each
action - please check the action first, go through the loop then only if we have
a match.

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

http://codereview.adblockplus.org/10585038/diff/41006/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:22: }
From where I'm standing this rule is obsolete and can be removed - all browsers
we support now have proper support for the hidden attribute. Did you find a
reason why it needs to be kept?