Issue 3259 - Store blockable items selection in the content process

Issue 3259 - Store blockable items selection in the content process

[Thomas Greiner, 2016-01-05 15:43:59]

https://codereview.adblockplus.org/29333172/diff/29333173/lib/requestNotifier.js
File lib/requestNotifier.js (right):

https://codereview.adblockplus.org/29333172/diff/29333173/lib/requestNotifier...
lib/requestNotifier.js:31: let windowDataCallbacks = new Map();
I noticed that most of the message logic (windowDataMaxResponseID,
windowDataCallbacks, onWindowDataReceived, retrieveWindowData) is identical to
the one for window stats so I'm wondering why you didn't reuse but instead
duplicated all of that.

Since there are a bunch of inconsistencies already (e.g. naming inconsistency
with `retrieveWindowData()` vs. `getWindowStats()` and
`onRetrieveWindowStats()`), I assume that the issue is that you don't want to
modify too much of that code or is there a different reason?

[Wladimir Palant, 2016-01-06 12:31:58]

https://codereview.adblockplus.org/29333172/diff/29333173/lib/requestNotifier.js
File lib/requestNotifier.js (right):

https://codereview.adblockplus.org/29333172/diff/29333173/lib/requestNotifier...
lib/requestNotifier.js:31: let windowDataCallbacks = new Map();
On 2016/01/05 15:43:58, Thomas Greiner wrote:
> I noticed that most of the message logic (windowDataMaxResponseID,
> windowDataCallbacks, onWindowDataReceived, retrieveWindowData) is identical to
> the one for window stats so I'm wondering why you didn't reuse but instead
> duplicated all of that.

The messaging logic is very suboptimal right now, and unifying it here would be
the wrong approach. Instead, I want our code to require("messaging") and use a
proper API. The goal is providing simple messaging and implicit callbacks, then
the ext.backgroundPage.sendMessage() implementation should use the new messaging
API as well.

[Thomas Greiner, 2016-01-06 13:16:22]

LGTM

https://codereview.adblockplus.org/29333172/diff/29333173/lib/requestNotifier.js
File lib/requestNotifier.js (right):

https://codereview.adblockplus.org/29333172/diff/29333173/lib/requestNotifier...
lib/requestNotifier.js:31: let windowDataCallbacks = new Map();
On 2016/01/06 12:31:58, Wladimir Palant wrote:
> On 2016/01/05 15:43:58, Thomas Greiner wrote:
> > I noticed that most of the message logic (windowDataMaxResponseID,
> > windowDataCallbacks, onWindowDataReceived, retrieveWindowData) is identical
to
> > the one for window stats so I'm wondering why you didn't reuse but instead
> > duplicated all of that.
> 
> The messaging logic is very suboptimal right now, and unifying it here would
be
> the wrong approach. Instead, I want our code to require("messaging") and use a
> proper API. The goal is providing simple messaging and implicit callbacks,
then
> the ext.backgroundPage.sendMessage() implementation should use the new
messaging
> API as well.

Sounds great. In that case I don't mind implementing it like it is for now.