1344138 - TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js - TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

First seen: Fri Mar 3, 2017, 1:15:51
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e32e7215e0e726eeef616940d4e57b3db70755e6

https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1488500198/comm-central_win7_ix_test-xpcshell-bm110-tests1-windows-build2.txt.gz

TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js | xpcshell return code: 0

Caused by bug 1330349, specifically:
https://hg.mozilla.org/mozilla-central/rev/2c581e1a3c44

Mike, can you please advise us. Usually we just disable all failing tests related to web extensions, for example:
https://dxr.mozilla.org/mozilla-central/search?q=appname+%3D%3D+thunderbird&redirect=false
(last few in the results).

But sometimes, it's just a missing preference that would make the test pass, I noticed that you removed:
-// Whether or not webextension themes are supported.
-pref("extensions.webextensions.themes.enabled", false);
but we don't have that preference.

Comment 1

[Jorg K (CEST = GMT+2)]

Aleth, could you please take a look here.

Comment 2

[aleth [:aleth]]

*If* there are no plans for Thunderbird to support the new webextensions-based theming API, then we should just disable these tests, otherwise it makes sense to add the pref. (In a sense it seems unfortunate there's no extensions.webextensions.enabled pref instead of disabling the tests by hand.)

Bug 1330349 says: "LightWeight Themes work on Fennec, Thunderbird and Seamonkey too. So providing the API to these applications too will be useful at some point. WebExtensions will end up being supported in one or more of these products some day. But that's not the reason why I'm moving it to toolkit/ now. (...)"

Paenglab is best informed about theming and to decide whether this new API will work for TB in principle.

Comment 3

[Jorg K (CEST = GMT+2)]

But which pref would we need to add? |pref("extensions.webextensions.themes.enabled", true);| ? I tried that and
  mozilla/mach xpcshell-test toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js
still fails.

Comment 4

[Richard Marti (:Paenglab)]

TB should support webextensions-based theming API to still be able to customize the theming when the old themes are no more supported.

extensions.webextensions.themes.enabled is in all.js and also for FX disabled. Actually I don't know how such a webextension theme has to look (only json file or packed in a xpi etc.). I don't know if we need to add some code to support this themes or everything is in toolkit. Maybe Mike de Boer could help.

Comment 5

[Mike de Boer [:mikedeboer]]

Well, the new Theming API lives in toolkit since bug 1330349 with the intention to have it available to other Gecko apps later on. That also means that I'd be more than willing to support this operation for TB - please feel free to reach out and have a call about what's possible.

I know that Complete Themes won't be available in a while, but that's only on Firefox. TB shouldn't be affected, thus Complete Themes for TB should still continue to work, just like XUL addons will.

WRT the failing test: you can disable to test for TB in its entirety, but that'll mean you'll lose coverage. You can also make the test bail out based on `AppConstants.MOZ_APP_NAME`, using AppConstants.jsm.

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks for your answer, Mike. So somehow the consensus seems to be that want the new Theming API for TB as well and that we shouldn't switch of the test. BTW,
> you can disable to test for TB in its entirety, but that'll mean you'll lose coverage.
isn't quite accurate of the "you" refers to the TB team. It's an M-C Xpcshell test which would need to be switched off in some .ini file in M-C. So you can switch it off, or we can send a patch to switch it off which you would approve.

> You can also make the test bail out based on `AppConstants.MOZ_APP_NAME`, using AppConstants.jsm.
That part I don't understand. AppConstants.jsm is also an M-C file.

Let's assume we want to make the test work. Any insights why it currently fails?

If you look for 
  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js
in https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1488500198/comm-central_win7_ix_test-xpcshell-bm110-tests1-windows-build2.txt.gz you might be able to spot something I can't see.

Comment 7

[Jorg K (CEST = GMT+2)]

Aleth, any insights into how we could get the test to work? Also, NI'ing Mike re. previous comment.

Comment 8

[aleth [:aleth]]

Locally it fails with

addons.xpi	WARN	Invalid XPI: Error: Cannot find id for addon /private/var/folders/fs/tfzpcz615s98r47_s2txlhj00000gp/T/TemporaryItems/generated-extension.xpi (resource://gre/modules/addons/XPIProvider.jsm:5571:19)
ERROR Unexpected exception Error: Expected file to be downloaded for install of /private/var/folders/fs/tfzpcz615s98r47_s2txlhj00000gp/T/TemporaryItems/generated-extension.xpi at resource://testing-common/AddonTestUtils.jsm:1059

As the new theme addons are a special case of webextensions, I'd start by reenabling the disabled webextensions tests locally and adding whatever prefs are missing to make them pass. Maybe that will be enough, maybe not.

Comment 9

[Richard Marti (:Paenglab)]

I can imagine that the now landed bug 1343921 introduces new errors.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #8)
> As the new theme addons are a special case of webextensions, I'd start by
> reenabling the disabled webextensions tests locally and adding whatever
> prefs are missing to make them pass. Maybe that will be enough, maybe not.
I think we disabled those webextension tests for a reason, it weren't just missing preferences which made them fail, see bug 1267612, bug 1293992 and bug 1296883 for some tests I disabled.
A little tough to get all these working.

Comment 11

[Mike de Boer [:mikedeboer]]

(In reply to Jorg K (GMT+1) from comment #6)
> Thanks for your answer, Mike. So somehow the consensus seems to be that want
> the new Theming API for TB as well and that we shouldn't switch of the test.

That's cool! I'm available to talk & help out regarding this integration, but perhaps we'll also need help from #teamaddons. At the moment I don't have a good handle on the scope of this work, but I'd like to know.

> BTW,
> > you can disable to test for TB in its entirety, but that'll mean you'll lose coverage.
> isn't quite accurate of the "you" refers to the TB team. It's an M-C
> Xpcshell test which would need to be switched off in some .ini file in M-C.
> So you can switch it off, or we can send a patch to switch it off which you
> would approve.

Well, I'm fine with either. It's adding 'skip-if = appname == "thunderbird"' to <m-c>/toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini, right?

> > You can also make the test bail out based on `AppConstants.MOZ_APP_NAME`, using AppConstants.jsm.
> That part I don't understand. AppConstants.jsm is also an M-C file.

Well, yes, but TB consumes <m-c>/toolkit JSMs regularly, no?

> Let's assume we want to make the test work. Any insights why it currently
> fails?

Well, it added tests for the new WebExtension-based theming API for Firefox and since these are not supported in Thunderbird I think it therefore fails to complete the test.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1344138-skip-test_undothemeuninstall.patch

A little hard to tell where TB is going re. web extensions. So let's disable the test for now until someone comes up with an integrated approach. As I said in comment #10, this is not the first test we disable.

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8844913 [details] [diff] [review]
1344138-skip-test_undothemeuninstall.patch

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

Thanks!

Comment 14

[Richard Marti (:Paenglab)]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Well, it added tests for the new WebExtension-based theming API for Firefox
> and since these are not supported in Thunderbird I think it therefore fails
> to complete the test.

What would be needed to support the WebExtension-based theming API? I tried a WebExtension theme in TB (through about:debugging) and it showed the text color and also the background image. The only thing I see is after applying the WebExtension, opening a Composer or Addressbook window doesn't get the theme applied. But this could be a limitation of applying through about:debugging because also FX doesn't apply the theme on later opened windows. It applies the theme only on windows which are open when using the theme.

I'm planning in the next days to port bug 1343921 to TB. Then we will see if this also works. But would then be something missing?

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Richard Marti (:Paenglab) from comment #14)
> What would be needed to support the WebExtension-based theming API? I tried
> a WebExtension theme in TB (through about:debugging) and it showed the text
> color and also the background image. The only thing I see is after applying
> the WebExtension, opening a Composer or Addressbook window doesn't get the
> theme applied. But this could be a limitation of applying through
> about:debugging because also FX doesn't apply the theme on later opened
> windows. It applies the theme only on windows which are open when using the
> theme.

Well, that's already super promising! :) Then it might be a matter of infrastructure to support installing WebExtension-type addons through AMO on TB?

> I'm planning in the next days to port bug 1343921 to TB. Then we will see if
> this also works. But would then be something missing?

Cool. If everything 'just works' already, no.

The idea behind the themeing API is extending LWT - so we're going to introduce new CSS variables that will be set through the API and these values will override the currently active default theme style. The Thunderbird theme would need to implement support for these CSS variables in their own theme and things will work.

At the moment we have all this disabled for Firefox as well. Please follow bug 1341722 to know when things are stable enough for browser, because that should be a good integration time for TB, I think.

Enabling the custom icons API depends on a performance investigation we still need to perform wrt SVG resources.

Comment 16

[Jorg K (CEST = GMT+2)]

Aleth, could you please get this landed for me. I hope you don't feel strongly against it. Maybe we should open a bug "Make web extension (or web extension themes) work in TB". Then we can see whether we can get this test and all the other ones we disabled so far going again.

Comment 17

[aleth [:aleth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1aad1aa023e4682368d9b15a121d37d5b5a5c1
Bug 1344138 - Skip test_undothemeuninstall.js for Thunderbird. r=mikedeboer

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d1aad1aa023

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8844913 [details] [diff] [review]
1344138-skip-test_undothemeuninstall.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330349.
[Why is the change risky/not risky?]: No risky, only disabling test for Thunderbird.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8844913 [details] [diff] [review]
1344138-skip-test_undothemeuninstall.patch

Disable test for Thunderbird. Aurora54+.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dfda7df6b72e