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)
Other Applications
DOM Inspector
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.
Updated•23 years ago
|
Severity: normal → enhancement
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Comment 1•21 years ago
|
||
The fix for this bug will probably resemble the fix for bug 105028.
Comment 2•21 years ago
|
||
Mass re-assigning bugs to dom.inspector@extensions.bugs
Assignee: hewitt → dom.inspector
Status: ASSIGNED → NEW
Updated•20 years ago
|
Product: Core → Other Applications
Assignee | ||
Comment 3•19 years ago
|
||
Just like "Inspect a Window", only for tabs.
Assignee: dom-inspector → jason_barnabe
Status: NEW → ASSIGNED
Attachment #182452 -
Flags: review?(caillon)
Comment 4•19 years ago
|
||
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. :)
Comment 5•19 years ago
|
||
you should really use getElementsByTagNameNS and give it the XUL namespace
Assignee | ||
Comment 6•19 years ago
|
||
(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
Comment 7•19 years ago
|
||
(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.
Assignee | ||
Comment 8•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #182452 -
Flags: review?(caillon)
Assignee | ||
Comment 9•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #182556 -
Attachment is obsolete: true
Attachment #182556 -
Flags: review?(caillon)
Assignee | ||
Comment 10•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Attachment #187262 -
Flags: review?(caillon)
Comment 11•19 years ago
|
||
Looks good to me! I don't see any nits to pick here.
Comment 12•19 years ago
|
||
I wonder, should <iframe>s/<frame>s also show up in the list?
Comment 13•19 years ago
|
||
(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.
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.9a1?
Comment 14•19 years ago
|
||
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-
Comment 15•19 years ago
|
||
I'd suggest enumerating the docShell tree via getDocShellEnumerator etc.
Assignee | ||
Comment 16•19 years ago
|
||
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?
Assignee | ||
Comment 17•19 years ago
|
||
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+
Assignee | ||
Comment 18•19 years ago
|
||
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 19•19 years ago
|
||
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-
Assignee | ||
Comment 20•19 years ago
|
||
(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 21•19 years ago
|
||
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+
Assignee | ||
Comment 22•19 years ago
|
||
Fix indentation.
Comment 23•19 years ago
|
||
Is this ready for checkin (has the patch changed significantly enough to mandate further review)?
Comment 24•19 years ago
|
||
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
Comment 25•19 years ago
|
||
I backed this out, all locales (not only en-US) need to be updated otherwise compare-locales fails.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 26•19 years ago
|
||
(as Vidar mentioned on IRC, that involves adding the english strings to the other localizations and posting to n.p.m.l10n)
Assignee | ||
Comment 27•19 years ago
|
||
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
Comment 28•19 years ago
|
||
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.
Comment 29•19 years ago
|
||
(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.
Comment 30•19 years ago
|
||
Good. I still need to know on which branches you intend to land this, i.e. fx2 vs fx3.
Assignee | ||
Comment 31•19 years ago
|
||
(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.
Assignee | ||
Comment 32•19 years ago
|
||
Axel, any word on how the localization should be handled for this bug?
Assignee | ||
Comment 33•18 years ago
|
||
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)
Assignee | ||
Comment 34•18 years ago
|
||
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?
Comment 35•18 years ago
|
||
fixed the wiki, it had markup trouble with '<' and email addresses.
Comment 36•18 years ago
|
||
Added Hungarian translation to patch called 'Implement "Inspect document" v6'.
Attachment #207223 -
Attachment is obsolete: true
Comment 37•18 years ago
|
||
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.
Comment 38•18 years ago
|
||
Polish translation of these strings. We really need to come up with a better solution for mozilla/extensions l10n than this.
Comment 39•18 years ago
|
||
Comment 40•18 years ago
|
||
(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.
Comment 41•18 years ago
|
||
Comment 42•18 years ago
|
||
Comment 43•18 years ago
|
||
Comment 44•18 years ago
|
||
Comment 45•18 years ago
|
||
Comment 46•18 years ago
|
||
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.
Comment 47•18 years ago
|
||
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.
Comment 48•18 years ago
|
||
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 49•18 years ago
|
||
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 50•18 years ago
|
||
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 51•18 years ago
|
||
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 52•18 years ago
|
||
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 53•18 years ago
|
||
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 54•18 years ago
|
||
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 55•18 years ago
|
||
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
Comment 56•18 years ago
|
||
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
Comment 57•18 years ago
|
||
> 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.
Assignee | ||
Comment 58•18 years ago
|
||
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 ago → 18 years ago
Resolution: --- → FIXED
Comment 59•18 years ago
|
||
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!
Updated•17 years ago
|
QA Contact: timeless → dom-inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•