Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(30)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by Wladimir Palant
Modified:
3 years, 9 months ago
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
3 years, 10 months ago (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 ...
3 years, 10 months ago (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 ...
3 years, 10 months ago (2015-12-21 19:14:16 UTC) #3
saroyanm
3 years, 9 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5