Can't inspect windows on Mac

RESOLVED FIXED in mozilla1.9alpha

Status

Other Applications
DOM Inspector
--
blocker
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: philor, Assigned: philor)

Tracking

({dogfood, regression})

Trunk
mozilla1.9alpha
PowerPC
Mac OS X
dogfood, regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.46 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
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.

Comment 1

12 years ago
Can you start the app from the terminal and see if anything's output there?
(Assignee)

Comment 2

12 years ago
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?

Comment 4

12 years ago
We need to investigate this more.  Any debug output from the console that can help pinpoint this is useful.
Keywords: dogfood

Comment 5

12 years ago
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.
(Assignee)

Comment 7

12 years ago
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 :)
(Assignee)

Comment 9

12 years ago
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

Comment 10

12 years ago
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.

Comment 11

12 years ago
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
(Assignee)

Comment 12

12 years ago
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

Comment 13

12 years ago
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)
(Assignee)

Comment 14

12 years ago
Created attachment 226481 [details] [diff] [review]
fix
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 16

12 years ago
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 17

12 years ago
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+
(Assignee)

Comment 18

12 years ago
Created attachment 227889 [details] [diff] [review]
for checkin

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
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
mozilla/extensions/inspector/resources/content/popupOverlay.xul 	1.15
mozilla/extensions/inspector/resources/content/inspector.js 	1.28
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.9a1?
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha

Comment 20

12 years ago
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...
(Assignee)

Comment 21

12 years ago
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.