1456225 - Port bug 1372694 part 1 to TB: Stop making the default theme a heavyweight theme

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1372694 plans to remove the heavy weight theme support. The default theme is now a heavy weight too. They change this in bug 1372694. We need to follow to not end in a not working TB.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: No-HW-default-theme.patch

I tried this together with the patch from bug 1372694 and it worked. :-)

Jörg, bug 1372694 has r+ on both patches and it can land soon. If Philipp hasn't reviewed it, when the m-c patches land, can you do it to not break TB?

Philipp, please can you check if all is correct for the build? This will affect Lightning too as the extension ID {972ce4c6-7e08-4474-a285-3208198ce6fd} will be removed and Lightning has a lot of references to it.

Comment 2

[Jorg K (CEST = GMT+2)]

I'll look at this when bug 1372694 goes to inbound. You meant to say "... together with the patch*es* from bug 1372694 ...", right?

Comment 3

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8830ae22166b78356ec22a83b83869c6bafc5fa1
build on
https://hg.mozilla.org/try-comm-central/rev/d15c8e51bc0fd1ede7525da549ceadc0f28773dd
https://hg.mozilla.org/try/rev/49dc98b3f8a5842cbb870bd3193bebfdab340874

Comment 4

[Jorg K (CEST = GMT+2)]

This works locally but on the server, the packaging fails with:

[task 2018-04-24T07:20:11.928Z] 07:20:11     INFO -  package> Error: /builds/worker/workspace/build/src/comm/mail/installer/package-manifest.in:161: Missing file(s): Thunderbird DailyDebug.app/Contents/Resources/defaults/profile/mimeTypes.rdf

That's because you remove that file here:
https://hg.mozilla.org/try-comm-central/rev/d15c8e51bc0fd1ede7525da549ceadc0f28773dd#l4.11

We did something related to mimetypes in bug 1446313 but never followed up removing that file completely from C-C. I filed bug 1456405 for that.

Let's try again removing that file from packaging as well:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f9d4fe7a0ba43f1cc90ea67014a71839ab2e33b

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #4)

> Let's try again removing that file from packaging as well:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=3f9d4fe7a0ba43f1cc90ea67014a71839ab2e33b

If this doesn't work, I have a patch that doesn't removes it.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: No-HW-default-theme.patch

This includes the removal of mimeTypes.rdf from the package manifest. Further clean-up in bug 1456405.

Sadly there is a test failure:
mozmill/content-tabs/test-lwthemes.js | test-lwthemes.js::test_lightweight_themes_install

INFO -  SUMMARY-UNEXPECTED-FAIL | test-lwthemes.js | test-lwthemes.js::test_lightweight_themes_install
INFO -    EXCEPTION: Lightweight theme selected when there should not have been.
INFO -      at: test-lwthemes.js line 130
INFO -         test_lightweight_themes_install test-lwthemes.js:130 11

It looks like the test asserts that the initial theme is not an LW theme, but since heavy themes are gone, even the initial theme is LW. I'll fix it.

Comment 7

[Jorg K (CEST = GMT+2)]

Sorry, my fix attempt of

 function test_lightweight_themes_install() {
-  // Before we run the test, check we've not got a theme already installed.
-  if (currentLwTheme())
-    throw new Error("Lightweight theme selected when there should not have been.");
-

only solved the first failure, there was another afterwards:

  EXCEPTION: Lightweight theme installation was not undone
    at: test-lwthemes.js line 91
       install_theme test-lwthemes.js:91 11
       test_lightweight_themes_install test-lwthemes.js:131 3

Too many tests that no LW theme is installed :-(

I'm running out of time now, so we will have to live with the test failure for now.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7011cd9d8481
Port bug 1372694 part 1 to TB: Stop making the default theme a heavyweight theme. r=jorgk

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1456225-fix-test.patch

I got the test going, but three minutes too late :-(

Comment 10

[:aceman]

Comment on attachment 8970486 [details] [diff] [review]
1456225-fix-test.patch

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

Does the theme have a name that we could check for to see if the theme in the test replaced it?

Comment 11

[:aceman]

Comment on attachment 8970482 [details] [diff] [review]
No-HW-default-theme.patch

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

I'm not sure why the mimetypes.rdf is touched in this patch. It could have been made in the appropriate bug.

Comment 12

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #10)
> Comment on attachment 8970486 [details] [diff] [review]
> 1456225-fix-test.patch
> 
> Review of attachment 8970486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does the theme have a name that we could check for to see if the theme in
> the test replaced it?

There is a "[e-mail]". Maybe you can check this.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #11)
> I'm not sure why the mimetypes.rdf is touched in this patch. It could have
> been made in the appropriate bug.
Yes, I thought mail/app/profile/moz.build was removed entirely, but that's not the case:
https://hg.mozilla.org/comm-central/rev/7011cd9d8481#l4.2

Well, mistakes happen when you have 10 minutes to avoid bustage and get the Daily to run. Anyway, the removal is correct, it just happened in the wrong bug.

Comment 14

[:aceman]

Attachment: 1456225.patch fix test v2

This works for me. I also enabled the skipped test_lightweight_themes_install_and_undo test function, it seems to pass for me now (maybe switching LW themes around is more reliable than from normal theme to LW one).
It works for me on Linux, try run here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a176802b4c68b46bfdcd6693ac4e285ef2f6f055

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8970725 [details] [diff] [review]
1456225.patch fix test v2

Looks good (incl. boy-scout clean-up) and works for me on Windows. Too late for feedback and I'm not sure I'll wait for the try either. I cancelled the Windows one.

THANKS!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/11a2c23125c1
adjust test-lwthemes.js for the default theme being lightweight too. r=jorgk