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

Issue 29494581: Issue 5386 - Do not include content.js in devtools panel (Closed)

July 21, 2017, 8:29 p.m. by Manish Jethani
July 25, 2017, 12:15 p.m.
Sebastian Noack
Base URL:


Issue 5386 - Do not include content.js in devtools panel This was working so far because the devtools panel simply does not receive any messages from either the content script or the background page via chrome.runtime.onMessage. But there's a bug in Firefox 55 now that's causing messages to come through. It turns out content.js is being included unnecessarily in the devtools panel. If we simply remove it, that fixes the issue.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+0 lines, -1 line) Patch
M devtools-panel.html View 1 chunk +0 lines, -1 line 0 comments Download


Total messages: 6
Manish Jethani
July 21, 2017, 8:29 p.m. (2017-07-21 20:29:57 UTC) #1
Manish Jethani
Patch Set 1 This was working so far because the devtools panel simply does not ...
July 21, 2017, 8:44 p.m. (2017-07-21 20:44:06 UTC) #2
Thomas Greiner
On 2017/07/21 20:44:06, Manish Jethani wrote: > This was working so far because the devtools ...
July 24, 2017, 12:06 p.m. (2017-07-24 12:06:16 UTC) #3
Manish Jethani
On 2017/07/24 12:06:16, Thomas Greiner wrote: > The naming is a bit misleading but content.js ...
July 24, 2017, 3:28 p.m. (2017-07-24 15:28:07 UTC) #4
Thomas Greiner
On 2017/07/24 15:28:07, Manish Jethani wrote: > ext.onMessage is set to port.onMessage > > We're ...
July 24, 2017, 5:31 p.m. (2017-07-24 17:31:57 UTC) #5
Manish Jethani
July 25, 2017, 12:14 p.m. (2017-07-25 12:14:29 UTC) #6
On 2017/07/24 17:31:57, Thomas Greiner wrote:

> Therefore I'd recommend to update the mock implementation at
> adblockplusui/ext/{content,devtools}.js to reflect which APIs are exposed in
> adblockpluschrome/ext/{content,devtools}.js. Additionally, it seems that we'd
> need to duplicate the background page frame injection code in devtools.js or
> introduce yet another script file for it.
> That being said, the underlying issue appears to be that we're using two
> redundant ways for passing messages between our background page and our UIs
> rather than one generic implementation in ext/content.js that can be used
> all our UIs.

I thought about this a little. It looks like ext/content.js is included in more
than one page and has a well-defined purpose. I'd like to address this
particular issue with minimal changes, especially since it's due to a bug in
Firefox. I'm going to go back to my original fix, i.e. 29482663.

The way ext.onMessage is being made to point to port.onMessage in the
extension's ext/devtools.js looks like a bit of a hack, but that needs to be
addressed separately.

Powered by Google App Engine
This is Rietveld