1612166 - Stop shipping all the calendar skin files on all platforms

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Paul Morris [:pmorris]]

A follow-up on bug 1608610. See comment 107 (https://bugzilla.mozilla.org/show_bug.cgi?id=1608610#c107).

Comment 1

[Paul Morris [:pmorris]]

Attachment: cal-skin-files-not-all-platforms-0.patch

To land after bug 1608610 lands. Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b698e41a8d8ac08754985b5e5f6d9de45776ac64

Comment 2

[Geoff Lankow (:darktrojan)]

I think I'd like this to match mail/themes and browser/themes: one JAR manifest in each OS directory, including a shared jar.inc.mn from the common directory.

Taking that thought a step further, I'd also like to get rid of chrome://calendar-common/skin/ namespace and have its contents under chrome://calendar/skin/shared/ as happens for mail and browser. (Ditto for lightning-common.)

If we're going to make such a change, now seems like the best opportunity we're going to get. Does that seem reasonable?

(Cc'ing some more people who might have an useful opinion.)

Comment 3

[Paul Morris [:pmorris]]

That all seems reasonable to me. Good to have consistency in how things are done.

Comment 4

[Richard Marti (:Paenglab)]

Yes, making this simpler would be great. Maybe we could also remove the chrome://lightning/skin namespace and use only chrome://calendar/skin.

Comment 5

[Paul Morris [:pmorris]]

Attachment: part1-rearrange-cal-skin-files-0.patch

1 of 3 - Only ship one platform's skin files in a given build and rearrange the skin build setup to match mail.

Comment 6

[Paul Morris [:pmorris]]

Attachment: part2-remove-calendar-common-name-0.patch

2 of 3 - Remove the calendar-common skin package name.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part3-move-lightning-skin-files-0.patch

3 of 3 - Merge skin files from lightning directory into base directory and drop the lightning-common package name.

Comment 8

[Paul Morris [:pmorris]]

Richard, do you know what the story is with these icon files?
https://searchfox.org/comm-central/source/calendar/base/jar.mn#130-132,134-135
They appear to be unused as the only place they appear in the code is here in this jar.mn file.

Comment 9

[Richard Marti (:Paenglab)]

(In reply to Paul Morris [:pmorris] from comment #8)

Richard, do you know what the story is with these icon files?
https://searchfox.org/comm-central/source/calendar/base/jar.mn#130-132,134-135
They appear to be unused as the only place they appear in the code is here in this jar.mn file.

They are to be shown on the top left of the windows under Linux. But actually I don't see a icon. So maybe we can remove them.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9125645 [details] [diff] [review]
part2-remove-calendar-common-name-0.patch

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

If removing calendar-common works that is great! I seem to recall we had some reason to do it, maybe it was just the ordering of the manifest directives that needs to be stable and with the extension we couldn't guarantee it. Maybe Paenglab remembers?

Comment 11

[Richard Marti (:Paenglab)]

It was because we packaged all platforms in one XPI (a on Linux built XPI worked on Windows or Mac too) and we needed dedicated shared paths to work on all platforms. And also it was because we had the calendar and the lightning path and to differentiate the two shared directories. Now with only one platform per build we can move the shared directory directly under calendar/skin/.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9125645 [details] [diff] [review]
part2-remove-calendar-common-name-0.patch

Yes, this looks good and makes it easier to find the correct files.

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 9125646 [details] [diff] [review]
part3-move-lightning-skin-files-0.patch

Thanks for all this work.

Comment 14

[Paul Morris [:pmorris]]

These patches are ready to land. (The reviewer part of the commit messages will need to be updated.)

They are to be shown on the top left of the windows under Linux. But actually I don't see a icon. So maybe we can remove them.

On these icons, I'll file a follow-up bug to either remove or use them.

Comment 15

[Paul Morris [:pmorris]]

Here's a try run, after rebasing on current trunk:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8df92de2beb8f670321286546596974bfa6908b4

Comment 16

[Paul Morris [:pmorris]]

Try run did not go well. Removing the checkin flag.
Update: failures were due to c-c bustage (see bug 1614718).

Comment 17

[Paul Morris [:pmorris]]

Attachment: part1B-remove-duplicate-icons-0.patch

New patch. Paths to some icon files in allowed-dupes.mn needed to be updated, and that caused try run failure. Upon closer inspection, these icon files could just be de-duped. (I must have overlooked them when de-duping the others because the duplicate versions had different names.) One of the files in allowed-dupes was not actually duped so I removed it.

Comment 18

[Paul Morris [:pmorris]]

Attachment: part2-remove-calendar-common-name-1.patch

Just rebased on top of the new part1B patch.

Comment 19

[Paul Morris [:pmorris]]

Attachment: part3-move-lightning-skin-files-1.patch

Just rebased on the new part1B patch.

Here's a try run I did for Linux only (along with patches for bug 1612190), just to check if it was all working: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5e8b6f154ea2a128ace263c6b3bd299c977e279c

Then I tried to do one for all platforms with just the patches for this bug, but that didn't work because the patches didn't change from the linux try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e3e74e24f59ea28c7b36962a2dc30b7c50307d00&selectedJob=288557198

I need to figure out how to trick it into doing the job. I could add a temporary bogus whitespace commit, but I imagine there must be a better way.

Comment 20

[Paul Morris [:pmorris]]

Modified the commit message to add the "r=" on the last patch to get this try run going:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=53efb80e5214f142ae31caa1f4516c7e2d0eb5a5

Comment 21

[Paul Morris [:pmorris]]

Whatever was going on with that try run in comment 20 seems to have cleared up now:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8c55df0e93d7a8c23e8cf932f8f1101577533287

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0dc699fa42c6
Stop shipping all calendar skin files to all platforms. r=Fallen
https://hg.mozilla.org/comm-central/rev/171779b184e0
Remove duplicated calendar icons. r=darktrojan
https://hg.mozilla.org/comm-central/rev/51f53932665f
Remove 'chrome://calendar-common/skin/' path. r=Fallen
https://hg.mozilla.org/comm-central/rev/3d6cb3e7f574
Move calendar skin files in 'lightning' directory into 'base' directory. r=Fallen,Paenglab