[devtools-addon] remove dependency between nsContextMenu and devtools

RESOLVED FIXED in Firefox 56

Status

P3
enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

nsContextMenu depends on devtools code to trigger the inspect node command. As DevTools are moving to an addon and will no longer be guaranteed to be available at runtime, this dependency should be removed.

In a next step DevTools code will also live in a different repository, so more than just lazy loading the DevTools files, we should also rely on events rather that explicitly loading code paths that might change in the future.
Comment hidden (mozreview-request)
What's the planned UX for installing devtools when it isn't currently installed? Just want to ensure it doesn't involve the context menu item as an entry point.
Flags: needinfo?(jdescottes)
I'm not 100% sure, I need to clarify the UX. I feel like we forgot about this entry point recently :/ 
I'm OK with blocking this until I have more info. 

In the first prototype Alex did (Bug 1361080), the UI shim listens to a "browser-inspect-node" event (and does nothing at the moment but it should probably trigger an install of devtools?). So in this scenario "Inspect Node" should still be displayed if devtools are not installed (but that was before we started working on UX).

I'm blocking on Bug 1361080 until I can clarify.
Depends on: 1361080
Flags: needinfo?(jdescottes)
After checking, the menu entry should trigger the installation process for DevTools (by opening a dedicated about: page).
Until the UI shim is ready, I would still keep the implementation as it is here though?

Comment 5

2 years ago
mozreview-review
Comment on attachment 8877130 [details]
Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

https://reviewboard.mozilla.org/r/148426/#review153022

r+ if we're certain multiple observers are fine here.

::: browser/base/content/nsContextMenu.js:641
(Diff revision 1)
> -    let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> -    let { gDevToolsBrowser } = require("devtools/client/framework/devtools-browser");
> -    return gDevToolsBrowser.inspectNode(gBrowser.selectedTab, this.targetSelectors);
> +      wrappedJSObject: {
> +        tab: gBrowser.selectedTab,
> +        selectors: this.targetSelectors,
> +      }
> +    };
> +    Services.obs.notifyObservers(subject, "on-contextmenu-inspect-node");

Are you certain there is only one listener, if not that it works when there is more than one?  I'm not crazy about this, I'd rather see DevToolsShim.inspectNode(tab, selectors);
Attachment #8877130 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8877130 [details]
Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

https://reviewboard.mozilla.org/r/148426/#review153022

Thanks for the review! I updated to use your suggestion of having inspectNode on the DevToolsShim directly, I don't have a strong preference here.

Comment 8

2 years ago
mozreview-review
Comment on attachment 8877130 [details]
Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

https://reviewboard.mozilla.org/r/148426/#review155914

::: devtools/shim/DevToolsShim.jsm:197
(Diff revision 2)
> +   */
> +  inspectNode: function (tab, selectors) {
> +    if (!this.isInstalled()) {
> +      return Promise.resolve();
> +    }
> +    return this.gDevTools.inspectNode(tab, selectors);

I believe the context menu is the only consumer of the gDevToolsBrowser.inspectNode function, which means this is now the only consumer.  That function doesn't seem to depend on anything in devtools-browser, so perhaps we should move the implementation here rather than passing the call along from gDevTools and onto gDevToolsBrowser.  Or if you prefer to keep the shim small, then move the implementation into gDevTools.

Can you think of any reason not to do this?
Attachment #8877130 - Flags: review?(bgrinstead)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8877130 [details]
Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

https://reviewboard.mozilla.org/r/148426/#review155914

> I believe the context menu is the only consumer of the gDevToolsBrowser.inspectNode function, which means this is now the only consumer.  That function doesn't seem to depend on anything in devtools-browser, so perhaps we should move the implementation here rather than passing the call along from gDevTools and onto gDevToolsBrowser.  Or if you prefer to keep the shim small, then move the implementation into gDevTools.
> 
> Can you think of any reason not to do this?

Thanks for the feedback! 

I'm okay with moving the implementation from devtools-browser to devtools. The goal of the devtools-shim is to leak as little implementation details as possible from devtools so I won't move the implementation to the shim though.
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8877130 [details]
Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

https://reviewboard.mozilla.org/r/148426/#review155938

This works for me as long as try is green and the decision has been made to remove the context menu item rather than leaving it in as an entry point for installing the extension
Attachment #8877130 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8877130 [details]
> Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;
> 
> https://reviewboard.mozilla.org/r/148426/#review155938
> 
> This works for me as long as try is green and the decision has been made to
> remove the context menu item rather than leaving it in as an entry point for
> installing the extension

try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd0ff2768166c5d2f03b3d49e82249f93ee29e0c 

Ping me when you are available tomorrow. The context menu entry will come back as an entry point for the devtools installation. I don't see why it should prevent landing this as it is, which is why I am proposing this patch. But let's clarify this before landing.
Try is green. Discussed with :bgrins on slack about the context menu entry. It will most likely come back when the UI shim is reintroduced, but no reason to prevent this patch from landing.

Comment 14

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff5b5c164713
use DevToolsShim to inspectNode in nContextMenu;r=bgrins,mixedpuppy

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff5b5c164713
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

a year ago
Blocks: 1379674

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.