399571 - Is communicator part of toolkit?

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[[e-mail]]

As it stands, toolkit, and therefore presumably XULRunner, ships a stub communicator skin. Why? If it's necessary for toolkit to provide it, can a XULRunner app reliably override this stub skin with a real skin? If so, how?

Comment 1

[Robert Kaiser]

The files in http://mxr.mozilla.org/mozilla/find?string=stripe%2Fcommunicator&tree=mozilla do really sound like a bug in toolkit to me, IMHO they just should go away...

Comment 2

[Phil Ringnalda (:philor)]

Judging by usage (in PKI, in mailnews, in Bugzilla's sidebar template, in XUL Planet tutorials, and in random googleable things) I assumed that chrome://communicator/skin/ was at one time what's now chrome://global/skin/, and that providing it was part of anything that consumed XUL, and thus, yeah, part of toolkit. But maybe I'm misunderstanding the question.

Comment 3

[[e-mail]]

I believe the idea was that it defined styles shared by communicator components such as the sidebar (and its panels, including bookmarks and history), taskbar, preferences, or the primary toolbar.

Comment 4

[Robert Kaiser]

Yes, global/ was always there in addition to communicator/ - from how I understand it, global/ was always for stuff that applies to any XUL anywhere, where communicator/ was meant to handle stuff that is used for the whole suite but might not make sense in other XUL-based applications. With the sparse use of XUL outside of the suite in earlier years, the distinction of that was sometimes not that visible and not even respected in some cases, but nowadays, communicator/ should in theory be SeaMonkey-only - and we're actually not far from that in practice.

Comment 5

[Karsten Düsterloh]

Actually, since both *stripe communicator.css just include chrome://global/skin, I think that their existence in toolkit is just lazyness and should get removed. ;-)
(Maybe DOMI/Venkman/Chatzilla need to be fixed.)

Comment 6

[Phil Ringnalda (:philor)]

http://mxr.mozilla.org/mozilla/source/security/manager/pki/resources/content/crlManager.xul#41
http://mxr.mozilla.org/mozilla/source/extensions/sroaming/resources/content/prefs/firefox.xul#40 (plus several more in sroaming, though I wouldn't be surprised if it doesn't work these days)
http://mxr.mozilla.org/mozilla/source/mailnews/compose/resources/content/MsgAttachPage.xul#43
http://mxr.mozilla.org/mozilla/source/mailnews/base/search/resources/content/CustomHeaders.xul#39
http://mxr.mozilla.org/mozilla/source/extensions/layout-debug/ui/content/layoutdebug.xul#53

So, say you fix those, plus the ones I'm too lazy to copy-paste, plus the ones in mail/ that I'll fix. Then...

http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/template/en/default/sidebar.xul.tmpl#27

means you turn the blue links on grey of however many hundred bugzillas there are to black underlined words on white in Firefox, and do who knows what to how many other sidebar panels and XUL apps written over the last seven years of that working, right or wrong, in every XUL consuming application.

So, is this targetted at Moz2, where the whole world's supposed to break anyway?

Comment 7

[Karsten Düsterloh]

Uh, I haven't made myself clear enough. What I meant is "Since both *stripe communicator.css just include chrome://global/skin/, I think all occurences of chrome://communicator/skin/ should get replaced by chrome://global/skin/ whereever that isn't already specified."

I don't see why this should break anything?

(I don't know how many 3rd party themes overlay/use chrome://communicator/skin/ for toolkit, but they'd just be plain wrong in doing so...)

Comment 8

[[e-mail]]

(In reply to comment #7)
>Uh, I haven't made myself clear enough. What I meant is "Since both *stripe
>communicator.css just include chrome://global/skin/, I think all occurrences of
>chrome://communicator/skin/ should get replaced by chrome://global/skin/
>wherever that isn't already specified."
Right, but how do we fix third-party occurrences?

(In reply to comment #6)
>... means you turn the blue links on grey of however many hundred bugzillas
>there are to black underlined words on white in Firefox...
Firefox is of course free to provide its own stub communicator skin.
I just don't think toolkit should do it.

Comment 9

[[e-mail]]

Attachment: Proposed patch

Comment 10

[Asaf Romano (gone)]

Could we use chrome overrides instead?

Comment 11

[[e-mail]]

@import is more efficient.

Comment 12

[Scott MacGregor]

Comment on attachment 287303 [details] [diff] [review]
Proposed patch

sr=me, but you'll still need moa for the browser changes.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 287303 [details] [diff] [review]
Proposed patch

sold, r=mano.

Comment 14

[Reed Loden [:reed]]

Don't you need to do the same thing for gnomestripe, too, now that it has landed?

Comment 15

[[e-mail]]

(In reply to comment #14)
>Don't you need to do the same thing for gnomestripe
No; both in toolkit and browser, gnomestripe is winstripe plus a few overrides.

Comment 16

[Reed Loden [:reed]]

(In reply to comment #15)
> (In reply to comment #14)
> >Don't you need to do the same thing for gnomestripe
> No; both in toolkit and browser, gnomestripe is winstripe plus a few overrides.

No, it's only an override in toolkit. It's a separate theme in browser.

Comment 17

[[e-mail]]

Ah, I see now, the browser version of the makefile is confusingly written.
(rev 1.5 works for me)

Comment 18

[[e-mail]]

Attachment: Updated for gnomestripe

Assuming r=mano,mscott still holds with this trivial change.

Comment 19

[[e-mail]]

Fix checked in, except for a couple of erroneous Makefile changes backed out.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

This bug will cause some grief for XULRunner apps that use extensions like DOMi and Venkman (and any others that use chrome://communicator/skin/ instead of chrome://global/skin/)

I'll make a note on MDC that such XULRunner apps should create a fake "communicator" skin that just imports the "global" skin

Comment 21

[[e-mail]]

...or you could just fix them to use global instead, you know...

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 404853 and bug 404851