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

RESOLVED FIXED in mozilla1.8alpha3

Status

P2
normal
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.8alpha3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

14 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.8alpha3
(Assignee)

Comment 1

14 years ago
Created attachment 184683 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Depends on: 295494
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?

(Assignee)

Comment 3

14 years ago
Created attachment 185026 [details] [diff] [review]
Mac widget changes to hide menus with no contents

This hides mac menus when there is no subcontent (submenu).
Attachment #185026 - Flags: superreview?(jst)
Attachment #185026 - Flags: review?(joshmoz)
(Assignee)

Comment 4

14 years ago
Created attachment 185031 [details] [diff] [review]
Hide empty menus from xul.css

I will make an identical change to the seamonkey xul.css.
Attachment #184683 - Attachment is obsolete: true
Attachment #185031 - Flags: review?(mconnor)
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.
Attachment #185026 - Flags: review?(joshmoz) → review-
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

14 years ago
Attachment #185031 - Flags: review?(mconnor) → review+
(Assignee)

Comment 8

14 years ago
Created attachment 185052 [details] [diff] [review]
Mac widget changes to hide menus with no contents, rev. 2

Updated, only hides main menus not submenus.
Attachment #185026 - Attachment is obsolete: true
Attachment #185052 - Flags: superreview?(jst)
Attachment #185052 - Flags: review?(joshmoz)

Updated

14 years ago
Attachment #185026 - Flags: superreview?(jst)
(Assignee)

Comment 9

14 years ago
Created attachment 185054 [details] [diff] [review]
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.
(Assignee)

Comment 10

14 years ago
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.
Attachment #185054 - Flags: superreview?(mconnor)
Attachment #185054 - Flags: review?(dmose)
(Assignee)

Comment 11

14 years ago
Created attachment 185056 [details] [diff] [review]
Installers fixup
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.
Attachment #185054 - Flags: superreview?(mconnor)
Attachment #185054 - Flags: superreview?(dmose)
Attachment #185054 - Flags: review?(dmose)
Attachment #185054 - Flags: review+
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.
Attachment #185054 - Flags: superreview?(dmose)
Attachment #185054 - Flags: superreview+
Attachment #185054 - Flags: approval-aviary1.1a2+
Comment on attachment 185052 [details] [diff] [review]
Mac widget changes to hide menus with no contents, rev. 2

sr=jst
Attachment #185052 - Flags: superreview?(jst) → superreview+
Comment on attachment 185056 [details] [diff] [review]
Installers fixup

a=shaver pending review.
Attachment #185056 - Flags: approval-aviary1.1a2+
Comment on attachment 185052 [details] [diff] [review]
Mac widget changes to hide menus with no contents, rev. 2

a=shaver pending review
Attachment #185052 - Flags: approval-aviary1.1a2+
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.
Attachment #185052 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #185056 - Flags: review?(mconnor)

Updated

14 years ago
Attachment #185056 - Flags: review?(mconnor) → review+

Updated

14 years ago
Blocks: 296622

Comment 18

14 years ago
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.
Blocks: 296679

Comment 19

14 years ago
There's a workaround for chatzilla in bug 296679.
(Assignee)

Comment 20

14 years ago
Whoops, I apparently forgot to mark this fixed (for 1.8b3).
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
This caused crash bug 297570.

Updated

14 years ago
Attachment #185052 - Flags: review?(joshmoz)

Comment 22

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

Updated

13 years ago
Blocks: 301252
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?
(Assignee)

Comment 24

13 years ago
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.
Depends on: 307045
Depends on: 314020

Comment 25

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

Updated

13 years ago
Blocks: 327833

Comment 26

13 years ago
(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

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

(Assignee)

Comment 28

13 years ago
That's a nonfatal warning.

Comment 29

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

Comment 30

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

Comment 31

13 years ago
(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

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

Comment 33

13 years ago
(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

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

Comment 36

13 years ago
(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
No longer blocks: 327833
Depends on: 327833
QA Contact: timeless → dom-inspector

Comment 37

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

menu:empty {
  display: none;
}

(Assignee)

Comment 38

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

11 years ago
(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.
You need to log in before you can comment on or make changes to this bug.