DOMI should be a "real" extension installed in <appdir>/extensions and managed through the extension manager. Making this patch has revealed some unfortunate side effects of the seamonkey/firefox split, especially WRT mac overlays. We really to get a group together to sit down and seriously map out a strategy for mac menus in xulrunner apps and extensions.

This is a checkpoint patch: it depends on the changes in bug 295494, it needs installer love, seamonkey testing, tbird testing, and mac testing.

bsmedberg: Let me see if I get this straight. You want to do something like the aviary landing, but for DOM Inspector: bringing the code that was forked back into our trunk codebase under extensions/inspector. Is that right? Or did you have some other intention here that I'm misreading?

This hides mac menus when there is no subcontent (submenu).

I will make an identical change to the seamonkey xul.css.

Comment on attachment 185026 [details] [diff] [review] Mac widget changes to hide menus with no contents On mac, the submenu behavior here ins't acceptable, If a menu's submenu is empty (say, the bookmarks toolbar folder under the bookmarks menu), it should be disabled, not hidden.

Comment on attachment 185026 [details] [diff] [review] Mac widget changes to hide menus with no contents (And, if we want to keep this bug in its scope, you can just skip the loadmenuitem part)

I know menus where all options are dimmed (disabled) the menu still needs to be available, but where is the behaviour of empty menus specified?

Updated, only hides main menus not submenus.

This patch requires the other two here, plus one more installer change patch that I'll attach from my windows machine.

Comment on attachment 185054 [details] [diff] [review] DOMI-as-extension, rev. 1 I know that caillon is gone for a while; don't know how long. Picking likely reviewers from a mythical hat, please pass along as appropriate.

Comment on attachment 185054 [details] [diff] [review] DOMI-as-extension, rev. 1 Let the guy on the SR list be the SR of record here. Looks good, minor fixup to change the <key/> for tbird to use the localized entity, but that can get done after.

Comment on attachment 185054 [details] [diff] [review] DOMI-as-extension, rev. 1 Yeah, that's great. I'm so happy to see this work, but I think you should fix the l10n issue before committing. sr+a=shaver.

Comment on attachment 185052 [details] [diff] [review] Mac widget changes to hide menus with no contents, rev. 2 sr=jst

Comment on attachment 185056 [details] [diff] [review] Installers fixup a=shaver pending review.

Comment on attachment 185052 [details] [diff] [review] Mac widget changes to hide menus with no contents, rev. 2 a=shaver pending review

Comment on attachment 185052 [details] [diff] [review] Mac widget changes to hide menus with no contents, rev. 2 Well, I'd rather have the firefox/seamonkey disparity fixed, but that would break many things. So this looks ok. My only concern is if we have any menus in firefox or seamonkey which have no children as one of their states; this patch would cause them to hide. But I can't think of any such menu.

FYI: chatzilla starts with empty menus and populates them with JS. But because of the change here and bug 98997, they still don't show up.

There's a workaround for chatzilla in bug 296679.

Whoops, I apparently forgot to mark this fixed (for 1.8b3).

This caused crash bug 297570.

Please remove the following entries from "allmakefiles.sh". http://lxr.mozilla.org/mozilla/source/allmakefiles.sh#923 configure log message: creating browser/extensions/inspector/Makefile can't read ./browser/extensions/inspector/Makefile.in: No such file or directory creating browser/extensions/Makefile

Warning: Failed to load overlay from chrome://browser/content/baseMenuOverlay.xul. Source File: chrome://inspector/content/inspector.xul Line: 0 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050726 SeaMonkey/1.0a bsmedberg: might inspector.xul need a little preprocessing to get rid of that warning?

We must not preprocess the extension like that... I'm trying to use the same binary for all applications. We can probably add app-specific chrome registration modifiers.

Late comments from the peanut gallery. I was just messing around with building thunderbird with the inspector extension and these changes worked well with one exception. inspector.xul loads tasksOverlay.xul into the chrome which in turn has navigator and editor specific key bindings which aren't present in thunderbird. Removing this overlay line from inspector.xul made the entity errors go away in the dom inspector window. Screen shot of the entity errors: http://members.dodo.com.au/~tigger_and_pooh/DOMi.jpg What's the impact to the seamonkey experience if we just took out this line (firefox doesn't have this file so it's a no-op for them)? Alternatively, we can figure out how to make thunderbid not include tasksOverlay.xul so it get stripped out of the build process.

(In reply to comment #25) > Late comments from the peanut gallery. > > I was just messing around with building thunderbird with the inspector > extension and these changes worked well with one exception. inspector.xul loads > tasksOverlay.xul into the chrome which in turn has navigator and editor > specific key bindings which aren't present in thunderbird. Removing this > overlay line from inspector.xul made the entity errors go away in the dom > inspector window. I'm messing around with the DOMi for TB 1.5 on Linux. I got it to work as an extension with the release build, avoiding the taskOverlay related glitches as above. I do see a line "No chrome package registered for chrome://browser/locale/browser.dtd ." in the JS console. DOMi works anayway, but do you get the same line with your MAC and Win builds? I don't see any references to browser.dtd anywhere in the DOMi. Once this is clarified I'd be happy to upload to addons or to share it with Scott so that all three builds somehow go together. Michael

Michael, I don't see the browser.dtd warning, but when I open DOMI from thunderbird, I do see the following: Couldn't convert chrome URL: chrome://browser/content/baseMenuOverlay.xul which comes from this line in inspector.xul http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/content/inspector.xul#43

That's a nonfatal warning.

yup, that's not fatal. The line that is 'fatal' is the one I talked about in comment 25. Thunderbird dom inspector users have to remove that line from their builds to get rid of the entity errors.

We can use a chrome-registry specified overlay instead of making it hardcoded, to make the overlay conditional on seamonkey.

(In reply to comment #27) > Michael, I don't see the browser.dtd warning, but when I open DOMI from > thunderbird, I do see the following: > > Couldn't convert chrome URL: chrome://browser/content/baseMenuOverlay.xul > > which comes from this line in inspector.xul > > http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/content/inspector.xul#43 Yes, that's how I got rid of the baseMenuOverlay warning. I don't know where the other one comes from, it's not there without inspector. bmsedberg: I know it's not fatal; it's just not nice either. mscott: Shoud I go ahead and upload inspectror-tb-linux to addons, or do you want to manage all three builds yourself? Michael BTW: Lot's of discussion here for a "resolved fixed" bug ;)

It'd be great if you could put it on addons for me. Thanks for doing that.

(In reply to comment #32) > It'd be great if you could put it on addons for me. Thanks for doing that. > OK, it's in the queue. Cheers, Michael

I've repackaged DOMi as an extension for Thunderbird 1.5.0.2 on Linux. addons.mozilla.org does not allow a maxVersion of 1.5.0.2 right now. I don't want to specify 1.5.0.* just as ti work around there bugs. They haven't even approved DOMi for TB 1.5 on L yet. As soon as mozdev mirrors have caught up you can download the current DOMi for Thunderbird 1.5.0.2 on Linux from http://downloads.mozdev.org/aboutconfig/inspector-1.5.0.2.xpi Michael P.S.: Should I even report updates likes this here in the bug?

(In reply to comment #34) > > P.S.: Should I even report updates likes this here in the bug? > Seems reasonable to me, since folks CCed on this bug are likely to care.

(In reply to comment #34) > I've repackaged DOMi as an extension for Thunderbird 1.5.0.2 on Linux. > addons.mozilla.org does not allow a maxVersion of 1.5.0.2 right now. I don't > want to specify 1.5.0.* just as ti work around there bugs. They haven't even > approved DOMi for TB 1.5 on L yet. Update: It's on addons.mozilla.org now that 1.5.0.2 is accepted as maxVersion. Thanks Mike S! Michael

bsmedberg, can you explain why this rule was added to xul.css: menu:empty { display: none; }

IIRC, there were integration points that existed in SeaMonkey but not in Firefox (i.e. the "Window" menu). So DOMI put a "window" menu in its XUL, but if there wasn't an overlay with a menupopup, the menu wouldn't be rendered.

(In reply to comment #38) > IIRC, there were integration points that existed in SeaMonkey but not in > Firefox (i.e. the "Window" menu). So DOMI put a "window" menu in its XUL, but > if there wasn't an overlay with a menupopup, the menu wouldn't be rendered. > So why is this rule not in the dom inspector css? Or can it at least be more specific, as in: menubar > menu:empty { visibility: collapse; }

Note: the visibility: collapse rule change is responsible for bug 314020, which will become more important when bug 461899 gets fixed.