Last Comment Bug 337069 - inspecting chrome is undiscoverable
: inspecting chrome is undiscoverable
Status: RESOLVED FIXED
:
Product: Other Applications
Classification: Client Software
Component: DOM Inspector (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha
Assigned To: Jason Barnabe (np)
:
:
Mentors:
Depends on:
Blocks: 332760 335775
  Show dependency treegraph
 
Reported: 2006-05-07 17:06 PDT by Tuukka Tolvanen (sp3000)
Modified: 2007-07-06 01:06 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (22.14 KB, patch)
2006-09-17 11:53 PDT, Jason Barnabe (np)
neil: superreview-
Details | Diff | Splinter Review
patch v2 (40.94 KB, patch)
2006-09-18 19:45 PDT, Jason Barnabe (np)
timeless: review+
neil: superreview+
Details | Diff | Splinter Review
patch v3 (40.94 KB, patch)
2006-09-24 17:50 PDT, Jason Barnabe (np)
jason.barnabe: review+
jason.barnabe: superreview+
Details | Diff | Splinter Review

Description Tuukka Tolvanen (sp3000) 2006-05-07 17:06:52 PDT
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 2006-05-08 00:16:23 PDT
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) 2006-09-17 11:53:00 PDT
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.
Comment 3 Jason Barnabe (np) 2006-09-17 11:54:58 PDT
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] 2006-09-17 16:49:14 PDT
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 neil@parkwaycc.co.uk 2006-09-18 03:50:40 PDT
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) 2006-09-18 19:45:32 PDT
Created attachment 239106 [details] [diff] [review]
patch v2

Takes out the pref and has separate menu items for chrome and content.
Comment 7 neil@parkwaycc.co.uk 2006-09-19 02:07:20 PDT
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) 2006-09-19 07:08:43 PDT
> >+        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 2006-09-24 11:35:05 PDT
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) 2006-09-24 17:50:56 PDT
Created attachment 239966 [details] [diff] [review]
patch v3

Updated to comments.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-10-10 06:23:38 PDT
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>

Note You need to log in before you can comment on or make changes to this bug.