295711 - Make DOMI a "real" extension, and cut the app-specific crap

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect, P2)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

Attachment: DOMI-as-extension, checkpoint 1

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

Comment 2

[Alex Vincent [:WeirdAl]]

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?

Comment 3

[Benjamin Smedberg]

Attachment: Mac widget changes to hide menus with no contents

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

Comment 4

[Benjamin Smedberg]

Attachment: Hide empty menus from xul.css

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

Comment 5

[Asaf Romano (gone)]

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 6

[Asaf Romano (gone)]

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)

Comment 7

[Mike Connor [:mconnor]]

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?

Comment 8

[Benjamin Smedberg]

Attachment: Mac widget changes to hide menus with no contents, rev. 2

Updated, only hides main menus not submenus.

Comment 9

[Benjamin Smedberg]

Attachment: DOMI-as-extension, rev. 1

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

Comment 10

[Benjamin Smedberg]

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 11

[Benjamin Smedberg]

Attachment: Installers fixup

Comment 12

[Mike Connor [:mconnor]]

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 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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 14

[Johnny Stenback (:jst)]

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

sr=jst

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 185056 [details] [diff] [review]
Installers fixup

a=shaver pending review.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

a=shaver pending review

Comment 17

[jhp (no longer active)]

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.

Comment 18

[Andrew Schultz]

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.

Comment 19

[Samuel Sieb]

There's a workaround for chatzilla in bug 296679.

Comment 20

[Benjamin Smedberg]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 297570.

Comment 22

[Hiro]

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

Comment 23

[Alex Vincent [:WeirdAl]]

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?

Comment 24

[Benjamin Smedberg]

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.

Comment 25

[Scott MacGregor]

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.

Comment 26

[M.J.G.]

(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

Comment 27

[Scott MacGregor]

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

Comment 28

[Benjamin Smedberg]

That's a nonfatal warning.

Comment 29

[Scott MacGregor]

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.

Comment 30

[Benjamin Smedberg]

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

Comment 31

[M.J.G.]

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

Comment 32

[Scott MacGregor]

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

Comment 33

[M.J.G.]

(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

Comment 34

[M.J.G.]

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?

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

(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.

Comment 36

[M.J.G.]

(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

Comment 37

[Neil Deakin]

bsmedberg, can you explain why this rule was added to xul.css:

menu:empty {
  display: none;
}

Comment 38

[Benjamin Smedberg]

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.

Comment 39

[Neil Deakin]

(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;
}

Comment 40

[Myk Melez [:myk] [@mykmelez]]

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