Issue 4580 - Replace ext.devtools with devtools

Issue 4580 - Replace ext.devtools with devtools

[Manish Jethani, 2017-10-11 22:46:00]

Patch Set 1

I'm trying to get rid of ext/devtools.js but it's going to take a few steps. The
first is to replace ext.devtools with the standard API.

https://codereview.adblockplus.org/29573905/diff/29573906/devtools-panel.js
File devtools-panel.js (left):

https://codereview.adblockplus.org/29573905/diff/29573906/devtools-panel.js#o...
devtools-panel.js:207: }, false);
Bonus.

https://codereview.adblockplus.org/29573905/diff/29573906/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29573905/diff/29573906/ext/common.js#newco...
ext/common.js:164: /* Polyfills */
The code in this section will eventually move to polyfill.js

https://codereview.adblockplus.org/29573905/diff/29573906/ext/common.js#newco...
ext/common.js:166: if (/\/devtools-panel\.html\b/.test(top.location.href))
chrome.devtools is not available outside the devtools panel

We have to make it available in background.js here though, I'll re-evaluate that
and maybe we do it differently.

[Manish Jethani, 2017-10-11 23:37:34]

Patch Set 2: Remove devtools.onCreated

https://codereview.adblockplus.org/29573905/diff/29573915/background.js
File background.js (right):

https://codereview.adblockplus.org/29573905/diff/29573915/background.js#newco...
background.js:546: port.on("devtools.ready", (message, sender) =>
There's no API called devtools.onCreated. This is the proper way to do it: the
devtools panel sends a "devtools.ready" message.

https://codereview.adblockplus.org/29573905/diff/29573915/ext/content.js
File ext/content.js (left):

https://codereview.adblockplus.org/29573905/diff/29573915/ext/content.js#oldc...
ext/content.js:52: ext.backgroundPage = {
ext.backgroundPage is no longer necessary.

https://codereview.adblockplus.org/29573905/diff/29573915/ext/devtools.js
File ext/devtools.js (right):

https://codereview.adblockplus.org/29573905/diff/29573915/ext/devtools.js#new...
ext/devtools.js:23: chrome.runtime.sendMessage({type: "devtools.ready"});
Using standard API

[Manish Jethani, 2017-10-11 23:48:34]

Patch Set 3: Fix ESLint errors

https://codereview.adblockplus.org/29573905/diff/29573930/desktop-options.js
File desktop-options.js (left):

https://codereview.adblockplus.org/29573905/diff/29573930/desktop-options.js#...
desktop-options.js:1216: let {url, recommended} = subscription;
Unrelated, but this was unused.

[Manish Jethani, 2017-10-12 01:30:03]

Patch Set 4: Use runtime.onConnect

https://codereview.adblockplus.org/29573905/diff/29573930/ext/devtools.js
File ext/devtools.js (right):

https://codereview.adblockplus.org/29573905/diff/29573930/ext/devtools.js#new...
ext/devtools.js:1: /*
This file is deleted.

https://codereview.adblockplus.org/29573905/diff/29573938/background.js
File background.js (right):

https://codereview.adblockplus.org/29573905/diff/29573938/background.js#newco...
background.js:419: modules.requestBlocker = {
Unrelated, but requestBlocker.filterTypes needs to be defined, otherwise it's an
error in messageResponder.js

https://codereview.adblockplus.org/29573905/diff/29573938/background.js#newco...
background.js:546: chrome.runtime.onConnect.addListener(port =>
Use runtime.onConnect, what the real background page does.

https://codereview.adblockplus.org/29573905/diff/29573938/devtools-panel.html
File devtools-panel.html (left):

https://codereview.adblockplus.org/29573905/diff/29573938/devtools-panel.html...
devtools-panel.html:25: <script src="ext/devtools.js"></script>
ext/devtools.js is no longer required.

https://codereview.adblockplus.org/29573905/diff/29573938/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29573905/diff/29573938/devtools-panel.js#n...
devtools-panel.js:23: {
Just indentation.

https://codereview.adblockplus.org/29573905/diff/29573938/devtools-panel.js#n...
devtools-panel.js:208: let port = chrome.runtime.connect({
This was previously done in ext/devtools.js in adblockpluschrome

https://codereview.adblockplus.org/29573905/diff/29573938/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29573905/diff/29573938/ext/common.js#newco...
ext/common.js:176: callback({
Call the onConnect handler with a port object [1]

[1]: https://developer.chrome.com/apps/runtime#type-Port

https://codereview.adblockplus.org/29573905/diff/29573938/ext/content.js
File ext/content.js (right):

https://codereview.adblockplus.org/29573905/diff/29573938/ext/content.js#newc...
ext/content.js:67: sendRawMessage({type: "connect"});
Send a "connect" message and return a port object for listening

[Sebastian Noack, 2017-10-17 21:40:26]

Is there a patch for the respective changes in adblockpluschrome?

https://codereview.adblockplus.org/29573905/diff/29573938/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29573905/diff/29573938/ext/common.js#newco...
ext/common.js:191: if (/\/devtools-panel\.html\b/.test(top.location.href))
I suppose we could just check for:

  top.location.pathname == 'devtools-panel.html'

[Manish Jethani, 2017-10-18 00:26:54]

Patch Set 5: Use Location.pathname

https://codereview.adblockplus.org/29573905/diff/29573938/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29573905/diff/29573938/ext/common.js#newco...
ext/common.js:191: if (/\/devtools-panel\.html\b/.test(top.location.href))
On 2017/10/17 21:40:25, Sebastian Noack wrote:
> I suppose we could just check for:
> 
>   top.location.pathname == 'devtools-panel.html'

Done.

https://codereview.adblockplus.org/29573905/diff/29581919/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29573905/diff/29581919/ext/common.js#newco...
ext/common.js:164: /* Polyfills */
So now a lot of this code will move into polyfill.js

Let me rebase this.

[Manish Jethani, 2017-10-18 00:28:13]

On 2017/10/17 21:40:26, Sebastian Noack wrote:
> Is there a patch for the respective changes in adblockpluschrome?

The only change in adblockpluschrome for this is to delete the file
ext/devtools.js

[Manish Jethani, 2017-10-18 01:41:31]

Patch Set 6: Rebase

https://codereview.adblockplus.org/29573905/diff/29581919/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29573905/diff/29581919/ext/background.js#n...
ext/background.js:27: parent.postMessage({
Just reformatting.

https://codereview.adblockplus.org/29573905/diff/29581919/ext/content.js
File ext/content.js (right):

https://codereview.adblockplus.org/29573905/diff/29581919/ext/content.js#newc...
ext/content.js:33: 
All of this code has moved to polyfill.js

https://codereview.adblockplus.org/29573905/diff/29581928/polyfill.js
File polyfill.js (left):

https://codereview.adblockplus.org/29573905/diff/29581928/polyfill.js#oldcode20
polyfill.js:20: (function()
This is not required.

https://codereview.adblockplus.org/29573905/diff/29581928/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573905/diff/29581928/polyfill.js#newcode27
polyfill.js:27: let backgroundWindow = null;
We have to maintain a reference to the background window here, because the
background frame is injected in ext/content.js and we only get an event here.

https://codereview.adblockplus.org/29573905/diff/29581928/polyfill.js#newcode33
polyfill.js:33: window.removeEventListener("message",
backgroundPageLoadedHandler);
Removing the listener first, I hope it's OK.