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

Issue 29332902: Issue 3443 - Use process script instead of a frame script in Element Hiding Helper (Closed)

Created:
Dec. 21, 2015, 12:45 p.m. by Wladimir Palant
Modified:
Dec. 23, 2015, 12:52 p.m.
Reviewers:
saroyanm
Visibility:
Public.

Description

This is mostly a straightforward change, with the old JS module becoming the process script (messaging endpoints move there from the old frame script). However, element selection no longer knows which process script to communicate with - so it has to broadcast the message, and the process scripts decide whether to respond depending on whether the element belongs to their process.

Patch Set 1 #

Total comments: 9

Patch Set 2 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -88 lines) Patch
M chrome/content/composer.js View 1 chunk +2 lines, -2 lines 0 comments Download
R chrome/content/frameScript.js View 1 1 chunk +0 lines, -49 lines 0 comments Download
M chrome/content/processScript.js View 1 2 chunks +35 lines, -21 lines 0 comments Download
M lib/aardvark.js View 1 chunk +6 lines, -8 lines 0 comments Download
M lib/main.js View 1 chunk +5 lines, -5 lines 0 comments Download
M metadata.gecko View 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 4
Wladimir Palant
Dec. 21, 2015, 12:45 p.m. (2015-12-21 12:45:09 UTC) #1
saroyanm
https://codereview.adblockplus.org/29332902/diff/29332903/chrome/content/processScript.js File chrome/content/processScript.js (right): https://codereview.adblockplus.org/29332902/diff/29332903/chrome/content/processScript.js#newcode28 chrome/content/processScript.js:28: Nit: No new line should be needed here. https://codereview.adblockplus.org/29332902/diff/29332903/chrome/content/processScript.js#newcode31 ...
Dec. 21, 2015, 4:48 p.m. (2015-12-21 16:48:56 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29332902/diff/29332903/chrome/content/processScript.js File chrome/content/processScript.js (right): https://codereview.adblockplus.org/29332902/diff/29332903/chrome/content/processScript.js#newcode28 chrome/content/processScript.js:28: On 2015/12/21 16:48:55, saroyanm wrote: > Nit: No new ...
Dec. 21, 2015, 7:14 p.m. (2015-12-21 19:14:16 UTC) #3
saroyanm
Dec. 23, 2015, 10:57 a.m. (2015-12-23 10:57:05 UTC) #4
LGTM

https://codereview.adblockplus.org/29332902/diff/29332903/chrome/content/proc...
File chrome/content/processScript.js (right):

https://codereview.adblockplus.org/29332902/diff/29332903/chrome/content/proc...
chrome/content/processScript.js:31: var onShutdown = (function()
On 2015/12/21 19:14:15, Wladimir Palant wrote:
> On 2015/12/21 16:48:55, saroyanm wrote:
> > I think it's something that I pointed in another review, I think we should
be
> > fine declaring this as "let".
> 
> True, this is no longer necessary. In fact, the complicated guard against
> multiple invocation isn't really necessary either - I removed it.

Looks good.

Powered by Google App Engine
This is Rietveld