1379674 - Port Bug 1372520 [remove dependency between nsContextMenu and devtools] to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, enhancement)

Description

[Isaac Schemm]

DevTools is going to move to an addon in the future, so the devtools-shim was introduced. Bug 1372520 moved inspectNode (used for the item "Inspect Element" in the context menu of the browser), and it now needs to be called through the shim.

Comment 1

[Isaac Schemm]

Attachment: 1379674-inspectNode-fix.patch

This patch replaces gDevToolsBrowser.inspectNode with DevToolsShim.inspectNode in nsContextMenu. The latter takes a list of selectors (multiple are needed for elements inside frames), not a single element, so findCssSelector is used.

Comment 2

[Isaac Schemm]

Attachment: 1379674-inspectNode-fix-v2.patch

Re-insert gBrowser definition

Comment 3

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8884871 [details] [diff] [review]
1379674-inspectNode-fix-v2.patch

Works great but a few issues:

> Leave message empty to use default message. HG: -- HG: user: Isaac

NIT comment garbled.

> +XPCOMUtils.defineLazyModuleGetter(this, "DevToolsShim",
> +  "chrome://devtools-shim/content/DevToolsShim.jsm");

Second line usually should be aligned with the "this". But there are a few occasions in suite where it is done like here. First one would also end beyond 80 chars so lets just keep it. Personally for these cases I like it more.


> +       "gDevTools" in window &&
>         Services.prefs.getBoolPref("devtools.inspector.enabled");

Fx uses a default if the pref is not there:
 Services.prefs.getBoolPref("devtools.inspector.enabled", false);

I think we should add this too.

For the selectors array could you port the getNodeSelectors(node) function including the comment from mozilla/browser/base/content/content.js

WQondered where this code came from and it might chanmge again so keeping it a little closer to Fx might help in the future.

f+ for now.

Comment 4

[Isaac Schemm]

Attachment: 1379674-inspectNode-fix-v3.patch

Move getNodeSelectors into its own function; use default false for  devtools.inspector.enabled; fix commit message

Comment 5

[Ian Neal]

Comment on attachment 8886812 [details] [diff] [review]
1379674-inspectNode-fix-v3.patch

Probably need to mention you are also porting parts of:
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit

>+++ b/suite/common/nsContextMenu.js
>   inspectNode: function() {
>+    const selectors = this.getNodeSelectors(this.target);
>+
>     let gBrowser = this.browser.ownerDocument.defaultView.gBrowser;
>+    return DevToolsShim.inspectNode(gBrowser.selectedTab, selectors);

Maybe inline selectors so this becomes:
>+    return DevToolsShim.inspectNode(gBrowser.selectedTab,
>+                                    this.getNodeSelectors(this.target));

r=me with those addressed.

Comment 6

[Frank-Rainer Grahl (:frg)]

NITs fixed for checkin:
https://hg.mozilla.org/comm-central/rev/b201b01c6fc1bf5ea4f9b4b2525419de5c6971a7

Thanks Isaac