Closed Bug 1367425 Opened 7 years ago Closed 7 years ago

[devtools-addon] Use DevToolsShim in web extensions

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(1 file)

Bug 1367425 - Use DevToolsShim in webextensions codebase;
59 bytes, text/x-review-board-request
rpl
: review+
Details 
Now that Bug 1356244, we should use the DevToolsShim to remove dependencies from the web extensions code on DevTools code.
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 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)
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 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 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 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 on attachment 8871719 [details]
Bug 1367425 - Use DevToolsShim in webextensions codebase;

https://reviewboard.mozilla.org/r/143208/#review150844
Attachment #8871719 - Flags: review?(mixedpuppy) → review+
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 on attachment 8871719 [details]
Bug 1367425 - Use DevToolsShim in webextensions codebase;

https://reviewboard.mozilla.org/r/143208/#review152920
Attachment #8871719 - Flags: review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6783f75565e
Use DevToolsShim in webextensions codebase;r=mixedpuppy
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 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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: