Applied changes from emailed code review

Applied changes from emailed code review

[Wladimir Palant, 2012-09-21 14:40:25]

When you address the review comments please update this review with a new
patchset created against the *same* revision as the previous one. This is
non-obvious but it actually makes things easier to review, this application does
a pretty good job comparing patchsets.

I am listing review comments from previous review that haven't been addressed
yet to make sure these aren't lost (and - yes, I see that you already addressed
some after you created that review):

* Survey shouldn't be a dialog, the panel needs to be injected into the browser
window.
* updateRules.js as a file name breaks naming convention (file name should be a
noun, not a verb).
* Rules logic (server-supplied rules, user's own whitelist etc.) is not what it
should be.
* Prefs.custom_replace logic is wrong (seems to look for exact matches instead
of substring matches).
* The code working with the infobar is Firefox-specific and will break on
Fennec.
* Replacing notification by a new one doesn't change the action performed when
"No" is clicked.

http://codereview.adblockplus.org/8382011/diff/1/chrome/content/options.js
File chrome/content/options.js (right):

http://codereview.adblockplus.org/8382011/diff/1/chrome/content/options.js#ne...
chrome/content/options.js:50: let prefRulesList = Prefs.custom_replace;
Whitelist and custom rules are no longer mangled together, how about you also
separate the corresponding code here? Meaning: declare prefRulesList and
ruleList first, populate the latter, sort; then declare prefWhitelist and
whitelist, populate the latter, sort.

http://codereview.adblockplus.org/8382011/diff/1/chrome/content/options.js#ne...
chrome/content/options.js:140: function removeRule(btn, pref)
If you have _list as the attribute of the button then it would be consistent to
have _pref as an attribute as well instead of passing it as a parameter.

http://codereview.adblockplus.org/8382011/diff/1/chrome/content/options.js#ne...
chrome/content/options.js:150: Prefs[pref] =
JSON.parse(JSON.stringify(Prefs[pref]));
It's too bad that we have to use that hack. This needs to be solved properly in
the prefs module...

http://codereview.adblockplus.org/8382011/diff/1/lib/rules.js
File lib/rules.js (right):

http://codereview.adblockplus.org/8382011/diff/1/lib/rules.js#newcode159
lib/rules.js:159: }
That's not what should be done with the whitelist. Suppose you have "foo.com" in
the whitelist, should "example.foo.com" still be corrected into
"example.food.com"? I don't think so. So whitelist needs to be merged with the
server-supplied rules. This will have two advantages over the approach here:

a) Subdomains of a whitelisted domain won't be corrected.
b) Corrections towards whitelisted domains will be possible. E.g. if somebody
adds "mygreatwebsite.com" and mistypes it as "mygretwebsite.com" later - we
should correct that.

http://codereview.adblockplus.org/8382011/diff/1/lib/survey.js
File lib/survey.js (right):

http://codereview.adblockplus.org/8382011/diff/1/lib/survey.js#newcode49
lib/survey.js:49:
Services.wm.getMostRecentWindow("navigator:browser").openDialog("chrome://url-fixer/content/survey.xul",
"survey", "top=100,left=300,chrome,dialog,dependent");
a) I'm not insisting on using 80 characters as line length limit but this line
is way too long. Please split it up.
b) getMostRecentWindow() isn't guaranteed to return something, it could also be
null. Please check its result.

Still not fixed from the previous review: the survey question shouldn't be a
dialog.

http://codereview.adblockplus.org/8382011/diff/1/lib/survey.js#newcode55
lib/survey.js:55:
require("appIntegration").getBrowser(Services.wm.getMostRecentWindow("navigator:browser")).loadOneTab(surveyUrl
+ "&lang=" + surveyLang, {
Same here - line too long and result of getMostRecentWindow() unchecked.

http://codereview.adblockplus.org/8382011/diff/1/lib/typedItCollector.js
File lib/typedItCollector.js (right):

http://codereview.adblockplus.org/8382011/diff/1/lib/typedItCollector.js#newc...
lib/typedItCollector.js:159: postData.domains = domains;
Now we are using JSON but not its advantages - the server is still receiving
three lists of domains that it has to compare. As mentioned in previous review,
it would be better to store and send structured data: each entry should
correspond to one string the user typed into the URL bar. That entry should
contain what the user type in, if we autocorrected it - what we corrected it
into and whether the user marked it as a false positive.

http://codereview.adblockplus.org/8382011/diff/1/lib/typedItCollector.js#newc...
lib/typedItCollector.js:185: request.send("data=" + JSON.stringify(postData));
There is little point to send JSON data in pseudo-urlencoded format.
Content-Type header should be application/json and request body just the plain
JSON data.

Side-note: Please verify that Unicode data is correctly encoded as UTF-8 here (I
think it is).

http://codereview.adblockplus.org/8382011/diff/1/lib/typoFixer.js
File lib/typoFixer.js (right):

http://codereview.adblockplus.org/8382011/diff/1/lib/typoFixer.js#newcode264
lib/typoFixer.js:264: }
Somebody reading this code now has to recognize that 4 is the length of the
string you looked for above; to verify correctness this somebody would have to
count the characters again and also check whether you made an off-by-one error
somewhere. This is bad for code maintainability.

How about value = value.replace(/^www\./, '') without any checks?

http://codereview.adblockplus.org/8382011/diff/1/lib/typoFixer.js#newcode266
lib/typoFixer.js:266: Prefs.whitelist =
JSON.parse(JSON.stringify(Prefs.whitelist));
I understand why you wanted the whitelist to be a hash but why store the key as
value as well? Why not just assign true as the value?

[Wladimir Palant, 2012-09-28 09:29:34]

http://codereview.adblockplus.org/8382011/diff/9001/chrome/content/survey.xul
File chrome/content/survey.xul (right):

http://codereview.adblockplus.org/8382011/diff/9001/chrome/content/survey.xul...
chrome/content/survey.xul:17: <image id="icon"/>
You are adding this to a large document updated by a third party and with
content being added by other third parties (extensions). And IDs have to be
unique document-wide. So you just cannot use any generic IDs. Something like
"url-fixer-icon" should do however (and similarly everywhere else).

http://codereview.adblockplus.org/8382011/diff/9001/chrome/content/survey.xul...
chrome/content/survey.xul:25: <space flex="1"/>
You probably meant <spacer>. Either way, just add pack="end" attribute on the
<hbox>, this will be sufficient.

http://codereview.adblockplus.org/8382011/diff/9001/chrome/content/survey.xul...
chrome/content/survey.xul:27: <button id="cancel-button"
dlgtype="cancel">&urlfixer.survey.decline;</button>
dlgtype attribute only makes sense in dialogs.

http://codereview.adblockplus.org/8382011/diff/9001/chrome/skin/survey.css
File chrome/skin/survey.css (right):

http://codereview.adblockplus.org/8382011/diff/9001/chrome/skin/survey.css#ne...
chrome/skin/survey.css:3: width: 600px;
Is it necessary to specify the width multiple time? Panel sizing should be
determined automatically by its contents - and you already defined the width for
the only flexible child (question_container).

http://codereview.adblockplus.org/8382011/diff/9001/lib/appIntegration.js
File lib/appIntegration.js (right):

http://codereview.adblockplus.org/8382011/diff/9001/lib/appIntegration.js#new...
lib/appIntegration.js:68: let notif = infobar.getNotificationWithValue(id);
As mentioned before - you don't need to save characters in an extension and your
editor has autocomplete. So nothing wrong with writing out "notification".

http://codereview.adblockplus.org/8382011/diff/9001/lib/appIntegration.js#new...
lib/appIntegration.js:87:
exports.getBrowser(Services.wm.getMostRecentWindow("navigator:browser")).loadURI(uri);
This will load the address in *some* browser window which is not necessarily the
one we want. It's bad enough that we don't specify the tab - but assuming that
the right tab is selected when the user clicks a notification button isn't
unreasonable. We should pass the window as a parameter explicitly however.

(Note that this issue was addressed in one of my changes)

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js
File lib/survey.js (right):

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode52
lib/survey.js:52: request.setRequestHeader("Content-Type", "text/html");
What's that good for in a non-network request? Remove this line?

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode58
lib/survey.js:58: let style =
document.createProcessingInstruction('xml-stylesheet',
'href="chrome://url-fixer/skin/survey.css?' + (new Date().getTime()) + '"
type="text/css"');
Why randomize the URL here? It's a static file, not going to change.

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode68
lib/survey.js:68: panel = document.getElementById(panel.id);
Our panel is already in the browser document at this point? This is a sign of
big issues - probably a panel from the previous version that wasn't removed for
some reason. We should not try to reuse it, it might not be compatible with
current code. If anything, we should remove it from the document.

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode70
lib/survey.js:70: panel.querySelector("#icon").setAttribute("src",
require("info").addonRoot + "icon64.png");
If you use unique IDs then you can use document.getElementById() instead of
querySelector().

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode71
lib/survey.js:71:
panel.querySelector("#accept-button").addEventListener("click", function()
Don't listen to "click" events, these are mouse-only in XUL. There is a
"command" event that deals with all kinds of input.

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode72
lib/survey.js:72: {
This bracket should not be indented...

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode78
lib/survey.js:78: {
This bracket should not be indented...

http://codereview.adblockplus.org/8382011/diff/9001/lib/survey.js#newcode91
lib/survey.js:91:
require("appIntegration").getBrowser(Services.wm.getMostRecentWindow("navigator:browser")).loadOneTab(surveyUrl
+ "&lang=" + surveyLang, {
Taking over comment from previous review: line too long and result of
getMostRecentWindow() unchecked.

http://codereview.adblockplus.org/8382011/diff/9001/lib/typedItCollector.js
File lib/typedItCollector.js (right):

http://codereview.adblockplus.org/8382011/diff/9001/lib/typedItCollector.js#n...
lib/typedItCollector.js:1: /* This Source Code Form is subject to the terms of
the Mozilla Public
Comments from previous review haven't been addressed in this file - one of my
changes addresses them however.

http://codereview.adblockplus.org/8382011/diff/9001/lib/typoFixer.js
File lib/typoFixer.js (right):

http://codereview.adblockplus.org/8382011/diff/9001/lib/typoFixer.js#newcode233
lib/typoFixer.js:233: let [message, yes, no, cancel] = getInfobarTexts();
You don't use a Cancel button - please remove the variable and the corresponding
strings.

http://codereview.adblockplus.org/8382011/diff/9001/lib/typoFixer.js#newcode254
lib/typoFixer.js:254: Prefs.whitelist[value] = value;
Comments from previous review not addressed here - one of my changes replaced
the code here however.

http://codereview.adblockplus.org/8382011/diff/9001/lib/typoFixer.js#newcode262
lib/typoFixer.js:262: require("appIntegration").openInfobar(window,
"url-fixer-infobar-askafter", message, buttons, 1);
It isn't obvious why we pass 1 for persistence here, I had to look it up myself.
Reason is that we are about to load a different page which will change location
- so the message needs to survive one location change. Maybe add a comment
explaining that?

[Wladimir Palant, 2012-09-28 11:29:33]

Please remove urlfixer.cancel from chrome/locale/en-US/locale.properties as
well.

http://codereview.adblockplus.org/8382011/diff/22001/lib/survey.js
File lib/survey.js (right):

http://codereview.adblockplus.org/8382011/diff/22001/lib/survey.js#newcode78
lib/survey.js:78: });
Forgot to mention in the previous review: you shouldn't leave the pop-up around,
please clean up after yourself (e.g. in case the extension is updated - you
don't want the old styles still around). Attaching a "popuphidden" event
listener to the panel and removing the nodes from the document there should do.

[Wladimir Palant, 2012-09-28 13:31:26]

http://codereview.adblockplus.org/8382011/diff/21002/chrome/content/survey.xul
File chrome/content/survey.xul (right):

http://codereview.adblockplus.org/8382011/diff/21002/chrome/content/survey.xu...
chrome/content/survey.xul:14: <label
id="url-fixer-title">&urlfixer.survey.title;</label>
Maybe <description> rather than <label>? Labels are meant to be used to label
controls, description is a regular text.

http://codereview.adblockplus.org/8382011/diff/21002/lib/survey.js
File lib/survey.js (right):

http://codereview.adblockplus.org/8382011/diff/21002/lib/survey.js#newcode81
lib/survey.js:81: panel.parentNode.removeChild(panel);
Please remove the style node as well.

[Wladimir Palant, 2012-09-28 14:24:57]

LGTM