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

Issue 29573905: Issue 4580 - Replace ext.devtools with devtools

Created:
Oct. 11, 2017, 10:38 p.m. by Manish Jethani
Modified:
Oct. 18, 2017, 1:41 a.m.
Base URL:
https://hg.adblockplus.org/adblockplusui/
Visibility:
Public.

Description

Issue 4580 - Replace ext.devtools with devtools

Patch Set 1 #

Total comments: 3

Patch Set 2 : Remove devtools.onCreated #

Total comments: 3

Patch Set 3 : Fix ESLint errors #

Total comments: 2

Patch Set 4 : Use runtime.onConnect #

Total comments: 9

Patch Set 5 : Use Location.pathname #

Total comments: 3

Patch Set 6 : Rebase #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -146 lines) Patch
M background.js View 1 2 3 4 5 2 chunks +10 lines, -6 lines 0 comments Download
M desktop-options.js View 1 2 1 chunk +1 line, -1 line 0 comments Download
M devtools-panel.html View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M devtools-panel.js View 1 2 3 4 5 5 chunks +27 lines, -24 lines 0 comments Download
M ext/background.js View 1 2 3 4 5 2 chunks +2 lines, -17 lines 0 comments Download
M ext/content.js View 1 2 3 4 5 1 chunk +0 lines, -61 lines 0 comments Download
M ext/devtools.js View 1 2 3 1 chunk +0 lines, -33 lines 0 comments Download
M polyfill.js View 1 2 3 4 5 2 chunks +100 lines, -3 lines 3 comments Download

Messages

Total messages: 9
Manish Jethani
Oct. 11, 2017, 10:38 p.m. (2017-10-11 22:38:38 UTC) #1
Manish Jethani
Patch Set 1 I'm trying to get rid of ext/devtools.js but it's going to take ...
Oct. 11, 2017, 10:46 p.m. (2017-10-11 22:46:00 UTC) #2
Manish Jethani
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#newcode546 background.js:546: port.on("devtools.ready", (message, sender) => ...
Oct. 11, 2017, 11:37 p.m. (2017-10-11 23:37:34 UTC) #3
Manish Jethani
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#oldcode1216 desktop-options.js:1216: let {url, recommended} ...
Oct. 11, 2017, 11:48 p.m. (2017-10-11 23:48:34 UTC) #4
Manish Jethani
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#newcode1 ext/devtools.js:1: /* This file is ...
Oct. 12, 2017, 1:30 a.m. (2017-10-12 01:30:03 UTC) #5
Sebastian Noack
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#newcode191 ...
Oct. 17, 2017, 9:40 p.m. (2017-10-17 21:40:26 UTC) #6
Manish Jethani
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#newcode191 ext/common.js:191: if (/\/devtools-panel\.html\b/.test(top.location.href)) On 2017/10/17 ...
Oct. 18, 2017, 12:26 a.m. (2017-10-18 00:26:54 UTC) #7
Manish Jethani
On 2017/10/17 21:40:26, Sebastian Noack wrote: > Is there a patch for the respective changes ...
Oct. 18, 2017, 12:28 a.m. (2017-10-18 00:28:13 UTC) #8
Manish Jethani
Oct. 18, 2017, 1:41 a.m. (2017-10-18 01:41:31 UTC) #9
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.

Powered by Google App Engine
This is Rietveld