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.

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.

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.

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 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);

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 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?

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 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

(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.

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