Closed
Bug 1367425
Opened 7 years ago
Closed 7 years ago
[devtools-addon] Use DevToolsShim in web extensions
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
Details
Attachments
(1 file)
Now that Bug 1356244, we should use the DevToolsShim to remove dependencies from the web extensions code on DevTools code.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Web extensions is relying on the following gDevTools methods: - showToolbox - closeToolbox - getTargetForTab - emit - getTheme getTheme is used from the actual web extensions code, while the other ones are only used in tests. I will review this once again after migrating the addon sdk code to check if some of those methods should actually be ported to the Shim directly.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8871719 [details] Bug 1367425 - Use DevToolsShim in webextensions codebase; https://reviewboard.mozilla.org/r/143208/#review150304 ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:94 (Diff revision 3) > > let backgroundPageCurrentTabId = await extension.awaitMessage("current-tab-id"); > > - let target = devtools.TargetFactory.forTab(tab); > + let target = DevToolsShim.gDevTools.getTargetForTab(tab); > > - await gDevTools.showToolbox(target, "webconsole"); > + await DevToolsShim.gDevTools.showToolbox(target, "webconsole"); Nit: in the tests we could retrieve the gDevTools instance at the top of the test (using something like `const {gDevTools} = DevToolsShim;`) and leave most of the rest of the test unmodified ::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:23 (Diff revision 3) > - const waitforThemeChanged = new Promise(resolve => gDevTools.once("theme-changed", resolve)); > + const waitforThemeChanged = new Promise(resolve => { > + DevToolsShim.on("theme-changed", function onThemeChanged() { > + DevToolsShim.off("theme-changed", onThemeChanged); > + resolve(); > + }); > + }); Can we provide `DevToolsShim.once` instead on workarounding its lack specifically for the "theme-changed" test?
Attachment #8871719 -
Flags: review?(lgreco)
Comment 6•7 years ago
|
||
Hi :jdescottes, I just took a look at this patch and added a couple of feedback comments on it. Besides the above two small nits, the change looks good to me. Once we have decided if we need to apply any further change to the patch based on the above comments or not, I'm going to add a peer from the Addons Team for a final review and sign-off on the patch.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8871719 [details] Bug 1367425 - Use DevToolsShim in webextensions codebase; https://reviewboard.mozilla.org/r/143208/#review150304 Thanks for the review! > Nit: in the tests we could retrieve the gDevTools instance at the top of the test (using something like `const {gDevTools} = DevToolsShim;`) and leave most of the rest of the test unmodified Thanks! updated > Can we provide `DevToolsShim.once` instead on workarounding its lack specifically for the "theme-changed" test? I would like to avoid implementing and maintaining once() for the shim, since it is only used by test code. I updated this to use DevToolsShim.gDevTools.once instead.
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8871719 [details] Bug 1367425 - Use DevToolsShim in webextensions codebase; https://reviewboard.mozilla.org/r/143208/#review150304 > I would like to avoid implementing and maintaining once() for the shim, since it is only used by test code. > I updated this to use DevToolsShim.gDevTools.once instead. sounds good to me.
Comment 11•7 years ago
|
||
Comment on attachment 8871719 [details] Bug 1367425 - Use DevToolsShim in webextensions codebase; Thanks Julian, it looks good to me. Hi Shane, can you take a look to this patch for a final review and sign-off on it?
Attachment #8871719 -
Flags: review?(mixedpuppy)
Attachment #8871719 -
Flags: review?(lgreco)
Attachment #8871719 -
Flags: feedback+
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8871719 [details] Bug 1367425 - Use DevToolsShim in webextensions codebase; https://reviewboard.mozilla.org/r/143208/#review150844
Attachment #8871719 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Comment 13•7 years ago
|
||
let's wait for the add-on sdk bug to land first. In the meantime try on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=987ee3c2b7ac985a8e78d8877a3cb95cec9f3675
Depends on: 1367424
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8871719 [details] Bug 1367425 - Use DevToolsShim in webextensions codebase; https://reviewboard.mozilla.org/r/143208/#review152920
Attachment #8871719 -
Flags: review+
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e6783f75565e Use DevToolsShim in webextensions codebase;r=mixedpuppy
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e6783f75565e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 19•7 years ago
|
||
Comment on attachment 8871719 [details] Bug 1367425 - Use DevToolsShim in webextensions codebase; (cleared the r? flag, the patch was already reviewed and it has also been landed).
Attachment #8871719 -
Flags: review?(lgreco) → review+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•