Closed Bug 1612166 Opened 2 months ago Closed 2 months ago

Stop shipping all the calendar skin files on all platforms

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 75.0

People

(Reporter: pmorris, Assigned: pmorris)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

Depends on: 1608610

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.)

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

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

Attachment #9123886 - Flags: review?(geoff)

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

Attachment #9123886 - Attachment is obsolete: true
Attachment #9125644 - Flags: review?(geoff)

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

Attachment #9125645 - Flags: review?(geoff)

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

Attachment #9125646 - Flags: review?(richard.marti)
Attachment #9125646 - Flags: review?(geoff)

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.

Flags: needinfo?(richard.marti)

(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.

Flags: needinfo?(richard.marti)
Attachment #9125644 - Flags: review?(geoff) → review+
Attachment #9125646 - Flags: review?(geoff) → review+
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?
Attachment #9125645 - Flags: review?(geoff)
Attachment #9125645 - Flags: review+
Attachment #9125645 - Flags: feedback?(richard.marti)

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 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.
Attachment #9125645 - Flags: feedback?(richard.marti) → feedback+
Comment on attachment 9125646 [details] [diff] [review]
part3-move-lightning-skin-files-0.patch

Thanks for all this work.
Attachment #9125646 - Flags: review?(richard.marti) → review+

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.

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

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.

Attachment #9126155 - Flags: review?(geoff)

Just rebased on top of the new part1B patch.

Attachment #9125645 - Attachment is obsolete: true
Attachment #9126156 - Flags: review+

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.

Attachment #9125646 - Attachment is obsolete: true
Attachment #9126157 - Flags: review+

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

Attachment #9126155 - Flags: review?(geoff) → review+
Status: NEW → ASSIGNED

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 75.0
You need to log in before you can comment on or make changes to this bug.