314506 - no calendar.manifest packaged

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

There is no chrome/calendar.manifest which is needed to be able to run sunbird without prior registration. (Toolkit apps should follow the new manifest style IMHO).

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

I'll try to fix this.

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch to create a manifest

this creates a calendar.manifest which "might" work correctly. I have to do some more tests but it seems (almost) complete.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 201460 [details] [diff] [review]
patch to create a manifest

I guess you need some #ifdefs, to make sure you don't try to create a calendar.manifest for seamonkey.
Make sure to test a seamonkey build.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Attachment: more complete patch

this one contains the ifdefs to avoid the manifest files for seamonkey builds and don't package the content.rdf files for xul/toolkit apps.
It's still only tested for seamonkey with calendar and sunbird.

Comment 5

[Simon Paquet [:sipaq]]

Comment on attachment 201499 [details] [diff] [review]
more complete patch

>Index: calendar/sunbird/base/jar.mn
>===================================================================
> calendar.jar:
>+#ifdef MOZ_XUL_APP
>+% content calendar %content/calendar/
>+% overlay chrome://browser/content/browser.xul chrome://calendar/content/calExtOverlay.xul

I'm not a qualified reviewer for this, but may I ask what browser.xul has to do with Sunbird? As far as I know we do not package this file at the moment.

>+% overlay chrome://communicator/content/tasksOverlay.xul chrome://calendar/content/calendarOverlay.xul
>+% overlay chrome://communicator/content/pref/preftree.xul chrome://calendar/content/pref/prefOverlay.xul

The same.

>+% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://calendar/content/pref/prefOverlay.xul

The same.

>+% overlay chrome://messenger/content/mailWindowOverlay.xul chrome://calendar/content/calExtOverlay.xul

The same. Why do we need mail files here?

>+% overlay chrome://messenger/content/messengercompose/messengercompose.xul chrome://calendar/content/calExtOverlay.xul

The same.

>Index: calendar/resources/jar.mn
>===================================================================
> calendar.jar:
>+#ifdef MOZ_XUL_APP
>+%   skin calendar modern/1.0 %skin/modern/calendar/

We do not use the modern theme for all toolkit-based apps. Do we still need this entry?

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

I'm also not absolutely sure about it.
I just copied the overlays which were created from chrome registration out of the chrome package. I think that at least calendar/resources/jar.mn is generic and used also for extension creation for seamonkey. So these overlays might be correct.
The overlay entries in sunbird/base/jar.mn maybe can be removed. As I play the first time with this stuff I have to test it.

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

Attachment: 3rd attempt

This one includes Simon's comments and removes skin/modern for xul-apps including Sunbird as well as the overlay definitions.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 201507 [details] [diff] [review]
3rd attempt

>Index: calendar/resources/jar.mn
>+    content/calendar/import-export.js (/calendar/base/content/import-export.js)

Why this change?

>@@ -134,6 +150,7 @@
>     skin/classic/calendar/unifinder/priority_header.png (skin/classic/unifinder/priority_header.png)
>     skin/classic/calendar/unifinder/priority_high.png (skin/classic/unifinder/priority_high.png)
>     skin/classic/calendar/unifinder/priority_low.png (skin/classic/unifinder/priority_low.png)   
>+#ifndef MOZ_XUL_APP

And this? It looks very much out of the scope for tis bug.

Comment 9

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #8)
> (From update of attachment 201507 [details] [diff] [review] [edit])
> >Index: calendar/resources/jar.mn
> >+    content/calendar/import-export.js (/calendar/base/content/import-export.js)
> 
> Why this change?

This just has been moved up from the bottom for cosmetical reasons.

> >@@ -134,6 +150,7 @@
> >     skin/classic/calendar/unifinder/priority_header.png (skin/classic/unifinder/priority_header.png)
> >     skin/classic/calendar/unifinder/priority_high.png (skin/classic/unifinder/priority_high.png)
> >     skin/classic/calendar/unifinder/priority_low.png (skin/classic/unifinder/priority_low.png)   
> >+#ifndef MOZ_XUL_APP
> 
> And this? It looks very much out of the scope for tis bug.

I discussed this with Simon. Sunbird doesn't need any skin/modern stuff.
You are right that this is out of scope from this bug but it's only cleanup work . Do we need another bug to catch this?

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

> I discussed this with Simon. Sunbird doesn't need any skin/modern stuff.
> You are right that this is out of scope from this bug but it's only cleanup
> work . Do we need another bug to catch this?
> 
Yes, it needs a different bug, because it isn't risk-free. You never know what weird references are made in some xul, js or css file. I think the is already filed, and even has a patch. ...(checks)... yes, bug 307685

Comment 11

[Wolfgang Rosenauer [:wolfiR]]

Attachment: next try

removed the removal of the modern skin. Hopefully OK now.

Comment 12

[Wolfgang Rosenauer [:wolfiR]]

Attachment: manifest files for locales

This one is for the localization chrome packages. It changes the build process slightly to make it a bit easier to maintain (towards the i10n stuff as it works with FF or TB). Please have a look and comment on the patch.

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 201542 [details] [diff] [review]
next try

>Index: calendar/sunbird/base/jar.mn
>+#ifdef MOZ_XUL_APP
>+%   content calendar %content/calendar/
>+#ifndef MOZ_SUNBIRD

I'm wondering what the effect of this change is. From a quick grep, it seems this is the first change between the xpi for thunderbird and the xpi for seamonkey. Do we want that split? Can't we just include the rdf and the manifest?

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #13)
> I'm wondering what the effect of this change is. From a quick grep, it seems
> this is the first change between the xpi for thunderbird and the xpi for
> seamonkey. Do we want that split? Can't we just include the rdf and the
> manifest?

Now I'm confused. You didn't want a manifest file for seamonkey (comment #3).
So I create it only for xul-apps. But if there is a manifest file I don't need the rdf-files. Seems that I don't know enough about that stuff to provide a proper fix.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

I'm confused myself too. I need to do some testing and code-reading, I guess. I just don't know the .manifest system, and how it behaves in seamonkey.

Comment 16

[Simon Paquet [:sipaq]]

(In reply to comment #15)
> I'm confused myself too. I need to do some testing and code-reading, I guess. I
> just don't know the .manifest system, and how it behaves in seamonkey.

Seamonkey doesn't use the manifest system yet. It is only used in Toolkit 1.8 apps like TB 1.5 and FF 1.5 as of now.

See http://developer.mozilla.org/en/docs/Chrome_Manifest and http://developer.mozilla.org/en/docs/Chrome_Manifest#Old-style_contents.rdf_manifests

Comment 17

[Wolfgang Rosenauer [:wolfiR]]

Attachment: my last try

This is my last try and it changes the absolute minimum in my eyes.
For the shared stuff in calendar/resources it always creates a manifest file (even for seamonkey as it doesn't hurt IMHO (reporter also has a manifest file in seamonkey). And it removes (only for sunbird builds hopefully) the contents.rdf from the JAR. If this wouldn't be done it would (AFAIK) register the chrome also in installed-chrome.txt which would create some potential problems.

Comment 18

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 201550 [details] [diff] [review]
manifest files for locales

This patch for the locales is not wanted here. I won't make a patch which creates manifest files for the locales until the creation mechanism has been changed anyhow. But this is another bug.

Comment 19

[Simon Paquet [:sipaq]]

Comment on attachment 201827 [details] [diff] [review]
my last try

>Index: calendar/resources/jar.mn
>===================================================================
>
> calendar.jar:
>+%   content calendar %content/calendar/
>+#ifndef MOZ_SUNBIRD
>+%   overlay chrome://browser/content/browser.xul chrome://calendar/content/calExtOverlay.xul
>+%   overlay chrome://communicator/content/tasksOverlay.xul chrome://calendar/content/calendarOverlay.xul
>+%   overlay chrome://communicator/content/pref/preftree.xul chrome://calendar/content/pref/prefOverlay.xul
>+%   overlay chrome://communicator/content/pref/pref-appearance.xul chrome://calendar/content/pref/prefOverlay.xul
>+%   overlay chrome://messenger/content/mailWindowOverlay.xul chrome://calendar/content/calExtOverlay.xul
>+%   overlay chrome://messenger/content/messengercompose/messengercompose.xul chrome://calendar/content/calExtOverlay.xul
>+*   content/calendar/contents.rdf (content/contents.rdf)
>+    locale/en-US/calendar/contents.rdf (locale/en-US/contents.rdf)
>+*   skin/classic/calendar/contents.rdf (skin/classic/contents.rdf)
>+*   skin/modern/calendar/contents.rdf (skin/modern/contents.rdf)
>+#endif

I would add an #ifndef MOZ_XUL_APP before the contents.rdf files, since these files will only be used by Seamonkey/Suite and may create some potential problems as you correctly state in comment 17. While you're there you could also put the preftree.xul and pref-appearance.xul files into this #ifndef, since those files are Suite-/Seamonkey-specific files.

Otherwise this looks good to me, but you should wait for mvl's review, since he knows this code much better than I do.

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

Let's not add any extra ifdefs until we are sure they are need.

Comment 21

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #20)
> Let's not add any extra ifdefs until we are sure they are need.

Was this a comment to the latest patch or to sipaq?
As I mentioned the posted patch is a minimalistic change to provide a correct manifest file for sunbird.
As you can see in the history of this bug this is not my favourite patch since there are some other things which needs cleanup. You wanted a minimalistic change and there you get it. I fixes just this one issue.

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

Sorry, the comment was to sipaq. We need to test if the contents.rdf really gived problems before removing it for certain builds.

Comment 23

[Wolfgang Rosenauer [:wolfiR]]

According to bug #313894 it can cause problems. I'm not sure that the fix which is used there is sufficient because I fear if someone is calling sunbird -register or regchrome it will again end up in installed-chrome.txt (while I'm not really sure about NO_JAR_AUTO_REG = 1).
Adding Benjamin for clarification.

Comment 24

[Benjamin Smedberg]

 Basically you should use manifests and no installed-chromed.txt entries for toolkit apps/extensions, and only use ifdefed installed-chrome.txt and contents.rdf for code that needs to continue to work in seamonkey.

Comment 25

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 201827 [details] [diff] [review]
my last try

Ok, seamonkey and sunbird still work with this patch r=mvl

Comment 26

[Michiel van Leeuwen (email: mvl+moz@)]

checked in