Closed Bug 331828 Opened 18 years ago Closed 18 years ago

Can't inspect windows on Mac

Categories

(Other Applications :: DOM Inspector, defect)

PowerPC
macOS
defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha

People

(Reporter: philor, Assigned: philor)

References

Details

(Keywords: dogfood, regression)

Attachments

(1 file, 1 obsolete file)

Bug 109481 broke inspecting chrome on the Mac: View > Chrome makes the open windows show up in the File > Inspect Document menu, but selecting one doesn't do anything at all: no errors, no actions.
Can you start the app from the terminal and see if anything's output there?
Oh, there was a reason why I was updating my debug build, wasn't there? Nothing but a string of six

WARNING: Not implemented: file /Users/philringnalda/moz/mozilla/widget/src/mac/nsMenuX.cpp, line 403
In addition to windows, I can't inspect documents by selecting them from the "Inspect Document" menu, either.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060402 Firefox/1.6a1 ID:2006040301
Flags: blocking1.9a1?
We need to investigate this more.  Any debug output from the console that can help pinpoint this is useful.
Keywords: dogfood
Attachment 207152 [details] [diff] changed the Inspect menu to load/unload lazily when onpopupshowing/onpopuphiding are fired.

When the menu hides, it removes each item in the submenu but the mac menu widget code doesn't support this. Thus the assertion in comment 2.

Still investigating this...
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060517 Minefield/3.0a1
Formerly I could choose: File > Inspect a window, and then I got information about the chrome, but now when I choose File > Inspect a document, I don't see that happen. Nothing happens in fact. See no errors in the JSConsole.
The regression range points to bug 109481 as the culprit.
The branch DOMI still works fine.
Ria: see comment 0. View menu, Chrome, to toggle including chrome windows in the File menu's list. Though an item at the bottom of the list itself that toggles between "Include chrome windows" and "Hide chrome windows" would probably be better UI.
(In reply to comment #7)
>
OK thanks :)
So, what are we going to do here? DOMi's now essentially unusable on the Mac on trunk, and has been for almost three months, and now that the days of freely checking everything into trunk and branch are over, it's only a matter of time before Firefox has something broken Mac-only that can't be inspected. Personally, I don't think the UI benefits from bug 109481 were that great, to abandon one platform for them.
Severity: major → blocker
If the problem is indeed related to comment 2, it seems relatively minor and must've been encountered before somewhere else - we need to remove the menu items all at once rather than one at a time.

Not having a Mac, though, there's no way for me to make/test a patch.
To see if the menu item removal really is the problem, could someone with a Mac try removing the onpopuphiding attribute here?
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/popupOverlay.xul#32
Yeah, lxring appendChild(menu and looking at how they cleared the previous menu already led me to clearing onpopupshowing rather than onpopuphiding, so I have a working build, just need to clean up the patch and test on all three platforms.
Assignee: dom-inspector → philringnalda
Someone ought to file a bug on the underlying issue here though with a testcase of the XUL that is non-working on mac (due to mac's special menu code)
Attached patch fix (obsolete) — Splinter Review
Attachment #226481 - Flags: review?(timeless)
Comment on attachment 226481 [details] [diff] [review]
fix

>+      <menupopup id="listDocuments-popup" onpopupshowing="inspector.showInspectDocumentList()"/>

Nit: Maybe put the onpopupshowing attribute on the next line, to keep each line under 80 characters.

Looks good to me, though.
Comment on attachment 226481 [details] [diff] [review]
fix

>+    // clear out existing menu

I don't see the value of in-lining this code.

>+    var popup = this.mInspectDocumentMenu;
>+    while (popup.firstChild) {
>+      popup.removeChild(popup.firstChild);      
>+    }

Using the out of line function gives a better indication to me about what's going on. but i don't feel that strongly about it.

as for the 80 cols, i accept that you weren't really changing that line, but unbunching it to two rows would probably be slightly better.
Attachment #226481 - Flags: superreview?(neil)
Attachment #226481 - Flags: review?(timeless)
Attachment #226481 - Flags: review+
Comment on attachment 226481 [details] [diff] [review]
fix

>+    // clear out existing menu
Logically I feel this code belongs at line 405-410 instead.

>+    while (popup.firstChild) {
If you're going to change the test I would suggest popup.hasChildNodes()

>+      popup.removeChild(popup.firstChild);      
The old code popup.removeChild(popup.lastChild); is slightly more efficient.
Attachment #226481 - Flags: superreview?(neil) → superreview+
Attached patch for checkinSplinter Review
Not inlined, called in a more reasonable place, with hasChildNodes() and killing the last, youngest and weakest child first, and under 80 cols.

Thanks for the comments!
Attachment #226481 - Attachment is obsolete: true
Whiteboard: [checkin needed]
mozilla/extensions/inspector/resources/content/popupOverlay.xul 	1.15
mozilla/extensions/inspector/resources/content/inspector.js 	1.28
Status: NEW → RESOLVED
Closed: 18 years ago
Flags: blocking1.9a1?
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
Does this bug exist on the branch? If so, we should definitely nominate it as blocker, since DOM inspector is hardly usable with this bug...
No, bug 109481 didn't touch branch, so we're fine.
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: