1372520 - [devtools-addon] remove dependency between nsContextMenu and devtools

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

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 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

As devtools become optional, nsContextMenu needs to check if devtools are installed before
displaying the devtools-related menu entries.

Rather than loading devtools classing and calling APIs, we also start using an event in
order to have less coupling between m-c and devtools code.

gContextMenu.inspectNode() used to return a promise which was only used in devtools tests
to wait for the inspectNode to have terminated. Instead we fire another event in devtools
browser to let tests know when the inspectNode() call is finished.

Review commit: https://reviewboard.mozilla.org/r/148426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148426/

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[Julian Descottes [: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.

Comment 4

[Julian Descottes [: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

[Shane Caraveo (:mixedpuppy)]

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 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8877130 [details]
Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148426/diff/1-2/

Comment 7

[Julian Descottes [:jdescottes]]

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

[Brian Grinstead [:bgrins]]

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 9

[Julian Descottes [:jdescottes]]

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 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8877130 [details]
Bug 1372520 - use DevToolsShim to inspectNode in nContextMenu;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148426/diff/2-3/

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Julian Descottes [:jdescottes]]

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

Comment 13

[Julian Descottes [:jdescottes]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff5b5c164713
use DevToolsShim to inspectNode in nContextMenu;r=bgrins,mixedpuppy

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff5b5c164713