Issue 3748 - Update URL Fixer dependency on buildtools to revision c92cc4e4a338 and make it E10S-co…

Issue 3748 - Update URL Fixer dependency on buildtools to revision c92cc4e4a338 and make it E10S-compatible

[Wladimir Palant, 2016-03-07 20:25:23]

https://codereview.adblockplus.org/29337915/diff/29337923/chrome/content/typo...
File chrome/content/typoRules.json (right):

https://codereview.adblockplus.org/29337915/diff/29337923/chrome/content/typo...
chrome/content/typoRules.json:1: {
Not obvious with Rietveld but this is a rename without any changes:
defaults/typoRules.json => chrome/content/typoRules.json (defaults directory is
no longer being packaged up).

https://codereview.adblockplus.org/29337915/diff/29337923/lib/prefs.json
File lib/prefs.json (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/prefs.json#newc...
lib/prefs.json:1: {
Not obvious with Rietveld but this is a rename without any changes:
defaults/prefs.json => lib/prefs.json (this file should now be loaded via
require("prefs.json")).

[Thomas Greiner, 2016-03-08 12:34:44]

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
File lib/child/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
lib/child/typoNetError.js:47: netErrorOverlay =
parser.parseFromString(message.data, "application/xml");
Note that previously we were first loading the error overlay and then installing
the `WindowObserver` which installed the "DOMContentLoaded" listener. Now,
however, we're installing the "DOMContentLoaded" listener in parallel to
requesting the error overlay.

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
lib/child/typoNetError.js:72: if (topic != "content-document-global-created")
Detail: Is this check necessary given that you've already specified in line 134
that you're only listening to "content-document-global-created"?

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
lib/child/typoNetError.js:77: subject.addEventListener("DOMContentLoaded",
handlePageLoad);
Detail: We usually explicitly specify the "useCapture" argument.

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js
File lib/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js...
lib/typoNetError.js:71: onShutdown.add(() => {
Coding style: Opening braces always go on their own line.

This applies everywhere where you call `onShutdown.add()`.

[Wladimir Palant, 2016-03-08 14:37:48]

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
File lib/child/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
lib/child/typoNetError.js:47: netErrorOverlay =
parser.parseFromString(message.data, "application/xml");
On 2016/03/08 12:34:43, Thomas Greiner wrote:
> Note that previously we were first loading the error overlay and then
installing
> the `WindowObserver` which installed the "DOMContentLoaded" listener. Now,
> however, we're installing the "DOMContentLoaded" listener in parallel to
> requesting the error overlay.

Yes. There is a check for netErrorOverlay in the event handler so doing this
sequentially is unnecessary.

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
lib/child/typoNetError.js:72: if (topic != "content-document-global-created")
On 2016/03/08 12:34:44, Thomas Greiner wrote:
> Detail: Is this check necessary given that you've already specified in line
134
> that you're only listening to "content-document-global-created"?

It's a somewhat theoretical scenario but some misguided extension could be
calling observers directly (e.g. after obtaining a list via
nsIObserverService.enumerateObservers()) - and they might be calling with a
different topic because the observer they mean to trigger isn't ours at all.
Yes, those things happen...

https://codereview.adblockplus.org/29337915/diff/29337923/lib/child/typoNetEr...
lib/child/typoNetError.js:77: subject.addEventListener("DOMContentLoaded",
handlePageLoad);
On 2016/03/08 12:34:43, Thomas Greiner wrote:
> Detail: We usually explicitly specify the "useCapture" argument.

Well, we can stop doing that - it's optional ever since Firefox 6.

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js
File lib/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js...
lib/typoNetError.js:71: onShutdown.add(() => {
On 2016/03/08 12:34:44, Thomas Greiner wrote:
> Coding style: Opening braces always go on their own line.

That style makes relatively little sense when used with arrow functions and so
far we've been consistently putting the braces on the same line in this case.
Yes, I'm looking forward to automatic style checks...

[Thomas Greiner, 2016-03-11 17:49:21]

Thanks for the explanations. I'd just like to finish off the part regarding the
coding style.

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js
File lib/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js...
lib/typoNetError.js:71: onShutdown.add(() => {
On 2016/03/08 14:37:48, Wladimir Palant wrote:
> On 2016/03/08 12:34:44, Thomas Greiner wrote:
> > Coding style: Opening braces always go on their own line.
> 
> That style makes relatively little sense when used with arrow functions and so
> far we've been consistently putting the braces on the same line in this case.
> Yes, I'm looking forward to automatic style checks...

So we'll have yet another exception to that rule? I don't see how we can keep
this consistent.

[Wladimir Palant, 2016-03-13 21:56:52]

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js
File lib/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js...
lib/typoNetError.js:71: onShutdown.add(() => {
On 2016/03/11 17:49:21, Thomas Greiner wrote:
> So we'll have yet another exception to that rule? I don't see how we can keep
> this consistent.

I consulted eslint and it thinks that this shouldn't be an exception.

[Thomas Greiner, 2016-03-14 11:06:29]

LGTM after the last change is applied to lib/child/typoNetError.js as well.

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js
File lib/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js...
lib/typoNetError.js:71: onShutdown.add(() => {
On 2016/03/13 21:56:52, Wladimir Palant wrote:
> On 2016/03/11 17:49:21, Thomas Greiner wrote:
> > So we'll have yet another exception to that rule? I don't see how we can
keep
> > this consistent.
> 
> I consulted eslint and it thinks that this shouldn't be an exception.

Thanks for checking. Note that this is also the case in
lib/child/typoNetError.js

[Wladimir Palant, 2016-03-14 14:19:36]

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js
File lib/typoNetError.js (right):

https://codereview.adblockplus.org/29337915/diff/29337923/lib/typoNetError.js...
lib/typoNetError.js:71: onShutdown.add(() => {
On 2016/03/14 11:06:29, Thomas Greiner wrote:
> Thanks for checking. Note that this is also the case in
> lib/child/typoNetError.js

Heh, missed that due to misconfiguring eslint...