The default bug view has changed. See this FAQ.

inspecting chrome is undiscoverable

RESOLVED FIXED in mozilla1.9alpha

Status

Other Applications
DOM Inspector
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Tuukka Tolvanen (sp3000), Assigned: Jason Barnabe (np))

Tracking

Trunk
mozilla1.9alpha
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

40.94 KB, patch
Jason Barnabe (np)
: review+
Jason Barnabe (np)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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

11 years ago
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.
(Assignee)

Comment 2

11 years ago
Created attachment 238913 [details] [diff] [review]
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.
Assignee: dom-inspector → jason_barnabe
Status: NEW → ASSIGNED
Attachment #238913 - Flags: superreview?(neil)
Attachment #238913 - Flags: review?(timeless)
(Assignee)

Comment 3

11 years ago
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 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

11 years ago
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.
Attachment #238913 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 6

11 years ago
Created attachment 239106 [details] [diff] [review]
patch v2

Takes out the pref and has separate menu items for chrome and content.
Attachment #238913 - Attachment is obsolete: true
Attachment #239106 - Flags: superreview?(neil)
Attachment #239106 - Flags: review?(timeless)
Attachment #238913 - Flags: review?(timeless)

Comment 7

11 years ago
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
Attachment #239106 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 8

11 years ago
> >+        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

11 years ago
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"));
Attachment #239106 - Flags: review?(timeless) → review+
(Assignee)

Comment 10

11 years ago
Created attachment 239966 [details] [diff] [review]
patch v3

Updated to comments.
Attachment #239106 - Attachment is obsolete: true
Attachment #239966 - Flags: superreview+
Attachment #239966 - Flags: review+
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed]
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>
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.