Closed Bug 337069 Opened 14 years ago Closed 14 years ago

inspecting chrome is undiscoverable

Categories

(Other Applications :: DOM Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha

People

(Reporter: tuukka.tolvanen, Assigned: jason.barnabe)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Attached patch patch v1 (obsolete) — Splinter Review
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)
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 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-
Attached patch patch v2 (obsolete) — Splinter Review
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 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+
> >+        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 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+
Attached patch patch v3Splinter Review
Updated to comments.
Attachment #239106 - Attachment is obsolete: true
Attachment #239966 - Flags: superreview+
Attachment #239966 - Flags: review+
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
Closed: 14 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.