Issue 5386 - Do not include content.js in devtools panel

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.

[Manish Jethani, 2017-07-21 20:44:06]

Patch Set 1

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.

Of course there's still the Firefox bug and I'll report it to Mozilla, but it
should not affect us.

[Thomas Greiner, 2017-07-24 12:06:16]

On 2017/07/21 20:44:06, Manish Jethani wrote:
> 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.

Maybe I'm misunderstanding what you mean but the DevTools panel does receive
messages from the background page. So here's a bit of background info in the
hope that we can clear things up:

The naming is a bit misleading but content.js is not related to our content
scripts but enables our UI to communicate with the background page. For
instance, the DevTools panel uses `ext.backgroundPage.sendMessage()` and also
provides `ext.onMessage` in our test environment. Removing content.js will
therefore break the filter action buttons as well as the test environment.

Note that our UIs share the same process as our background page so, strictly
speaking, we may not even need that layer anymore. Although checking out our
DevTools panel, the situation appears to be different there.

[Manish Jethani, 2017-07-24 15:28:07]

On 2017/07/24 12:06:16, Thomas Greiner wrote:
> The naming is a bit misleading but content.js is not related to our content
> scripts but enables our UI to communicate with the background page. For
> instance, the DevTools panel uses `ext.backgroundPage.sendMessage()` and also
> provides `ext.onMessage` in our test environment. Removing content.js will
> therefore break the filter action buttons as well as the test environment.

See this code review for background: 

https://codereview.adblockplus.org/29482663/

ext.onMessage is set to port.onMessage

We're listening on runtime.onMessage here, which will never get any messages
from the background page, since the only way for the background page to send
messages directly is via the port. We're already listening on the port
elsewhere.

If the devtools panel uses sendMessage, it'll get the response right there.

See also this Mozilla bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1383310

[Thomas Greiner, 2017-07-24 17:31:57]

On 2017/07/24 15:28:07, Manish Jethani wrote:
> ext.onMessage is set to port.onMessage
> 
> We're listening on runtime.onMessage here, which will never get any messages
> from the background page, since the only way for the background page to send
> messages directly is via the port. We're already listening on the port
> elsewhere.

Ok, I wasn't aware of that implementation detail so thanks for mentioning it. In
that case this change only breaks the development environment because there,
unlike in the extension, the DevTools panel relies on content.js to inject the
background page frame and to provide `ext.backgroundPage.sendMessage()`.
You can find instructions on how to test the page in the development environment
in the repo's README.

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 across
all our UIs.

Let me know how you'd prefer to progress with this issue.

[Manish Jethani, 2017-07-25 12:14:29]

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
across
> 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.