Issue 3222 - Split up requestNotifier module into a parent and child part

This only splits up the RequestNotifier class so that blockable items list can work (after adjusting constructor parameters - outerWindowID instead of window). RequestNotifier.getWindowStatistics() and RequstNotifier.getDataForNode() are still broken, these will aren`t used by blockable items and will be fixed in other issues. RequestNotifier.storeSelection() on the other hand works just fine via wrappers - I`ll file a follow-up issue on adjusting this one for E10S but priority is lower here.

[tschuster, 2015-11-07 12:38:57]

On 2015/11/02 19:48:43, Wladimir Palant wrote:

LGTM

[Thomas Greiner, 2015-11-12 12:24:06]

https://codereview.adblockplus.org/29329654/diff/29329659/lib/child/requestNo...
File lib/child/requestNotifier.js (right):

https://codereview.adblockplus.org/29329654/diff/29329659/lib/child/requestNo...
lib/child/requestNotifier.js:63: * @param {Integer} id  Parent notifier ID to be
messaged
Detail: The documented parameter names here do not correspond to the actual
parameter names.

https://codereview.adblockplus.org/29329654/diff/29329659/lib/child/requestNo...
lib/child/requestNotifier.js:132: if (!this.window)
Detail: I guess you don't need to pass `wnd` anymore because you removed the
check in the constructor and use `this.window` here anyway.

https://codereview.adblockplus.org/29329654/diff/29329659/lib/requestNotifier.js
File lib/requestNotifier.js (left):

https://codereview.adblockplus.org/29329654/diff/29329659/lib/requestNotifier...
lib/requestNotifier.js:111: this.listener.call(this.listenerObj, wnd, node,
entry, this.scanComplete);
From what I see, this will break stuff (namely
chrome/content/ui/{sendReport,sidebar}.js) because the listeners are expecting
those parameters to be present. Is that something that's already planned to get
fixed in other reviews?

Or are our own pages not even getting notified anymore? Because I also noticed
that the change in the constructor signature has also not been considered in
those files.

[Wladimir Palant, 2015-11-12 12:40:57]

https://codereview.adblockplus.org/29329654/diff/29329659/lib/child/requestNo...
File lib/child/requestNotifier.js (right):

https://codereview.adblockplus.org/29329654/diff/29329659/lib/child/requestNo...
lib/child/requestNotifier.js:63: * @param {Integer} id  Parent notifier ID to be
messaged
On 2015/11/12 12:24:06, Thomas Greiner wrote:
> Detail: The documented parameter names here do not correspond to the actual
> parameter names.

Done.

https://codereview.adblockplus.org/29329654/diff/29329659/lib/child/requestNo...
lib/child/requestNotifier.js:132: if (!this.window)
On 2015/11/12 12:24:06, Thomas Greiner wrote:
> Detail: I guess you don't need to pass `wnd` anymore because you removed the
> check in the constructor and use `this.window` here anyway.

You misunderstood the check here. startScan is being called recursively for
frames, so the wnd parameter is important (and not necessarily identical to
this.window). The point of this check is aborting the scan if the notifier was
shut down. I added a comment explaining that.

https://codereview.adblockplus.org/29329654/diff/29329659/lib/requestNotifier.js
File lib/requestNotifier.js (left):

https://codereview.adblockplus.org/29329654/diff/29329659/lib/requestNotifier...
lib/requestNotifier.js:111: this.listener.call(this.listenerObj, wnd, node,
entry, this.scanComplete);
On 2015/11/12 12:24:06, Thomas Greiner wrote:
> From what I see, this will break stuff (namely
> chrome/content/ui/{sendReport,sidebar}.js) because the listeners are expecting
> those parameters to be present. Is that something that's already planned to
get
> fixed in other reviews?

Of course that will break stuff - we change the API here, not merely the
listeners but also RequestNotifier constructor parameters. There are follow-up
issues for all users of this API, all of them under review. All of that stuff is
broken by the changes in https://codereview.adblockplus.org/29329571/ already,
which is why these changes are supposed to land on a branch until everything is
working again.

Note that the wnd parameter isn't actually used anywhere, so removing this one
isn't an issue. Designing a proper replacement for the node parameter on the
other hand was rather tricky.

[Thomas Greiner, 2015-11-12 14:15:57]

LGTM