Closed Bug 109481 Opened 23 years ago Closed 18 years ago

Easy way to pick open pages (as opposed to open windows)

Categories

(Other Applications :: DOM Inspector, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha

People

(Reporter: ian, Assigned: jason.barnabe)

References

()

Details

Attachments

(16 obsolete files)

I suspect the main use of DOM Inspector will be to inspect web pages, and open
ones at that. Therefore on the file menu there should be a way to select web
pages in any open tabs or windows.
Severity: normal → enhancement
Status: NEW → ASSIGNED
Target Milestone: --- → Future
The fix for this bug will probably resemble the fix for bug 105028.
Mass re-assigning bugs to dom.inspector@extensions.bugs
Assignee: hewitt → dom.inspector
Status: ASSIGNED → NEW
Product: Core → Other Applications
Attached patch Implement "Inspect a tab" v1 (obsolete) — Splinter Review
Just like "Inspect a Window", only for tabs.
Assignee: dom-inspector → jason_barnabe
Status: NEW → ASSIGNED
Attachment #182452 - Flags: review?(caillon)
Comment on attachment 182452 [details] [diff] [review]
Implement "Inspect a tab" v1

Well, how about that, someone finally decided to do something!	:D  I'll offer
a few comments, bearing in mind this is not a formal review, nor anything you
have to listen to.


>+    var tabbrowsers = windows.getNext().document
>+                                       .getElementsByTagName("tabbrowser");
>+    // For every <tabbrowser> in that window

Do we know that tabbrowser elements are, and forever will be, only used in a
navigator?  You might want to check the URL of the window you grab. 
(chrome://navigator/content/navigator.xul)

>+    for (var i = 0; i < tabbrowsers.length; i++) {
>+      // For every tab in that <tabbrowser>
>+      var tabs = tabbrowsers.item(i).browsers;
>+      for (var j = 0; j < tabs.length; j++) {
>+        // Create a menu item for the document in that tab
>+        var currentTab = tabs.item(j);

I'm not sure you have to use item.  tabs[i] would probably work just as well --
though I don't know the impact that has on the DOM.

>+        var menuItem = gInspectTabTemplate.cloneNode(true);

Why are you cloning an element (using true) that has no attributes and no child
nodes?	You could just create the element fresh, or at least change the true to
false here.

>+    <menu oncommand="inspector.goToTab(event.target);" onpopupshowing="showTabList()" onpopuphiding="hideTabList()"
>+      id="mnTabsFile" label="&mnTabs.label;" accesskey="&mnTabs.accesskey;">
>+      <menupopup id="listTabs-popup"/>
>+    </menu>

Still, this is a sweet thing to see.  :)
you should really use getElementsByTagNameNS and give it the XUL namespace
(In reply to comment #4)
> (From update of attachment 182452 [details] [diff] [review] [edit])
> Do we know that tabbrowser elements are, and forever will be, only used in a
> navigator?  You might want to check the URL of the window you grab. 
> (chrome://navigator/content/navigator.xul)

I (think I) made it so that any kind of tabbrowser will get its tabs picked up
by this, which is why I'm not assuming only one tabbrowser and I'm not just
doing a getElementById on Seamonkey/Firefox's tabbrowser element. Are you saying
we *should* limit it to tabbrowsers we know about? Why would we do that?

> I'm not sure you have to use item.  tabs[i] would probably work just as well --
> though I don't know the impact that has on the DOM.

I'll give it a shot.

> Why are you cloning an element (using true) that has no attributes and no child
> nodes?	You could just create the element fresh, or at least change the true to
> false here.

cloneNode is faster that createElement, as far as I know. As for why true rather
than false, it should probably be false (I thought I was going to have to add
some more stuff to it) but is cloneNode(true) necessarily slower than
cloneNode(false)?

(In reply to comment #5)
> you should really use getElementsByTagNameNS and give it the XUL namespace

Will do.
OS: Linux → All
Hardware: PC → All
(In reply to comment #6)
> I (think I) made it so that any kind of tabbrowser will get its tabs picked up
> by this, which is why I'm not assuming only one tabbrowser and I'm not just
> doing a getElementById on Seamonkey/Firefox's tabbrowser element. Are you saying
> we *should* limit it to tabbrowsers we know about? Why would we do that?

Not necessarily.  I just wanted to make sure you'd considered it.  :) I'm
perfectly happy being wrong.
Attached patch Implement "Inspect a tab" v2 (obsolete) — Splinter Review
Additional changes:

(From comments)
-collection.item(i) -> collection[i]
-gInspectTabTemplate.cloneNode(true); -> gInspectTabTemplate.cloneNode(false);
-getElementsByTagName -> getElementsByTagNameNS

(Other things I noticed)
-Accesskeys now start at 1, not 0, just like the Inspect a Window submenu
-Untitled tabs now show (Untitled), not nothing

Does anyone know of an XUL app that uses tabbrowser that I can test this on?
Attachment #182452 - Attachment is obsolete: true
Attachment #182556 - Flags: review?(caillon)
Attachment #182452 - Flags: review?(caillon)
Talking to caillon on IRC and we agreed this isn't the best solution. Here's
what the best solution is:

There would be an Inspect menu instead of Inspect a Tab or Inspect a Window.
This would list all the inspectable stuff, seperated into content (like this
patch implements) and chrome (like in Inspect a Window). An option will be given
to show/hide chrome stuff, defaulting to off. The content entries will be named
after their respective document's titles, the chrome will also be the titles
with the exception of browser windows, which will say something like "Mozilla
Firefox (Tab title 1, Tab title 2...). If chrome is disabled, "click to inspect"
then clicking on chrome will do nothing.

Furthermore, a context menu item will be added to tabs to inspect them. This'll
be handled in a different bug.
Attachment #182556 - Attachment is obsolete: true
Attachment #182556 - Flags: review?(caillon)
Attached patch Implement "Inspect document" v1 (obsolete) — Splinter Review
Attachment #187262 - Flags: review?(caillon)
Looks good to me!  I don't see any nits to pick here.
I wonder, should <iframe>s/<frame>s also show up in the list?
(In reply to comment #12)
> I wonder, should <iframe>s/<frame>s also show up in the list?

As a sub-menu item perhaps, but I'm fine with handling that in a separate bug. 
I will try and play with the latest patch in the next few days.  Thanks for the
updated work, Jason.
Flags: blocking1.9a1?
Comment on attachment 187262 [details] [diff] [review]
Implement "Inspect document" v1

1. please add some try blocks to showInspectDocumentList.
2. please consider grabbing all <browser>/<iframe>/<page>s instead of just the ones you expect. this won't let me inspect mailnews pages, nor will it let me inspect sidebars/webpanels nor will it let me inspect browser content in my product which is a navigator:browser but has no tabbrowser (except when i open a couple of random standard browsers, but most likely the lack of try blocks [first point] will result in no menu...)
3.

+  <!ENTITY mnInspectDocument.label "Inspect Document">
+  <!ENTITY mnInspectDocument.accesskey "d">

Should be "D" not "d".


+  <!ENTITY cmdToggleChrome.label "Chrome">
+  <!ENTITY cmdToggleChrome.accesskey "c">

should be "C"
Attachment #187262 - Flags: review?(caillon) → review-
I'd suggest enumerating the docShell tree via getDocShellEnumerator etc.
getDocShellEnumerator notices that the DOM Inspector window itself has a content document (View -> Browser). I think it'd be a bit confusing to users to have that document (which most of the time is about:blank) show up at all times, but then again it'd be a bit confusing for the list to be "all content documents except this one". Suggestions?
Attached patch Implement "Inspect document" v2 (obsolete) — Splinter Review
This patch uses getDocShellEnumerator to decide what is content and what is chrome. It picks up on <browser>, <tabbrowser>, <frame>, <iframe>, <page>, and perhaps more things that I don't know of. The titles are the documents' titles, or their URLs if they have none. I've added try blocks so that if any window or document fails, the others will still show up.

As for the browser in the DOM Inspector, it is hidden if it's not being used (is about:blank).

When compiling this, I was having build failures because I didn't define strings for locales other than en-US, so I had to edit the make file to not build the other locales. I don't know if this will be a problem when checked into CVS.
Attachment #187262 - Attachment is obsolete: true
Attachment #205268 - Flags: review?(timeless)
Attachment #205268 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #205268 - Flags: review?(timeless)
Attachment #205268 - Flags: review+
Attached patch Implement "Inspect document" v3 (obsolete) — Splinter Review
Per discussion with timeless, switch to javadoc style comments and fix an indentation problem.
Attachment #205268 - Attachment is obsolete: true
Attachment #206458 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #205268 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 206458 [details] [diff] [review]
Implement "Inspect document" v3

>Index: ./extensions/inspector/resources/content/commandOverlay.xul
Please avoid .s in your cvs diff, it confuses me when i try to use patch -p4

>+const nsIXULWindow         = Components.interfaces.nsIXULWindow;
Nit: only used once (compare nsIWindowMediator).

>+    this.mInspectDocumentTemplate = document.createElement("menuitem");
I don't see the point of this, just create new menuitems each time.

>+    this.mInspectDocumentMenu = document.getElementById("listDocuments-popup");
This could be passed in as a parameter to showInspectDocumentList (compare with the usage of emptyChildren).

>+    document.getElementById("cmdToggleChrome").setAttribute("checked",
>+                                               PrefUtils.getPref("inspector.showChrome"));
Nit: not important, but the other preferences aren't instant-global-apply i.e. changing them doesn't affect other existing inspectors.

>-    this.setTargetWindowById(aMenuitem.id);
IIRC this was the only consumer of setTargetWindowById

>+    if (showChrome) {
>+      var chromeDocs = [];
>+    }
Nit: abuse of JS scope rules. I know that the var has function-level scope but it just looks silly, and probably isn't worth the if in this case anyway.

>+    // For every window
Nit: don't go overboard with comments

>+        contentDocs = contentDocs.concat(this.getContainedDocuments(windowDocShell, 
>+                                         nsIDocShellTreeItem.typeContent));
It might be clearer to do something along the lines of
this.appendContainedDocuments(contentDocs, windowDocShell, .typeContent);

>+        if (showChrome) {
>+          chromeDocs = chromeDocs.concat(this.getContainedDocuments(windowDocShell, 
>+                                         nsIDocShellTreeItem.typeChrome));
>+        }
Conveniently the bracing in this file is inconsistent so you get to choose the version you prefer ;-)

>+    var docNumber = 1;
Nit: start at 0 and use ++docNumber.

>+        if (docShell.contentViewer.DOMDocument.location.href != window.location.href ||
Nit: use document.location.ref for consistency.

>+          var title = childDoc.title;
>+          // If there's no title, use the URL
>+          if (!title) {
>+            title = childDoc.location.href;
>+          }
I don't see the point of computing the title here, do it when adding the menuitem. Also, use childDoc.title || childDoc.location.href.

>+    <menu oncommand="inspector.setTargetDocument(event.target.doc);" onpopupshowing="inspector.showInspectDocumentList()"
>+      onpopuphiding="inspector.emptyChildren(this.firstChild);" id="mnInspectDocumentFile" label="&mnInspectDocument.label;"
>+      accesskey="&mnInspectDocument.accesskey;">
>+      <menupopup id="listDocuments-popup"/>
Ideally onpopupXXX handlers belong on the popup please.
Attachment #206458 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attached patch Implement "Inspect document" v4 (obsolete) — Splinter Review
(In reply to comment #19)
> >+    this.mInspectDocumentMenu = document.getElementById("listDocuments-popup");
> This could be passed in as a parameter to showInspectDocumentList (compare with
> the usage of emptyChildren).
I've made emptyChildren is a generic function because it might be useful in other parts of the code. It's unlikely any other code will want to reuse showInspectDocumentList.

> IIRC this was the only consumer of setTargetWindowById
And setTargetWindowById was the only consumer of kWindowDataSourceCID, inspectWindow.error.message, and inspectWindow.error.title. I've removed them all.

Everything else updated as requested. I also switched createElement to createElementNS.
Attachment #206458 - Attachment is obsolete: true
Attachment #206834 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 206834 [details] [diff] [review]
Implement "Inspect document" v4

Sorry for not spotting these before but style for XUL files is to line up subsequent lines of attributes under the first attribute as you can see in the subsequent menuitems.

>+    <menu oncommand="inspector.setTargetDocument(event.target.doc);" id="mnInspectDocumentFile" label="&mnInspectDocument.label;"
>+      accesskey="&mnInspectDocument.accesskey;">
>+      <menupopup id="listDocuments-popup" onpopupshowing="inspector.showInspectDocumentList()"
>+      onpopuphiding="inspector.emptyChildren(this);"/>
>     </menu>
>     <menuitem label="&cmdShowOpenURLDialog.label;" accesskey="&cmdShowOpenURLDialog.accesskey;"
>               observes="cmdShowOpenURLDialog"/>
>     <menuseparator/>
>     <menuitem label="&cmdClose.label;" accesskey="&cmdClose.accesskey;" 
>               observes="cmdClose" key="key_closeInspector"/>
Attachment #206834 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attached patch Implement "Inspect document" v5 (obsolete) — Splinter Review
Fix indentation.
Is this ready for checkin (has the patch changed significantly enough to mandate further review)?
mozilla/extensions/inspector/resources/content/commandOverlay.xul
new revision: 1.5; previous revision: 1.4
mozilla/extensions/inspector/resources/content/inspector.js
new revision: 1.25; previous revision: 1.24
mozilla/extensions/inspector/resources/content/popupOverlay.xul
new revision: 1.12; previous revision: 1.11
mozilla/extensions/inspector/resources/content/prefs/inspector.js
new revision: 1.9; previous revision: 1.8
mozilla/extensions/inspector/resources/locale/en-US/inspector.dtd
new revision: 1.6; previous revision: 1.5
mozilla/extensions/inspector/resources/locale/en-US/inspector.properties
new revision: 1.5; previous revision: 1.4
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Flags: blocking1.9a1?
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9alpha
I backed this out, all locales (not only en-US) need to be updated otherwise compare-locales fails.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(as Vidar mentioned on IRC, that involves adding the english strings to the other localizations and posting to n.p.m.l10n)
Attached patch Implement "Inspect document" v6 (obsolete) — Splinter Review
Includes English strings in all localizations. I've posted to n.p.m.l10m http://groups.google.com/group/netscape.public.mozilla.l10n/browse_thread/thread/842f67c335c08d55

(In reply to comment #17)
> When compiling this, I was having build failures because I didn't define
> strings for locales other than en-US, so I had to edit the make file to not
> build the other locales. I don't know if this will be a problem when checked
> into CVS.
I guess it was.
Attachment #206834 - Attachment is obsolete: true
Attachment #207152 - Attachment is obsolete: true
Back this out again, this is crap. By adding random strings to some random tree,
we're left without any traction on the localization quality at all.

Note that neither Vidar nor Gavin have a say here, decisions like this are done
by me.

I'll bother about finding a solution for this once I heard on which branches this 
fix is supposed to land.
(In reply to comment #28)
> Back this out again, this is crap. By adding random strings to some random
> tree,
> we're left without any traction on the localization quality at all.
> 
> Note that neither Vidar nor Gavin have a say here, decisions like this are done
> by me.

The latest patch was never landed, the previous one was backed out as soon as the l10n issue was noted.
Good. I still need to know on which branches you intend to land this, i.e. fx2 vs
fx3.
(In reply to comment #30)
> Good. I still need to know on which branches you intend to land this, i.e. fx2
> vs
> fx3.

Just on the trunk.
Axel, any word on how the localization should be handled for this bug?
http://groups.google.com/group/mozilla.dev.l10n/browse_thread/thread/50e5d03b7ea29dd7
Axel says to inform the localization owners to get them to attach the updated translations here. I'll give at least two weeks for the localizations to come in - after that, any locale that hasn't gotten updated will be removed from the DOM Inspector build.

So I'm CCing all of them, and here's the list of things to be updated:

inspector.dtd
 Remove
  mnWindows.label
  mnWindows.accesskey
 Add
  mnInspectDocument.label "Inspect Document"
  mnInspectDocument.accesskey "D"
  cmdToggleChrome.label "Chrome"
  cmdToggleChrome.accesskey "C"

inspector.properties
 Remove
  inspectWindow.error.message
  inspectWindow.error.title=
 Add
  inspectWindow.noDocuments.message = (None)
CCing a few more people...

The Czech localizer's e-mail address from http://wiki.mozilla.org/L10n:Localization_Teams wasn't accepted. Anyone know it?
fixed the wiki, it had markup trouble with '<' and email addresses.
Attached patch Implement "Inspect document" v7 (obsolete) — Splinter Review
Added Hungarian translation to patch called 'Implement "Inspect document" v6'.
Attachment #207223 - Attachment is obsolete: true
Please check the German L10n in with the English strings as it is in the patch, I'll do a checkin afterwards changing them to the correct German counterparts.
Polish translation of these strings.

We really need to come up with a better solution for mozilla/extensions l10n than this.
(In reply to comment #37)
> Please check the German L10n in with the English strings as it is in the patch,
> I'll do a checkin afterwards changing them to the correct German counterparts.
> 

Not without follow-up bug and commenting out the german locale from the build,
please.
That's funny: the Italian translation wasn't accepted and is not included but I was asked to make changes anyway... Nice!
The updated translation is in bug 311079, if it will ever matter to someone.
Any progress on resolving this bug?
Jason have said that grace period would be 2 weeks. It was a month ago.
It would be nice to resolve that bug for Firefox 2.0a1.
Blocks: 331189
Comment on attachment 212574 [details] [diff] [review]
Implement "Inspect document" v7

mozilla/extensions/inspector/resources/content/commandOverlay.xul 	1.8
mozilla/extensions/inspector/resources/content/inspector.js 	1.27
mozilla/extensions/inspector/resources/content/popupOverlay.xul 	1.14
mozilla/extensions/inspector/resources/content/prefs/inspector.js 	1.11

mozilla/extensions/inspector/resources/locale/ca/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/ca/inspector.properties 	1.2
mozilla/extensions/inspector/resources/locale/en-US/inspector.dtd 	1.8
mozilla/extensions/inspector/resources/locale/en-US/inspector.properties 	1.7
mozilla/extensions/inspector/resources/locale/hu/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/hu/inspector.properties 	1.2

mozilla/extensions/inspector/resources/locale/de/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/de/inspector.properties 	1.2
Attachment #212574 - Attachment is obsolete: true
Comment on attachment 212607 [details] [diff] [review]
Patch for Polish (pl) localization 

mozilla/extensions/inspector/resources/locale/pl/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/pl/inspector.properties 	1.2
Attachment #212607 - Attachment is obsolete: true
Comment on attachment 212608 [details] [diff] [review]
Patch for Russian (ru) locale of DOM Inspector

mozilla/extensions/inspector/resources/locale/ru/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/ru/inspector.properties 	1.2
Attachment #212608 - Attachment is obsolete: true
Comment on attachment 212776 [details] [diff] [review]
Patch for Swedish (sv-SE) localization

mozilla/extensions/inspector/resources/locale/sv-SE/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/sv-SE/inspector.properties 	1.3
Attachment #212776 - Attachment is obsolete: true
Comment on attachment 212861 [details] [diff] [review]
Patch for Greek (el) localization of inspector

mozilla/extensions/inspector/resources/locale/el/inspector.dtd 	1.2 	5/2 
mozilla/extensions/inspector/resources/locale/el/inspector.properties 	1.2
Attachment #212861 - Attachment is obsolete: true
Comment on attachment 212937 [details] [diff] [review]
Patch for Norwegian bokmål (nb-NO) localization

mozilla/extensions/inspector/resources/locale/nb-NO/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/nb-NO/inspector.properties 	1.2
Attachment #212937 - Attachment is obsolete: true
Comment on attachment 213175 [details] [diff] [review]
patch for Chinese Simplified (zh-CN) locale of DOM Inspector

mozilla/extensions/inspector/resources/locale/zh-CN/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/zh-CN/inspector.properties 	1.2
Attachment #213175 - Attachment is obsolete: true
Comment on attachment 213262 [details] [diff] [review]
Patch for Irish (ga-IE) localization of inspector

mozilla/extensions/inspector/resources/locale/ga-IE/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/ga-IE/inspector.properties 	1.2
Attachment #213262 - Attachment is obsolete: true
mozilla/extensions/inspector/resources/Makefile.in 	1.23

removed the locales that were unsupported

cs-CZ was in the newsgroups, it didn't recognize it until i went to file bugs, so i added it back (I hope I got it right):
mozilla/extensions/inspector/resources/Makefile.in 	1.24
mozilla/extensions/inspector/resources/locale/cs-CZ/inspector.dtd 	1.2
mozilla/extensions/inspector/resources/locale/cs-CZ/inspector.properties 	1.2

jason: please resolve this as fixed if it works to your satisfaction.

any problems or concerns (hereafter "bugs") about any issues relating this bug should be filed as new bugs (and may be placed in the blocks field of this bug next to the bugs already filed for the unfixed localizations).
Status: REOPENED → ASSIGNED
> cs-CZ was in the newsgroups, it didn't recognize it until i went to file bugs,
> so i added it back (I hope I got it right):

It looks right. Thanks a lot for digging the strings from NG.
It works to my satisfaction. Thanks for doing this for me, timeless; my development environment is kinda screwy lately so I haven't been able to do much.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
Depends on: 331828
Since this broke XUL window on mac, it would be great if someone, possibly the creator of the patch or any of the reviewers could provide some insight into what could be wrong.  

see bug 331828

Thanks!
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: