Closed Bug 882761 Opened 11 years ago Closed 10 years ago

Package all theme files in a unified XPI

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Paenglab, Assigned: Paenglab)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

With ical.js where will be no need to package platform specific XPIs. But this needs all platform theme files in one XPI.
Attached patch WIP patch (obsolete) β€” β€” Splinter Review
This patch adds in chrome folder a skin folder. In this folder there there this stucture:

- common  (the common theme files)
- linux   (the Linux specific files)
- osx     (the OSX specific files)
- windows (the Windows specific files)

'skin calendar' and 'skin lightning' will be registered platform depending  to the calendar and lightning folder in the platform folders.

To access the common files, chrome://calendar-common/skin/... will do this.

The problem I'm seeing is, the manifest file isn't created with the same order as the entries are in jar.mn. This makes it impossible to add other platforms to register to a default theme folder. The order need to be strictly like this to work on all platforms correctly:

% skin calendar classic/1.0 chrome/skin/linux/calendar/
% skin calendar classic/1.0 chrome/skin/osx/calendar/ os=Darwin
% skin calendar classic/1.0 chrome/skin/windows/calendar/ os=WINNT

This would mean, linux is the default folder for all platforms except OS X and Windows. This two have their own folders. Linux is only a proposal as I think, linux is not optimal because GTX-icons don't be supported on all other platforms.

If somebody knows how to fix the order in manifest, please tell how to do.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
I'm waiting after the merge to TB 25 Lightning 2.7 for review to not risk breakage in the ESR release.
I think Linux being the default is our best bet. Aside from OS/2, there are likely a few other unix flavors that will not be covered by using "Linux".

I'm often angry with chrome registration flags not being flexible enough. Afaik we can't just do this:

> skin calendar classic/1.0 chrome/skin/linux/calendar/ os!=Darwin os!=WINNT

So here is a hack that might work. In the main file:

> include notwindows.manifest os!=WINNT

Then in notwindows.manifest:

> include general.manifest os!=Darwin

Then in general.manifest

> skin calendar classic/1.0 chrome/skin/linux/calendar/


Filenames are subject to change of course.
Or instead of generating the manifest file, create it with the correct order and copy it to the right place with jar.mn. Would this work?

I think it's no difference when new or changed rules are written in manifest file than in jar.mn.
This means we need one central file for the manifest, I think thats counterproductive. Maybe instead we could fix the manifest parser to retain the local order of lines per file.
I filed bug 891476 for a fixed order during build.
Depends on: 891476
Attached patch patch (obsolete) β€” β€” Splinter Review
Okay, let's try a review to have enough time until 31ESR. I'm using it on every build and saw no issue on Windows and Linux.
Attachment #762130 - Attachment is obsolete: true
Attachment #8333985 - Flags: review?(philipp)
Comment on attachment 8333985 [details] [diff] [review]
patch

Review of attachment 8333985 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with these comments considered/clarified.

::: calendar/base/jar.mn
@@ +113,5 @@
>  % style chrome://global/content/customizeToolbar.xul chrome://calendar/skin/calendar-task-view.css
>  % style chrome://global/content/customizeToolbar.xul chrome://calendar/skin/calendar-event-dialog.css
> +% style chrome://calendar/content/calendar-event-dialog.xul chrome://calendar-common/skin/dialogs/calendar-event-dialog.css
> +% style chrome://calendar/content/calendar-event-dialog-attendees.xul chrome://calendar-common/skin/dialogs/calendar-event-dialog.css
> +    ../skin/common/alarm-flashing.png                 (themes/common/images/alarm-flashing.png)

Just for clarification: using the parent directory here does what to the final path?

@@ +226,5 @@
>  % style chrome://calendar/content/calendar-event-dialog.xul chrome://calendar-windows/skin/calendar-event-dialog.css os=WINNT
>  % style chrome://calendar/content/calendar-event-dialog-attendees.xul chrome://calendar-windows/skin/calendar-event-dialog.css os=WINNT
>  % style chrome://calendar/content/calendar-occurrence-prompt.xul chrome://calendar-windows/skin/calendar.css os=WINNT osversion>=6
> +% skin calendar-windows classic/1.0 chrome/skin/windows/calendar/win-classic/ os=WINNT osversion<6
> +% skin calendar-windows classic/1.0 chrome/skin/windows/calendar/win-aero/ os=WINNT osversion>=6

Do you think it makes sense to move these skin registrations up to the place where the other skin registrations for Linux/Mac/Windows are done?

::: calendar/lightning/themes/linux/imip.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +@import url(chrome://calendar-common/skin/imip.css);

Shouldn't this be lightning-common (also in a few other places) ? Even though we only have one product in tree now, I don't think we should mix calendar/lightning just yet.
Attachment #8333985 - Flags: review?(philipp) → review+
Attached patch Patch for check-in β€” β€” Splinter Review
(In reply to Philipp Kewisch [:Fallen] from comment #8)
> Comment on attachment 8333985 [details] [diff] [review]
> patch
> 
> Review of attachment 8333985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with these comments considered/clarified.
> 
> ::: calendar/base/jar.mn
> @@ +113,5 @@
> >  % style chrome://global/content/customizeToolbar.xul chrome://calendar/skin/calendar-task-view.css
> >  % style chrome://global/content/customizeToolbar.xul chrome://calendar/skin/calendar-event-dialog.css
> > +% style chrome://calendar/content/calendar-event-dialog.xul chrome://calendar-common/skin/dialogs/calendar-event-dialog.css
> > +% style chrome://calendar/content/calendar-event-dialog-attendees.xul chrome://calendar-common/skin/dialogs/calendar-event-dialog.css
> > +    ../skin/common/alarm-flashing.png                 (themes/common/images/alarm-flashing.png)
> 
> Just for clarification: using the parent directory here does what to the
> final path?

This is to go out of the calendar and lightning directory and to create a skin directory at the same level of calendar and lightning.

> @@ +226,5 @@
> >  % style chrome://calendar/content/calendar-event-dialog.xul chrome://calendar-windows/skin/calendar-event-dialog.css os=WINNT
> >  % style chrome://calendar/content/calendar-event-dialog-attendees.xul chrome://calendar-windows/skin/calendar-event-dialog.css os=WINNT
> >  % style chrome://calendar/content/calendar-occurrence-prompt.xul chrome://calendar-windows/skin/calendar.css os=WINNT osversion>=6
> > +% skin calendar-windows classic/1.0 chrome/skin/windows/calendar/win-classic/ os=WINNT osversion<6
> > +% skin calendar-windows classic/1.0 chrome/skin/windows/calendar/win-aero/ os=WINNT osversion>=6
> 
> Do you think it makes sense to move these skin registrations up to the place
> where the other skin registrations for Linux/Mac/Windows are done?

It works also at this position, but I moved then in my last patch to the other skin registrations. Then all are together.

> ::: calendar/lightning/themes/linux/imip.css
> @@ +1,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +@import url(chrome://calendar-common/skin/imip.css);
> 
> Shouldn't this be lightning-common (also in a few other places) ? Even
> though we only have one product in tree now, I don't think we should mix
> calendar/lightning just yet.

I've created now a lightning-common. There are now a common and a lightning-common directory in skin. I don't rename common to calendar-common for when we merge both together, okay?

I made a try run to check nothing breaks: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=57458737a743

The Try-build Lightning xpis can be found here: http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/richard.marti@gmail.com-57458737a743/
Attachment #8333985 - Attachment is obsolete: true
Attachment #8357324 - Flags: review+
Check-in: http://hg.mozilla.org/comm-central/rev/b55d96af36a4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.1
Fwiw on OpenBSD it seems this broke lightning display as noted in comment 3, tried with seamonkey 2.26b1 with bundled lightning 3.1. I'll file a followup to see how i can make non-tier1-unix-oses-that-are-not-mac use the linux skin.
Depends on: 1001985
You need to log in before you can comment on or make changes to this bug.