337069 - inspecting chrome is undiscoverable

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Tuukka Tolvanen (sp3000)]

I imagine theme developers and users/devs looking for userchrome hacks or bugs are likely to be click-happy or well-connected and as such eventually able to discover the View -> Chrome toggle and that it adds/removes menuitems in another menu with no visible feedback, but seriously, wtf.

I don't think web developers looking for content documents (the target for bug 109481) will be too seriously injured by e.g. an always-present File -> Inspect Chrome submenu next to one for content documents.

Comment 1

[timeless]

seems reasonable to me, i don't suppose you intend to post a patch? domi doesn't have people with free time to do such things. i'll gladly review and help it get landed, but that's about all i can do.

Comment 2

[Jason Barnabe (np)]

Attachment: patch v1

I'm not sure whether this (cached JS object) is the best approach to having the Show Chrome menu item in that dynamic submenu, or whether wiping out everything except the Show Chrome menu item and then making sure to add the list of documents in the right place is better.

Comment 3

[Jason Barnabe (np)]

Rereading comment 0, this patch is actually slightly different that what was requested. Rather than having Inspect Chrome and Inspect Content submenus, it simply moves the toggle to within the Inspect Document submenu.

Comment 4

[Alex Vincent [:WeirdAl]]

Comment on attachment 238913 [details] [diff] [review]
patch v1

>   setTargetDocument: function(aDoc)
>   {
>-    this.mDocPanel.subject = aDoc;
>+    if (aDoc)
>+      this.mDocPanel.subject = aDoc;
>   },

Why this change?

Comment 5

[[e-mail]]

Comment on attachment 238913 [details] [diff] [review]
patch v1

If anything I'd go for a static menuitem.

Or we could just resurrect the Inspect Window submenu.

Comment 6

[Jason Barnabe (np)]

Attachment: patch v2

Takes out the pref and has separate menu items for chrome and content.

Comment 7

[[e-mail]]

Comment on attachment 239106 [details] [diff] [review]
patch v2

>+        this.appendContainedDocuments(docs, windowDocShell,
>+                                      aChrome ? nsIDocShellTreeItem.typeChrome :
>+                                               nsIDocShellTreeItem.typeContent);
Nit: indentation off by one space

>-        // We've failed with this window somehow, but we're catching the error so the
>-        // others will still work
>-        dump(ex + "\n");
>+        // We've failed with this window somehow, but we're catching the error
>+        // so the others will still work
>+        Components.utils.reportError(ex);
[Not convinced we need this change]

>+      noneMenuItem.setAttribute("label", this.mPanelSet.stringBundle
>+                               .getString("inspectWindow.noDocuments.message"));
Funky wrapping ;-)

>+        this.addInspectDocumentMenuItem(menu, docs[i], ++docNumber);
So, as we've only got one array, ++docNumber == i + 1

Comment 8

[Jason Barnabe (np)]

> >+        Components.utils.reportError(ex);
> [Not convinced we need this change]

It's much better to show an error on the Error Console than to only print it to the terminal where only people with dump enabled and currently running from the terminal will see it. When I wrote this code originally, I wasn't aware of reportError.

> Funky wrapping ;-)

What's the best way to wrap it?

Comment 9

[timeless]

Comment on attachment 239106 [details] [diff] [review]
patch v2

>+        this.appendContainedDocuments(docs, windowDocShell,
>+                                      aChrome ? nsIDocShellTreeItem.typeChrome :
>+                                               nsIDocShellTreeItem.typeContent);

personally i'd stick the : under the ?

>>+      noneMenuItem.setAttribute("label", this.mPanelSet.stringBundle
>>+                               .getString("inspectWindow.noDocuments.message"));
>Funky wrapping ;-)

something like:
      noneMenuItem.setAttribute("label",
                                this.mPanelSet.stringBundle
                                    .getString("inspectWindow.noDocuments.message"));

Comment 10

[Jason Barnabe (np)]

Attachment: patch v3

Updated to comments.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

mozilla/extensions/inspector/resources/content/commandOverlay.xul 1.9
mozilla/extensions/inspector/resources/content/inspector.js 1.30
mozilla/extensions/inspector/resources/content/popupOverlay.xul 1.16
mozilla/extensions/inspector/resources/content/prefs/inspector.js 1.13
<and all the dtds>