Issue 3851 - Implement subscribe link handling via process scripts

Issue 3851 - Implement subscribe link handling via process scripts

Repository: hg.adblockplus.org/adblockplus

[Wladimir Palant, 2016-03-21 19:27:20]

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
File lib/child/subscribeLinks.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
lib/child/subscribeLinks.js:42: function onClick(event)
It's mostly an indentation change for this function. The only changes are
onShutdown.done check at the beginning and `link instanceof
content.HTMLAnchorElement` => `link instanceof Ci.nsIDOMHTMLAnchorElement` a bit
further down.

[Wladimir Palant, 2016-03-21 19:29:20]

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
File lib/child/subscribeLinks.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
lib/child/subscribeLinks.js:42: function onClick(event)
On 2016/03/21 19:27:20, Wladimir Palant wrote:
> It's mostly an indentation change for this function. The only changes are
> onShutdown.done check at the beginning and `link instanceof
> content.HTMLAnchorElement` => `link instanceof Ci.nsIDOMHTMLAnchorElement` a
bit
> further down.

Heh, forgot the sendAsyncMessage call at the bottom, it has been replaced by
port.emit.

[Erik, 2016-04-06 03:11:41]

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
File lib/child/subscribeLinks.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
lib/child/subscribeLinks.js:45: return;
This will stop the function from running after shutdown, but the function is
leaked if it runs after shutdown right?

https://codereview.adblockplus.org/29338861/diff/29338862/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/ui.js#newcode451
lib/ui.js:451: let frameScript =
"chrome://adblockplus/content/subscribeLinkHandler.js?" + Math.random();
should this line not be removed too?

[Thomas Greiner, 2016-04-18 15:16:25]

Just one question from me.

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
File lib/child/subscribeLinks.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
lib/child/subscribeLinks.js:32: if (subject instanceof Ci.nsIDOMWindow &&
subject.top == subject)
So we're not listening to clicks in subdocuments?

[Wladimir Palant, 2016-04-18 15:38:40]

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
File lib/child/subscribeLinks.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
lib/child/subscribeLinks.js:32: if (subject instanceof Ci.nsIDOMWindow &&
subject.top == subject)
On 2016/04/18 15:16:25, Thomas Greiner wrote:
> So we're not listening to clicks in subdocuments?

We do, clicks will bubble up from frames. I verified that.

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
lib/child/subscribeLinks.js:45: return;
On 2016/04/06 03:11:41, Erik wrote:
> This will stop the function from running after shutdown, but the function is
> leaked if it runs after shutdown right?

Yes, the function itself is leaked - getting the event handler removed after
shutdown would be rather tricky.

https://codereview.adblockplus.org/29338861/diff/29338862/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/ui.js#newcode451
lib/ui.js:451: let frameScript =
"chrome://adblockplus/content/subscribeLinkHandler.js?" + Math.random();
On 2016/04/06 03:11:41, Erik wrote:
> should this line not be removed too?

Done.

https://codereview.adblockplus.org/29338861/diff/29340507/lib/child/subscribe...
File lib/child/subscribeLinks.js (right):

https://codereview.adblockplus.org/29338861/diff/29340507/lib/child/subscribe...
lib/child/subscribeLinks.js:38: if (eventTarget)
Not sure when this happens but there are apparently windows without
chromeEventHandler - these must be special-purpose, not to be considered by us.

[Thomas Greiner, 2016-04-18 17:49:43]

LGTM

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
File lib/child/subscribeLinks.js (right):

https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribe...
lib/child/subscribeLinks.js:32: if (subject instanceof Ci.nsIDOMWindow &&
subject.top == subject)
On 2016/04/18 15:38:40, Wladimir Palant wrote:
> On 2016/04/18 15:16:25, Thomas Greiner wrote:
> > So we're not listening to clicks in subdocuments?
> 
> We do, clicks will bubble up from frames. I verified that.

Thanks for verifying!