Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29338861: Issue 3851 - Implement subscribe link handling via process scripts (Closed)

Created:
March 21, 2016, 7:25 p.m. by Wladimir Palant
Modified:
April 18, 2016, 5:54 p.m.
Reviewers:
Thomas Greiner, Erik
Visibility:
Public.

Description

Issue 3851 - Implement subscribe link handling via process scripts Repository: hg.adblockplus.org/adblockplus

Patch Set 1 #

Total comments: 9

Patch Set 2 : Added guard and removed dead code #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -99 lines) Patch
M lib/child/main.js View 1 chunk +1 line, -0 lines 0 comments Download
M lib/child/subscribeLinks.js View 1 1 chunk +94 lines, -82 lines 1 comment Download
M lib/ui.js View 1 3 chunks +5 lines, -17 lines 0 comments Download

Messages

Total messages: 7
Wladimir Palant
March 21, 2016, 7:25 p.m. (2016-03-21 19:25:11 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js File lib/child/subscribeLinks.js (right): https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js#newcode42 lib/child/subscribeLinks.js:42: function onClick(event) It's mostly an indentation change for this ...
March 21, 2016, 7:27 p.m. (2016-03-21 19:27:20 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js File lib/child/subscribeLinks.js (right): https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js#newcode42 lib/child/subscribeLinks.js:42: function onClick(event) On 2016/03/21 19:27:20, Wladimir Palant wrote: > ...
March 21, 2016, 7:29 p.m. (2016-03-21 19:29:20 UTC) #3
Erik
https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js File lib/child/subscribeLinks.js (right): https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js#newcode45 lib/child/subscribeLinks.js:45: return; This will stop the function from running after ...
April 6, 2016, 3:11 a.m. (2016-04-06 03:11:41 UTC) #4
Thomas Greiner
Just one question from me. https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js File lib/child/subscribeLinks.js (right): https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js#newcode32 lib/child/subscribeLinks.js:32: if (subject instanceof Ci.nsIDOMWindow ...
April 18, 2016, 3:16 p.m. (2016-04-18 15:16:25 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js File lib/child/subscribeLinks.js (right): https://codereview.adblockplus.org/29338861/diff/29338862/lib/child/subscribeLinks.js#newcode32 lib/child/subscribeLinks.js:32: if (subject instanceof Ci.nsIDOMWindow && subject.top == subject) On ...
April 18, 2016, 3:38 p.m. (2016-04-18 15:38:40 UTC) #6
Thomas Greiner
April 18, 2016, 5:49 p.m. (2016-04-18 17:49:43 UTC) #7
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!

Powered by Google App Engine
This is Rietveld