added hook function to appIntegration to handle function-overwrites from other extensions

added hook function to appIntegration to handle function-overwrites from other extensions

[Wladimir Palant, 2012-09-26 09:27:50]

Note that I already modified the unit tests (mostly for consistency but also to
make sure that they test the real code and not a copy of it).

Also note that your SeaMonkey code is completely broken - please don't change
app-dependent code blindly, always try it in the respective app.

Obviously, the code for Fennec (both variants) still needs to be adjusted.

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js
File lib/appIntegration.js (right):

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode5
lib/appIntegration.js:5: function hook(obj, name, func)
Any reason why you've put it here? I expected it to be in a separate module -
this is generic functionality that is not application-specific.

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode10
lib/appIntegration.js:10: var dumbOverrideAttempt = false;
You should use let instead of var in Firefox code - the scope of let variables
is defined in a much cleaner way, unexpected scope issues are unlikely here.

(Note that I fixed that issue in one of my changes already)

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode35
lib/appIntegration.js:35: );
Two levels of indentation for a single closure function is weird. Normally, the
brackets aren't indented and only the function body is.

(Note that I fixed this issue in one of my changes already)

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode60
lib/appIntegration.js:60: }
Inconsistent style here (brackets) - plus there is a missing semicolon at the
return statement (only mentioning because missing semicolons after function
literals seems to be a recurring issue in your code).

(Note that I fixed this issue in one of my changes already)

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode63
lib/appIntegration.js:63: let wm = new WeakMap();
"wm" stands for "WeakMap" I guess? A proper variable name should reflect the
purpose, not the implementation - how about "unhookCallbacks" or something like
that?

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode64
lib/appIntegration.js:64: let {Prefs} = require("prefs");
I would be very worried if our generic app-support code were suddenly dependent
on preferences... This seems unused however, remove?

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode83
lib/appIntegration.js:83: let hooks = [];
From the look of it, we never hook more than one function (in Fennec native we
hook additional functions only temporarily - that's different). How about
putting a single unhook function into the map instead of an array?

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode84
lib/appIntegration.js:84: if (wm.get(window))
The window should definitely be unknown here - if we are called twice for the
same window then we have bigger problems than having to merge hooks. Instead of
the check here you probably want to replace the outdated !("urlfixerOldHandler"
in urlbar.handleCommand) check above by !wm.has(window).

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcode99
lib/appIntegration.js:99: }
The try..catch block here is no longer necessary - the hook() function already
takes care of that.

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcod...
lib/appIntegration.js:109: if (urlbar && urlbar.handleCommand && wm.has(window))
There is little point keeping this check - either we have the window in the map
and then we can just call the unhook function or we don't. Also, the
functionality is no longer app-dependent then - I suggest putting it before the
switch statement so that the code is run for all applications.

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcod...
lib/appIntegration.js:160: if (urlbar && urlbar.handleCommand &&
!("urlfixerOldHandler" in urlbar.handleCommand))
Replacing SeaMonkey code by Firefox-specific code isn't going to work. The
function to hook is window.handleURLBarCommand in SeaMonkey, please restore the
functionality here.

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcod...
lib/appIntegration.js:186: 
I suggest the following setting for the Sublime Text editor:

  "trim_trailing_white_space_on_save": true

[Thomas Greiner, 2012-09-26 13:51:13]

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js
File lib/appIntegration.js (right):

http://codereview.adblockplus.org/8433028/diff/1/lib/appIntegration.js#newcod...
lib/appIntegration.js:186: 
On 2012/09/26 09:27:50, Wladimir Palant wrote:
> I suggest the following setting for the Sublime Text editor:
> 
>   "trim_trailing_white_space_on_save": true

I tried it but it interfers with my saving-behavior. I save every couple seconds
so if I make a new function, indent its body and save, the indentation is being
removed which made me freak out. :D

[Wladimir Palant, 2012-09-27 14:36:01]

Only a few minor nits this time. And - lots of my changes in there, having two
people work on the same branch definitely sucks for reviews.

http://codereview.adblockplus.org/8433028/diff/4001/lib/appIntegration.js
File lib/appIntegration.js (right):

http://codereview.adblockplus.org/8433028/diff/4001/lib/appIntegration.js#new...
lib/appIntegration.js:97: let unhook = hook(urlbar, "_fireEvent",
function(eventType) {
This bracket should be on next line.

http://codereview.adblockplus.org/8433028/diff/4001/lib/appIntegration.js#new...
lib/appIntegration.js:128: eventListener.element.removeEventListener("command",
eventListener.listener, true);
eventListeners.delete(window)?

http://codereview.adblockplus.org/8433028/diff/4001/lib/hooks.js
File lib/hooks.js (right):

http://codereview.adblockplus.org/8433028/diff/4001/lib/hooks.js#newcode22
lib/hooks.js:22: orig.apply(this, arguments);
This should be: return orig.apply(...)
We don't want to drop the return value if there is any.

http://codereview.adblockplus.org/8433028/diff/4001/lib/hooks.js#newcode30
lib/hooks.js:30: obj.__defineGetter__(name, function() {
This bracket should be on next line.

http://codereview.adblockplus.org/8433028/diff/4001/lib/hooks.js#newcode35
lib/hooks.js:35: obj.__defineSetter__(name, function(value) {
This bracket should be on next line.

[Wladimir Palant, 2012-09-28 09:35:19]

Lots of my own changes in this diff. One issue hasn't been addressed however.

http://codereview.adblockplus.org/8433028/diff/16001/lib/appIntegration.js
File lib/appIntegration.js (right):

http://codereview.adblockplus.org/8433028/diff/16001/lib/appIntegration.js#ne...
lib/appIntegration.js:127: eventListener.element.removeEventListener("command",
eventListener.listener, true);
Taking over comment from previous review:

eventListeners.delete(window)?

[Wladimir Palant, 2012-09-28 10:48:47]

LGTM