Closed Bug 314506 Opened 14 years ago Closed 14 years ago

no calendar.manifest packaged

Categories

(Calendar :: Sunbird Only, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wolfiR, Assigned: mostafah)

References

Details

Attachments

(2 files, 4 obsolete files)

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).
I'll try to fix this.
Attached patch patch to create a manifest (obsolete) — Splinter Review
this creates a calendar.manifest which "might" work correctly. I have to do some more tests but it seems (almost) complete.
Assignee: mostafah → mozilla
Status: NEW → ASSIGNED
Attachment #201460 - Flags: first-review?
Attachment #201460 - Flags: first-review? → first-review?(mvl)
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.
Attached patch more complete patch (obsolete) — Splinter Review
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.
Attachment #201460 - Attachment is obsolete: true
Attachment #201499 - Flags: first-review?(mvl)
Attachment #201460 - Flags: first-review?(mvl)
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?
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.
Attached patch 3rd attempt (obsolete) — Splinter Review
This one includes Simon's comments and removes skin/modern for xul-apps including Sunbird as well as the overlay definitions.
Attachment #201499 - Attachment is obsolete: true
Attachment #201507 - Flags: first-review?(mvl)
Attachment #201499 - Flags: first-review?(mvl)
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.
Attachment #201507 - Flags: first-review?(mvl) → first-review-
(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?
> 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
Attached patch next try (obsolete) — Splinter Review
removed the removal of the modern skin. Hopefully OK now.
Attachment #201507 - Attachment is obsolete: true
Attachment #201542 - Flags: first-review?(mvl)
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 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?
(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.
Assignee: mozilla → mostafah
Status: ASSIGNED → NEW
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.
(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
Attached patch my last trySplinter Review
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.
Attachment #201542 - Attachment is obsolete: true
Attachment #201827 - Flags: first-review?(mvl)
Attachment #201542 - Flags: first-review?(mvl)
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 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.
Let's not add any extra ifdefs until we are sure they are need.
(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.
Sorry, the comment was to sipaq. We need to test if the contents.rdf really gived problems before removing it for certain builds.
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.
 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.
Depends on: 267981
Comment on attachment 201827 [details] [diff] [review]
my last try

Ok, seamonkey and sunbird still work with this patch r=mvl
Attachment #201827 - Flags: first-review?(mvl) → first-review+
checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.