Integrated URL Fixer into Adblock Plus

Integrated URL Fixer into Adblock Plus

[Wladimir Palant, 2012-11-06 15:48:03]

Publishing a partial review to get you going.

Note that typo correction functionality in Adblock Plus needs to disable
automatically (and the corresponding UI needs to be hidden) if URL Fixer is
installed.

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/filters.js
File chrome/content/ui/filters.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/filters...
chrome/content/ui/filters.js:93: E("filtersContainer").style.visibility =
"visible";
a) Please use |hidden = true| or at least |collapsed = true| rather than
manipulating styles.
b) You should hide filtersSplitter element as well, that should make sure that
the tabbox can expand to the right.

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/filters...
File chrome/content/ui/filters.xul (right):

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/filters...
chrome/content/ui/filters.xul:309: <checkbox id="typo_enable"
label="&typoCorrections.enableLabel;"
oncommand="TypoActions.toggleEnable(this.checked)"/>
This isn't a "toggle" action if you pass in the value explicitly ;)
How about TypoActions.setEnabled()?

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/filters...
chrome/content/ui/filters.xul:320: <button flex="1"
label="&typoCorrections.removeButtonLabel;"
onclick="TypoActions.removeRule(this,'whitelist');" disabled="true"
id="typo_whitelist_remove-button" _list="typo_whitelist" />
There should be a space after the comma here.

Please use <hbox pack="right"> and remove the flex attribute - this will
position the button on the right side which is more in line with how the Adblock
Plus preferences look like.

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/typo.js
File chrome/content/ui/typo.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/typo.js...
chrome/content/ui/typo.js:1: /*
The name for this file should be more descriptive - typoSettings.js?

http://codereview.adblockplus.org/8559070/diff/7001/defaults/typoRules.json
File defaults/typoRules.json (right):

http://codereview.adblockplus.org/8559070/diff/7001/defaults/typoRules.json#n...
defaults/typoRules.json:1: {
This will increase the size of the download package quite significantly. Since
this is an opt-in feature I would really consider not shipping with default
rules - the rules can be downloaded when the extension is running.

http://codereview.adblockplus.org/8559070/diff/7001/lib/appSupport.js
File lib/appSupport.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/lib/appSupport.js#newcode17
lib/appSupport.js:17: exports.__proto__ = require("typoAppIntegration");
This is a hack. Also, __proto__ is bound to stop working in future. Please just
require() typoAppIntegration when you need it, no point merging that module with
ABP's appSupport.js.

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoRules.js
File lib/typoRules.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoRules.js#newcode84
lib/typoRules.js:84: let result = FileUtils.getFile("ProfD",
["url-fixer-rules.json"]);
The file name should depend on the extension - just in case URL Fixer and
Adblock Plus are installed in parallel. Maybe use require("info").addonName
somehow.

[Thomas Greiner, 2012-11-06 16:57:07]

> Note that typo correction functionality in Adblock Plus needs to disable
> automatically (and the corresponding UI needs to be hidden) if URL Fixer is
> installed.

Any preferred way on how I should do it? My first idea would be to find a way to
detect URL Fixer somehow (e.g. by searching for a specific XUL element) and if
it exists, disable the feature (which includes hiding the corresponding tab in
the settings) and also disable the notification asking the user to opt-in to
that feature.

...but also enabling it when URL Fixer is no longer installed. Therefore the
detection needs to run on each browser start.

[Wladimir Palant, 2012-11-07 08:07:25]

Thinking about it, the top priority right now should be landing this code -
disabling it when URL Fixer is detected can be done later.

Preferred way to detect it is via Add-on Manager API by the way. Main problem
being - it can be installed/uninstalled/disabled at any moment, not just the
browser start.

[Thomas Greiner, 2012-11-07 12:26:53]

On 2012/11/06 15:48:03, Wladimir Palant wrote:
http://codereview.adblockplus.org/8559070/diff/7001/defaults/typoRules.json#n...
> defaults/typoRules.json:1: {
> This will increase the size of the download package quite significantly. Since
> this is an opt-in feature I would really consider not shipping with default
> rules - the rules can be downloaded when the extension is running.

I think we can leave the static ones in there and only remove the automatically
generated dictionary. (before: ~400kB, after: ~500B)

On 2012/11/07 08:07:25, Wladimir Palant wrote:
> Thinking about it, the top priority right now should be landing this code -
> disabling it when URL Fixer is detected can be done later.

Agreed.

[Wladimir Palant, 2012-11-09 08:26:15]

Done with the review assuming that isTypoCorrectionEnabled was the only change
to URL Fixer code.

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/fennecS...
File chrome/content/ui/fennecSettings.xul (right):

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/fennecS...
chrome/content/ui/fennecSettings.xul:25: <setting
id="adblockplus-typoCorrection" type="bool"
title="&typoCorrections.enableLabel;" oncommand="/**See bug 762015*/ if
(event.type == 'oncommand') {event = document.createEvent('Events');
event.initEvent('command', false, false); this.dispatchEvent(event);}"/>
Please use a pref="" attribute and remove custom code handling this checkbox -
no special handling required here.

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/filters.js
File chrome/content/ui/filters.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/filters...
chrome/content/ui/filters.js:19: TypoActions.init();
Please don't initialize TypoActions explicitly here - they can be initialized in
their own file, like Backup or FilterActions or FilterSearch:

window.addEventListener("load", function()
{
  TypoActions.init();
}, false);

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/typo.js
File chrome/content/ui/typo.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/typo.js...
chrome/content/ui/typo.js:26: toggleEnable: function(checked)
This function doesn't toggle, it just sets a variable. Proper name would be
setEnabled().

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/typo.js...
chrome/content/ui/typo.js:28: E("typo_whitelist_container").style.visibility =
(checked) ? "visible" : "hidden";
Please don't assume that the pref can only change in this dialog.
|Prefs.correctTypos = checked| is all you should need to do. You add code to
onPrefChange along these lines:

  if (name == "correctTypos")
    TypeActions.updateState();

Here updateState() should have the functionality that you currently have in the
init() function - check/uncheck the checkbox and hide/unhide the list.

http://codereview.adblockplus.org/8559070/diff/7001/chrome/content/ui/typo.js...
chrome/content/ui/typo.js:93: window.addEventListener("unload", function()
Prefs.removeListener(TypoActions.onPrefChange), false);
These two lines should be in the init() function.

http://codereview.adblockplus.org/8559070/diff/7001/defaults/prefs.js
File defaults/prefs.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/defaults/prefs.js#newcode35
defaults/prefs.js:35: pref("extensions.adblockplus.correctTyposAsk", false);
Judging from the logic, this preference should be named "correctTyposAsked" - it
determines whether the user has been asked, not whether we still need to ask
him.

http://codereview.adblockplus.org/8559070/diff/7001/lib/main.js
File lib/main.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/lib/main.js#newcode29
lib/main.js:29: require("typoFixer");
Please check preferences here - for the (Prefs.correctTyposAsked &&
!Prefs.correctTypos) case we shouldn't load these modules at all. No memory
impact and no useless rules downloads.

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoAppIntegration.js
File lib/typoAppIntegration.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoAppIntegration.js...
lib/typoAppIntegration.js:66: Prefs.correctTyposAsk = true;
No, this flag should only be set once the user makes a decision. The
notification might disappear for one reason or another, if the user didn't click
a button we should show it again next time.

Also, please make sure to set this flag if the user changes correctTypos
preference manually (e.g. in our settings) - user found the feature, we
shouldn't ask any more. Something along these lines:

    Prefs.addListener(function(pref)
    {
      if (pref == "correctTypos")
        Prefs.correctTyposAsked = true;
    });

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoAppIntegration.js...
lib/typoAppIntegration.js:68: return false;
This line is unnecessary - if you remove it we will just return
Prefs.correctTypos which happens to be false here. This will be more consistent.

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoFixer.js
File lib/typoFixer.js (right):

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoFixer.js#newcode145
lib/typoFixer.js:145: return;
This should be |return null|

http://codereview.adblockplus.org/8559070/diff/7001/lib/typoFixer.js#newcode176
lib/typoFixer.js:176: appIntegration.openInfobar(window,
"url-fixer-infobar-askafter", message, buttons, 1);
This should be dependent on addonName as well, just in case.

[Wladimir Palant, 2012-11-09 13:20:51]

http://codereview.adblockplus.org/8559070/diff/23001/chrome/content/ui/typoSe...
File chrome/content/ui/typoSettings.js (right):

http://codereview.adblockplus.org/8559070/diff/23001/chrome/content/ui/typoSe...
chrome/content/ui/typoSettings.js:27: TypoActions.updateList();
Are you making extra sure to handle the whitelist preference? :)

http://codereview.adblockplus.org/8559070/diff/23001/chrome/content/ui/typoSe...
chrome/content/ui/typoSettings.js:98:
E("typo_whitelist_container").style.visibility = (enabled) ? "visible" :
"hidden";
Please use .hidden property here, forgot to note that in the previous review.

http://codereview.adblockplus.org/8559070/diff/23001/defaults/typoRules.json
File defaults/typoRules.json (right):

http://codereview.adblockplus.org/8559070/diff/23001/defaults/typoRules.json#...
defaults/typoRules.json:19: "domain":{}
You probably want dummies for "domainReferrals" and "scheme" keys as well.

http://codereview.adblockplus.org/8559070/diff/23001/lib/main.js
File lib/main.js (right):

http://codereview.adblockplus.org/8559070/diff/23001/lib/main.js#newcode14
lib/main.js:14: let {Prefs} = require("prefs");
I think this should be added below so that its startup time impact is measured.

[Wladimir Palant, 2012-11-09 14:44:31]

Two last comments from the previous review not addressed yet.

http://codereview.adblockplus.org/8559070/diff/33002/lib/main.js
File lib/main.js (right):

http://codereview.adblockplus.org/8559070/diff/33002/lib/main.js#newcode45
lib/main.js:45: }
I'm not really happy with advanced logic being moved to main.js. How about
having this code in typoAppIntegration.js, you deal with these flags there
already.

[Wladimir Palant, 2012-11-09 15:32:22]

LGTM, please land this.

The prefs observer you added to typoAppIntegration.js also needs to be removed
if correctTyposAsked preference changes to true elsewhere - please fix that
later (separate review).