Issue 1708 - Integrate first-run page changes in Chrome/Opera/Safari

Note that the dependency on adblockplusui is bogus right now as the corresponding changes haven`t been reviewed/pushed.

[Wladimir Palant, 2014-12-18 22:06:28]

Uploaded a rebased patch, no longer implementing Page.equals().

[Wladimir Palant, 2014-12-19 15:48:07]

Rebased this patch again, now a PageMap.keys() method is implemented instead of
having an ext.onPageRemoved event target. I also removed all the default message
handlers - responding to random messages is a certain way to cause issues, and
it seems to happen on the first-run page indeed (hard to reproduce
unfortunately).

[Sebastian Noack, 2014-12-19 17:57:52]

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
File background.js (left):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
background.js:570: default:
We discussed that earlier on IRC. So you made sure this doesn't result in memory
leaks on Safari?

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/ext/...
File ext/background.js (right):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/ext/...
ext/background.js:25: this._map = Object.create(null);
Is that an unrelated change in a line you wouldn't have touched otherwise? Fine
for me. But I guess, I still don't get it. ;P

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/opti...
File options.js (left):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/opti...
options.js:97: default:
While changing this code, mind changing the verbose switch-case to a simpler
if-statement? IMO there is no point to use switch-case to handle a single case.

[Wladimir Palant, 2014-12-19 19:38:42]

The dependency on adblockplusui actually points to an existing revision now.

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
File background.js (left):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
background.js:570: default:
On 2014/12/19 17:57:53, Sebastian Noack wrote:
> We discussed that earlier on IRC. So you made sure this doesn't result in
memory
> leaks on Safari?

Not more leaks than what we usually have (see "get-domain-enabled-state"
handler, it doesn't always send a response). As long as every message is handled
there are no memory leaks. And if we have unhandled messages expecting response
and send an empty object there then we have bigger issues than memory leaks.

That said: yes, it seems that our Safari messaging system is prone to memory
leaks. We should solve it the same way Chrome solved it: if the message handler
didn't return true and didn't call the callback then send "there won't be any
response, delete that callback" to the other side. I'd suggest fixing this in a
separate issue however, it's only marginally related.

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/ext/...
File ext/background.js (right):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/ext/...
ext/background.js:25: this._map = Object.create(null);
On 2014/12/19 17:57:53, Sebastian Noack wrote:
> Is that an unrelated change in a line you wouldn't have touched otherwise?
Fine
> for me. But I guess, I still don't get it. ;P

Yes, it's an unrelated change - I simply fixed this since I was changing this
code already.

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/opti...
File options.js (left):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/opti...
options.js:97: default:
On 2014/12/19 17:57:53, Sebastian Noack wrote:
> While changing this code, mind changing the verbose switch-case to a simpler
> if-statement? IMO there is no point to use switch-case to handle a single
case.

Done.

[Sebastian Noack, 2014-12-19 19:52:00]

LGTM, with the buildtools submodule update reverted.

Moreover, as discussed on IRC that change is too late for the upcoming release.
But feel free to push it as soon as we have released Adblock Plus 1.8.9 on
January 6th.

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
File background.js (left):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
background.js:570: default:
On 2014/12/19 19:38:42, Wladimir Palant wrote:
> That said: yes, it seems that our Safari messaging system is prone to memory
> leaks. We should solve it the same way Chrome solved it: if the message
handler
> didn't return true and didn't call the callback then send "there won't be any
> response, delete that callback" to the other side. I'd suggest fixing this in
a
> separate issue however, it's only marginally related.

Mind filing that issue? :)

http://codereview.adblockplus.org/6528746257383424/diff/5764640680181760/.hgs...
File .hgsubstate (right):

http://codereview.adblockplus.org/6528746257383424/diff/5764640680181760/.hgs...
.hgsubstate:4: 7e6c948937d942737dccfaacbadf1ed33c459cb9 buildtools
Note that the buildtools submodule has been updated recently (to a more recent
revision).

[Wladimir Palant, 2014-12-19 20:10:51]

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
File background.js (left):

http://codereview.adblockplus.org/6528746257383424/diff/5766466041282560/back...
background.js:570: default:
On 2014/12/19 19:52:00, Sebastian Noack wrote:
> Mind filing that issue? :)

Sure: https://issues.adblockplus.org/ticket/1724

http://codereview.adblockplus.org/6528746257383424/diff/5764640680181760/.hgs...
File .hgsubstate (right):

http://codereview.adblockplus.org/6528746257383424/diff/5764640680181760/.hgs...
.hgsubstate:4: 7e6c948937d942737dccfaacbadf1ed33c459cb9 buildtools
On 2014/12/19 19:52:00, Sebastian Noack wrote:
> Note that the buildtools submodule has been updated recently (to a more recent
> revision).

Done.

[Sebastian Noack, 2014-12-19 20:12:58]

Still LGTM