Detect and handle the case of our own typo correction extension being installed in parallel

Detect and handle the case of our own typo correction extension being installed in parallel

[Wladimir Palant, 2012-11-19 07:30:02]

http://codereview.adblockplus.org/8788183/diff/1/lib/main.js
File lib/main.js (right):

http://codereview.adblockplus.org/8788183/diff/1/lib/main.js#newcode32
lib/main.js:32: (function()
Please move that entire logic into a separate typoBootstrap.js module.

http://codereview.adblockplus.org/8788183/diff/1/lib/main.js#newcode51
lib/main.js:51: }
How about just doing require("typoFixer").attachWindowObserver() here and
leaving it up to attachWindowObserver to figure out whether it needs to do
anything? You should be able to save some of unnecessary state variables and
simplify the logic then.

http://codereview.adblockplus.org/8788183/diff/1/lib/main.js#newcode64
lib/main.js:64: }
How about just doing require("typoFixer").detachWindowObserver() here and
leaving it up to detachWindowObserver to figure out whether it need to do
anything? You should be able to save some of unnecessary state variables and
simplify the logic then.

http://codereview.adblockplus.org/8788183/diff/1/lib/main.js#newcode85
lib/main.js:85: if (!Prefs.correctTyposAsked || (Prefs.correctTyposAsked &&
Prefs.correctTypos))
This logic seems wrong. If typo corrections are disabled then we have no reason
to do anything - including watching URL Fixer. We should check the preferences
first and only install the add-on listener if we have a reason for that.

http://codereview.adblockplus.org/8788183/diff/1/lib/main.js#newcode129
lib/main.js:129: checkAddonStatusAndEnable();
Please ignore this call - URL Fixer is restartless, all operations are executed
immediately.

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

http://codereview.adblockplus.org/8788183/diff/1/lib/typoFixer.js#newcode20
lib/typoFixer.js:20: {
Please check whether the observer is already attached:

if (typoWindowObserver)
  return;

http://codereview.adblockplus.org/8788183/diff/1/lib/typoFixer.js#newcode47
lib/typoFixer.js:47: {
Please check whether the observer is already detached:

if (!typoWindowObserver)
  return;

[Wladimir Palant, 2012-11-19 13:49:45]

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js
File lib/typoBootstrap.js (right):

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:23: Cu.reportError("enable");
Debug code?

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:25: return;
This check (and the corresponding variable) should no longer be necessary.

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:33: Cu.reportError("disable");
Debug code?

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:35: return;
This check (and the corresponding variable) should no longer be necessary.

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:41: /*
Debug code?

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:111: */
Debug code?

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:114: function checkAndEnable(isInstalledAndEnabled)
Please separate the initialization and the update scenario, those should be two
separate functions.

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:119: Cu.reportError("check addon status and enable");
Debug code?

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:126: if (addon && !addon.userDisabled)
We should use isActive instead of userDisabled here, it will consider more
conditions.

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:169: Cu.reportError("feature disabled");
Debug code?

http://codereview.adblockplus.org/8788183/diff/6001/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:172: if (name == "correctTypos")
Please don't check just the name but also the new value.

[Wladimir Palant, 2012-11-19 17:24:23]

Nice, the logic got a lot easier to understand.

http://codereview.adblockplus.org/8788183/diff/7007/lib/typoBootstrap.js
File lib/typoBootstrap.js (right):

http://codereview.adblockplus.org/8788183/diff/7007/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:15: TimeLine.log("Done loading preferences");
Please remove this, it assumes that no other module will load preferences
earlier.

http://codereview.adblockplus.org/8788183/diff/7007/lib/typoBootstrap.js#newc...
lib/typoBootstrap.js:75: AddonManager.addAddonListener(addonListener);
This needs cleanup code - remove this listener on shutdown.

[Wladimir Palant, 2012-11-20 10:21:13]

http://codereview.adblockplus.org/8788183/diff/17001/lib/typoBootstrap.js
File lib/typoBootstrap.js (right):

http://codereview.adblockplus.org/8788183/diff/17001/lib/typoBootstrap.js#new...
lib/typoBootstrap.js:46: AddonManager.removeAddonListener(addonListener);
Please use onShutdown.add() to add a shutdown handler for that - right after you
add the listener. This is a global thing, not a per-window one. Please look
around for examples.

[Wladimir Palant, 2012-11-20 11:13:30]

http://codereview.adblockplus.org/8788183/diff/25001/lib/typoBootstrap.js
File lib/typoBootstrap.js (right):

http://codereview.adblockplus.org/8788183/diff/25001/lib/typoBootstrap.js#new...
lib/typoBootstrap.js:49: onShutdown.add(cleanup);
Seriously, please look around at how this is done everywhere else (search for
onShutdown.add). If we have to remember to keep cleanup handlers in sync with
the code this will become an unmanageable mess. The only way to keep things
manageable is to add a cleanup handler immediately after doing something that
requires reverting. Meaning that after calling AddonManager.addAddonListener()
the next line should be:

    onShutdown.add(function() AddonManager.removeAddonListener(...));

No need for any checks or variable cleanup.

[Wladimir Palant, 2012-11-20 11:41:51]

LGTM