Comment on attachment 8830757 [details] Bug 1330349 - Part 1 - move the new theme WebExtension API to the toolkit collection of APIs. https://reviewboard.mozilla.org/r/107484/#review108760 Why is this code moving to toolkit? If it works on fennec and other applications, then it shouldn't be tested with a browser mochitest, or if it is desktop only it should stay in browser/

Comment on attachment 8830757 [details] Bug 1330349 - Part 1 - move the new theme WebExtension API to the toolkit collection of APIs. Well, I'm certainly not doing it on a whim ;-) 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. I'm moving it, because I'm introducing a new webextension type, called 'webextension-theme' in the next patch - part deux - to make the necessary mapping to the 'theme' external type. This change seemed to me the most contained, least intrusive and least amount of LOC changed to accomplish our goals here. Patch number three adds all kinds unit tests to make sure things continue to function as they do now and the new type also works as expected under various circumstances. But to correctly test anything, not having the code in toolkit/ complicates things considerably. Therefore I opted to move it to toolkit/ now and make the integration solid. I hope I explained things thoroughly enough here, but please feel free to reach to me here or IRC if you have any further questions!

(In reply to Andrew Swan [:aswan] from comment #5) > If it works on fennec and other applications, then it > shouldn't be tested with a browser mochitest This I think needs a separate comment, because the division of toolkit/ vs. browser/ doesn't dictate this. We have browser mochitests in various places in toolkit/ where we need an integration test for features where we benefit from having a tabbed browser interface and various utilities that make writing integration tests easier. Much of the code we write in toolkit is targeted at the browser as primary consumer and a relatively complex one at that, thus it makes sense to choose for a browser-chrome test for integration testing. This is not meant demeaning in any way, but merely thinking in terms of maintenance cost and developer productivity. If, in any way, we can write a mochitest-plain test to cover generic functionality/ parts of the API, we should and will. But we're taking a pragmatic stance regarding these choices.

Comment on attachment 8830758 [details] Bug 1330349 - Part 2 - install and present theme type WebExtensions as themes in the Addon Manager. https://reviewboard.mozilla.org/r/107486/#review109012 I'm going to defer to rhelmer on this but I think we probably want to find a way not to use a new internal type and instead make getExternalType be a bit smarter in deciding to return "theme" for these things. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5545 (Diff revision 1) > zipreader.close(); > return Promise.reject([AddonManager.ERROR_INCORRECT_ID, > `Refusing to upgrade addon ${this.existingAddon.id} to different ID {this.addon.id}`]); > } > > - if (this.existingAddon.type == "webextension" && this.addon.type != "webextension") { > + if (isWebExtension(this.existingAddon.type) && isWebExtension(this.addon.type)) { The second clause should be inverted.

Comment on attachment 8830759 [details] Bug 1330349 - Part 3 - make sure only one theme may be selected at the same time. https://reviewboard.mozilla.org/r/107488/#review109082 This looks fine. I'm not convinced that we need the duplicated tests that you've done but I'm not going to argua against redundant tests too much.

Mike, I read the google doc you linked on IRC earlier ("The Future of Firefox Theming") but I still have some questions. The entire "story" for this bug is: As a user I’d like new themes to be made selectable from the AddonManager in the same way as LWTs currently are. But if that's the desired behavior, I think we have a lot of work to do in addition to the patches that are currently on this bug. I'm not clear how this is meant to all work, if I write a webextension that has a populated theme property in the manifest but also has other webextension elements (ie, a background page, browser action, options page, etc) what does this extension look like to a user? Does it show up in the "Extensions" list in about:addons? Does it also show up in the "Appearance" list? If my webextension theme is active and I select a different theme, does the rest of my extension continue to run? I think there are probably many more questions that will have to be addressed about more specific scenarios but lets start with those questions.

This was talked about in Hawaii, so things may have changed since then, but I recall us making a distinction between simple themes - ones that only contain the manifest theme property - and ones that contain other webextension elements. This bug would only deal with simple themes, while others would be treated as webextensions. Mike, please correct me if things have changed :)

We've discussed this in IRC for a bit now - I think given comment 14 and the goal of making this a replacement for lightweight themes, and wanting to appear in the right places on AMO and about:addons, having this be a new type that maps internally to the existing "theme" type makes sense, we just need to make sure not to break old themes until post-2017 at the earliest (which means you might have to deal with or workaround any existing bugs) I think there are some unresolved questions such as comment 13 we will need to figure out once we're past this stage - the one I am most interested in is: can you have a WebExtension that contains both static theme elements in the manifest and *also* loads script right now? If so, should that be considered a "theme" or an "extension"? I think we don't want to have the same WebExtension to show up in multiple places (like both in Appearance and also Extensions), and be able to enable/disable these independently. This seems confusing for the user and not consistent with how it's worked in the past with heavy/light themes - extension authors should choose whether this should be a "theme" or an "extension". We don't need to actually implement all of this right now of course but it'd be good to have a plan. One idea we discussed in IRC is that a new "WebExtension theme" type should be allowed to load scripts, but should have a set of permissions that can be requested that are appropriate for themes, but reduce the risk of things like data exfiltration.

(In reply to Robert Helmer [:rhelmer] from comment #15) > We've discussed this in IRC for a bit now - I think given comment 14 and the > goal of making this a replacement for lightweight themes, and wanting to > appear in the right places on AMO and about:addons, having this be a new > type that maps internally to the existing "theme" type makes sense, we just > need to make sure not to break old themes until post-2017 at the earliest > (which means you might have to deal with or workaround any existing bugs) Thanks. Totally prepared to deal with that when it hits us. > I think there are some unresolved questions such as comment 13 we will need > to figure out once we're past this stage - the one I am most interested in > is: can you have a WebExtension that contains both static theme elements in > the manifest and *also* loads script right now? If so, should that be > considered a "theme" or an "extension"? Nope. That's out of scope entirely for this time. We believe that making a strict separation between manifest-only themes and WebExtensions that may do stuff using an API namespaced 'theme' will result in an implementation that needs to deal with fewer edge cases. > I think we don't want to have the same WebExtension to show up in multiple > places (like both in Appearance and also Extensions), and be able to > enable/disable these independently. This seems confusing for the user and > not consistent with how it's worked in the past with heavy/light themes - > extension authors should choose whether this should be a "theme" or an > "extension". Yup, and that's what these patches do in fact. > We don't need to actually implement all of this right now of course but it'd > be good to have a plan. One idea we discussed in IRC is that a new > "WebExtension theme" type should be allowed to load scripts, but should have > a set of permissions that can be requested that are appropriate for themes, > but reduce the risk of things like data exfiltration. I agree. Now I'll need to file and work on a couple of follow-up bugs, depending on this one, to hammer down the manifest-only theme; it won't be allowed to have any other API clauses or 'scripts' defined in the manifest. Just a 'theme' clause and packaged resources may be defined, if you reason the other way around. I agree with earlier remarks that this work should live in the parser and will throw errors when an invalid manifest is encountered. I also added all this in the Engineering Plan - https://docs.google.com/document/d/1ueD6V7aLLTuc1GAOxxQYcwl2HR-k62HHu3q8knTJ4FU

Given the design that has been laid out for themes, I don't think the current implementation makes sense. Instantiating an Extension is total overkill for a lightweight theme. Using ExtensionData for manifest parsing is fine (though I would like to see us be much more strict about an individual xpi being either an extension or theme, and refecting xpis that have elements of both). But for actually applying themes, I think it would make more sense to create a new internal bootstrap module a la WebExtensionBootstrap.js and invoke that from XPIProvider.jsm for themes, leaving the full-blown webextensions mechanisms out of the picture entirely.

(In reply to Andrew Swan [:aswan] from comment #17) > Given the design that has been laid out for themes, I don't think the > current implementation makes sense. Instantiating an Extension is total > overkill for a lightweight theme. Using ExtensionData for manifest parsing > is fine (though I would like to see us be much more strict about an > individual xpi being either an extension or theme, and refecting xpis that > have elements of both). But for actually applying themes, I think it would > make more sense to create a new internal bootstrap module a la > WebExtensionBootstrap.js and invoke that from XPIProvider.jsm for themes, > leaving the full-blown webextensions mechanisms out of the picture entirely. I'm not sure about either of these, because I (apparently) lack the appropriate amount of inside knowledge to be sure. Two of the things that strike me as peculiar are the statement that instantiating an extension is overkill - for an LWT or not, if it's overkill I'd like to re-review its architecture because they appeared to me as rather lean and slim - and generally I'd like to keep the amount of branching in any architecture to a minimum to avoid complexity and steep learning curve for a next generation - even at the cost of a little overhead. All in all, given the feedback in this bug, I think it's warranted to get together early next week to come up with a plan we all agree on or even go back to the drawing board if necessary.

(In reply to Andrew Swan [:aswan] from comment #17) > Given the design that has been laid out for themes, I don't think the > current implementation makes sense. Instantiating an Extension is total > overkill for a lightweight theme. Also, this might be out of scope for this bug. Perhaps it's better to move this discussion to another bug and decide whether to refactor our current implementation there? A refactor at this point wouldn't hurt us much - the footprint is still very small.

I'd really like this to move forward now. What do we need to happen before these patches can be reviewed?

Perhaps I wasn't very clear, my concern isn't that instantiating an Extension object is too expensive by some measure, this is about sound engineering practice. Static themes have nothing in common with webextensions beyond some superficial packaging similarities. I'm all for sharing the code that handles that (that's the ExtensionData remark) but lets not couple the implementations together any further than that. Doing so will only make the shared code harder to maintain and evolve in the future. As for moving forward, I guess I would like to see a more spelled-out plan for getting this feature finished. Since, as you say, the code is small enough that a refactor isn't much work at this point, why not do it now instead of landing this and planning to re-do it in a follow-up?

(In reply to Andrew Swan [:aswan] from comment #21) > Perhaps I wasn't very clear, my concern isn't that instantiating an > Extension object is too expensive by some measure, this is about sound > engineering practice. Static themes have nothing in common with > webextensions beyond some superficial packaging similarities. I'm all for > sharing the code that handles that (that's the ExtensionData remark) but > lets not couple the implementations together any further than that. Doing > so will only make the shared code harder to maintain and evolve in the > future. True, but I'd like to not shut the door early to a hybrid approach, where themes _can_ in fact run background scripts too, which is not scheduled for v1 but might be later. > As for moving forward, I guess I would like to see a more spelled-out plan > for getting this feature finished. Since, as you say, the code is small > enough that a refactor isn't much work at this point, why not do it now > instead of landing this and planning to re-do it in a follow-up? 1. In this bug, I'm merely moving the current implementation to toolkit/, not (re-)implementing it. Would Addon Manager integration be wildly different when we make it use ExtensionData only? 2. Why would you propose to make that block this bug? We'd like to move forward with building out the framework so that the whole team can make progress - and not be blocked by implementation details such as this. 3. AFAIK the implementation of the framework/ theming API itself is orthogonal to Addon Manager integration. I vote for moving this discussion and potential work to a more appropriate location.

I'm not sure I grasp exactly what is going on here. We have a rather detailed engineering plan at https://docs.google.com/document/d/1ueD6V7aLLTuc1GAOxxQYcwl2HR-k62HHu3q8knTJ4FU/edit?pli=1# and during Hawaii and other venues we've requested feedback all.the.time. If we can not make progress here at the very start, I'm not really sure if the theming API is in fact any kind of priority at the moment. What I need is detailed feedback that explains why a certain design decision is not sound and why another is better/ superior. Like I said earlier with different words, I'm open to adopting _any_ design. Technical stuff usually isn't the problem, proper communication is. At the moment you could say I'm having a problem with multi-day feedback loops as well.

(In reply to Mike de Boer [:mikedeboer] from comment #23) > What I need is detailed feedback that explains why a certain design decision > is not sound and why another is better/ superior. I'm not sure what you're looking for beyond comment 21, the feedback wasn't "this will break for this reason", it was "this is less maintainable in the long run". Regarding your concern about "shut(ting) the door" in comment 22, I don't think that handling themes with custom bootstrap instead of with Extension instances would prevent us from making any changes in the future. I'm unclear whether you're referring to extensions that use the dynamic themeing api or some type of hybrid that combines elements of the two. Assuming its the latter, we'll have many design decisions to make at that point (beginning with designing the user experience) and I don't think it is safe to assume that the implementation in your current patches will be the way it gets implemented. And sorry for the vague wording about "plans", let me be more specific: if you want to land this as-is now, please file follow-up bugs to fix the problems that have already been identified. Though to be frank, it seems easier to just do it in a way we can all agree on now.

Comment on attachment 8830758 [details] Bug 1330349 - Part 2 - install and present theme type WebExtensions as themes in the Addon Manager. https://reviewboard.mozilla.org/r/107486/#review112168 So two things here. First I agree with Andrew that having a separate bootstrap process for webextension-themes is ultimately the right way to go. It protects us from accidentally allowing them to do more than we want and keeps things as slim as possible. I don't think that that needs to happen here though so lets do it as a follow-up bug. Second this patch doesn't get us to where we want with only being able to activate one theme at a time. I think it is important to do this from the start because it is difficult work to do and it has the possibility of changing the approach you use here to make it work. One thing I think you should consider is not using a different internal type and instead use a different flag to control how we run these, using type="theme" and bootstrap="true" might be enough. But either way you are going to need to vet everywhere where we do something special for themes and make sure we're doing the right things. We're also going to need tests that demonstrate that switching back and forth between the default, lightweight and webextension themes work correctly and send out the right events to addon listeners.

I was hoping to do that in bug 1330347? I mean, it seemed like a logical split-off to me, because of the very different tests I'd need to work on. Whilst I was waiting here, I already did some of the research, thinking about the most practical way to implement the 'only one theme can be selected at the same time' scenario.

(In reply to Mike de Boer [:mikedeboer] from comment #26) > I was hoping to do that in bug 1330347? I mean, it seemed like a logical > split-off to me, because of the very different tests I'd need to work on. I disagree because as I say I think how you handle the theme switching will inform how we add these as a type and until I either look very deep myself or see the code you come up with I'm not convinced that this is the correct way to do it. Basically I'm saying that it will be easier for me to review in one patch than doing these two parts piecemeal. This patch also leaves us in a place where testers can start playing around with these themes, I don't want people to see it working one way and it then changed to another. There's also the potential for bugs to arise for such testers if we don't disable all their webextension-themes when switching to the correct model. > Whilst I was waiting here, I already did some of the research, thinking > about the most practical way to implement the 'only one theme can be > selected at the same time' scenario. Great! I look forward to seeing it.

Cool! I'm on PTO tomorrow, but expect it on Friday :)

Comment on attachment 8830757 [details] Bug 1330349 - Part 1 - move the new theme WebExtension API to the toolkit collection of APIs. https://reviewboard.mozilla.org/r/107484/#review112576 I'm fine with this route, though now that both Dave and Andrew are saying to evaluate the bootstrap route first then we should go that route, or at least attempt implementing it that way to see what the difference is. With the current patch, a couple things need to be done still. The 'extensions.webextensions.themes.enabled' pref needs to be moved from firefox.js to all.js, and we would need to implement a way for applications to opt-in to supporting the theme. Right now that would be as simple as placing 'extensions.webextensions.themes.enabled' in firefox.js (or similar application-specific pref file), but in the future it will also mean that the application will need to provide the schema as well as schema->CSS selector mappings.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #29) > Comment on attachment 8830757 [details] > Bug 1330349 - Part 1 - move the new theme WebExtension API to the toolkit > collection of APIs. > > https://reviewboard.mozilla.org/r/107484/#review112576 > > I'm fine with this route, though now that both Dave and Andrew are saying to > evaluate the bootstrap route first then we should go that route, or at least > attempt implementing it that way to see what the difference is. To expand why I'm fine with this route, it appears that some of the feedback as to why this route is bad deals with the presumption that WebExtensions are somewhat heavy-weight and since themes don't use other aspects of WebExtensions then they shouldn't need all this baggage. I like pushing with this current route because I'm not sure we need all WebExtensions to carry this baggage anyways. If themes and WebExtensions share a common foundation, then both will benefit from optimizations to the foundation. For example, some current WebExtensions may lack a content script, but bug 1337160 got duped to a bug that only mentions content script overhead.

Comment on attachment 8830759 [details] Bug 1330349 - Part 3 - make sure only one theme may be selected at the same time. https://reviewboard.mozilla.org/r/107488/#review114120

Comment on attachment 8837548 [details] Bug 1330349 - Part 4 - make sure theme type WebExtension support is covered by the existing test suite. https://reviewboard.mozilla.org/r/112686/#review114124 Carrying over r=Mossop

Dear me, reviewboard is not agreeing with me today. Alright, so, apart from the bugspam - I wanted to add the remark that there is no test coverage for Part 3 yet - I wanted to verify my approach with Dave T. first before I go spend a lot of time there. So please consider the r? as an f? for Part 3. (The additional tests will be a Part 5, yesyes.)

Comment on attachment 8830759 [details] Bug 1330349 - Part 3 - make sure only one theme may be selected at the same time. https://reviewboard.mozilla.org/r/107488/#review114310 The approach seems fine but I did spot an error in behaviour in testing. I installed a theme and when looking in the add-ons manager the default theme still showed as enabled. Include that in your set of tests please.

Comment on attachment 8830758 [details] Bug 1330349 - Part 2 - install and present theme type WebExtensions as themes in the Addon Manager. https://reviewboard.mozilla.org/r/107486/#review114262 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:950 (Diff revision 5) > let uri = NetUtil.newURI("./", null, aUri); > > let extension = new ExtensionData(uri); > > let manifest = yield extension.readManifest(); > + let isTheme = !!manifest.theme; This is going to shadow the function from the next patch which confused me for a second, please rename it.

Comment on attachment 8837548 [details] Bug 1330349 - Part 4 - make sure theme type WebExtension support is covered by the existing test suite. https://reviewboard.mozilla.org/r/112686/#review114318

(In reply to Dave Townsend [:mossop] from comment #47) > The approach seems fine but I did spot an error in behaviour in testing. I > installed a theme and when looking in the add-ons manager the default theme > still showed as enabled. Include that in your set of tests please. Yeah, I see that too and seems like it's due to temporary addons setting the enabled flag by default, without calling notifyAddonChanged. I added that to the patch.

Comment on attachment 8830757 [details] Bug 1330349 - Part 1 - move the new theme WebExtension API to the toolkit collection of APIs. https://reviewboard.mozilla.org/r/107484/#review114688 The themese tests will presumably move again when new themes get implemented without instantiating Extension. This seems like unnecessary churn to me but if it unblocks folks in the short term then go ahead... ::: toolkit/components/extensions/jar.mn:10 (Diff revision 6) > + content/extensions/ext-c-backgroundPage.js > + content/extensions/ext-c-extension.js > +#ifndef ANDROID > + content/extensions/ext-c-identity.js > +#endif > + content/extensions/ext-c-runtime.js > + content/extensions/ext-c-storage.js > + content/extensions/ext-c-test.js Please don't mix these together with the other scripts, the convention is that `ext-c-*` are scripts that run in the child process. Alphabetizing within categories is a fine thing but lets keep the primary grouping by category.

(In reply to Andrew Swan [:aswan] from comment #55) > Please don't mix these together with the other scripts, the convention is > that `ext-c-*` are scripts that run in the child process. Alphabetizing > within categories is a fine thing but lets keep the primary grouping by > category. I can see that this grouping based on file-naming convention works, but the grouping _inside_ the jar manifest seems entirely arbitrary. I mean, when you sort it alphabetically like this, the grouping still persists. I'm saying this as a fresh contributor and also a bit with my toolkit-peer hat on: custom conventions have a cost. This specific case might seem like a trivial thing, but it already wasn't trivial enough to keep you from commenting on it. The more of these things you add to something - this is and will be an integral part of toolkit - the more you alienate (volunteer) contributors. Since we're part of the Mozilla project full-time, but fresh eyes on the WebExtensions code base, would it be a good idea to file bugs for all the things we notice that we feel are making our onboarding experience feel worse than it could be? After all, we'd rather spend more of our time working/ coding on cool new stuff instead of being stuck in reviewing all the things, right?

(In reply to Mike de Boer [:mikedeboer] from comment #56) > (In reply to Andrew Swan [:aswan] from comment #55) > > Please don't mix these together with the other scripts, the convention is > > that `ext-c-*` are scripts that run in the child process. Alphabetizing > > within categories is a fine thing but lets keep the primary grouping by > > category. > > I can see that this grouping based on file-naming convention works, but the > grouping _inside_ the jar manifest seems entirely arbitrary. I mean, when > you sort it alphabetically like this, the grouping still persists. I agree with Andrew here, but it wouldn't hurt to put a comment above the separate group stating that these are scripts that run in the child process.

Comment on attachment 8839967 [details] Bug 1330349 - Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. https://reviewboard.mozilla.org/r/114544/#review116074 This is a good start but there are some more things that need to be tested here. Please add tests for the case where a non-default full theme is active and the user switches to a webextension theme. ::: toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js:373 (Diff revision 1) > + Assert.ok(t1.isActive); > + Assert.ok(!t1.userDisabled); > + Assert.equal(t1.pendingOperations, AddonManager.PENDING_NONE); > + > + Assert.ok(d, "Addon should be there"); > + Assert.ok(d.isActive); This doesn't look correct, surely the default theme should be disabled if the new theme is active? ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:84 (Diff revision 1) > + let [ t1, t2, t3, d ] = yield promiseAddonsByIDs(THEME_IDS); > + Assert.ok(t1, "Theme addon should exist"); > + Assert.ok(t2, "Theme addon should exist"); > + Assert.ok(t3, "Theme addon should exist"); > + Assert.ok(d, "Theme addon should exist"); > + Nit: whitespace ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:117 (Diff revision 1) > + if (which != THEME_IDS[3]) > + expected[THEME_IDS[3]] = !disabled; > + > + // Set the state of the theme to change. > + let theme = yield promiseAddonByID(which); > + theme.userDisabled = disabled; I want to see verification that the right events are sent out to listeners when this happens.

(In reply to Dave Townsend [:mossop] from comment #63) > This is a good start but there are some more things that need to be tested > here. > > Please add tests for the case where a non-default full theme is active and > the user switches to a webextension theme. Sure! > This doesn't look correct, surely the default theme should be disabled if > the new theme is active? Quite, but I have to admit that I don't fully understand the difference between 'active' (true|false) vs. 'enabled|disabled'. When should which be flipped? Are they mutually exclusive? In other words: when I flip `userDisabled`, I get inconsistent results when testing `isActive` when considering them mutually exclusive just like 'enabled' and 'disabled are binary opposites. > I want to see verification that the right events are sent out to listeners > when this happens. Ah yes, of course.

(In reply to Mike de Boer [:mikedeboer] from comment #64) > (In reply to Dave Townsend [:mossop] from comment #63) > > This is a good start but there are some more things that need to be tested > > here. > > > > Please add tests for the case where a non-default full theme is active and > > the user switches to a webextension theme. > > Sure! > > > This doesn't look correct, surely the default theme should be disabled if > > the new theme is active? > > Quite, but I have to admit that I don't fully understand the difference > between 'active' (true|false) vs. 'enabled|disabled'. When should which be > flipped? Are they mutually exclusive? > In other words: when I flip `userDisabled`, I get inconsistent results when > testing `isActive` when considering them mutually exclusive just like > 'enabled' and 'disabled are binary opposites. isActive tells you whether an add-on is actually running. It should be true when the add-on is not disabled (userDisabled = false and the varying other ways to disable an add-on like blocklisted, incompatible etc. are all false). Flipping userDisabled should flip isActive, the only exception being when a restart is needed to complete the action in which case isActive stays as it is and pendingOperations will say a restart is needed to enable/disable. Put another way, isActive == !userDisabled as long as pendingOperations == 0. Switching from the default theme to a web extension theme or a lightweight theme and vice versa should require no restart so in those cases isActive should only be true for one theme, the one with userDisabled==false. There is a fudge going on though. When a webextension theme or lightweight theme is active technically we're still using the default full theme under the hood. We still say isActive==false for the default theme even though that is arguably not the case because then it makes the add-ons manager UI look right with only one theme being active at a time. Adding a different full theme into the mix makes things more complex. It takes a restart to switch from a non-default full theme to any other theme because under the hood we're switching the default theme back on (again not exposed through the API). Does that help answer your question?

(In reply to Dave Townsend [:mossop] from comment #65) > Does that help answer your question? Absolutely, thanks!

To resolve the issue in the uninstall test I had to add things to the Part 3 patch, to make sure that the logic as enforced in `XPIProvider.addonChanged` was applied upon install of any theme type, using any install method. (At first I only fixed it this way for temp addon installs, but that turned out to be naive.)

Comment on attachment 8839967 [details] Bug 1330349 - Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. https://reviewboard.mozilla.org/r/114544/#review116856 ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:114 (Diff revisions 1 - 2) > * switching works as expected by checking the state of all installed themes. > * > * @param {String} which ID of the addon to set the `userDisabled` property on > * @param {Boolean} disabled Flag value to switch to > */ > function* setDisabledStateAndCheck(which, disabled = false) { Add a sanity check at the top here: if (disabled) { Assert.equal(which, gActiveTheme, "Only the active theme can be disabled"); } ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:150 (Diff revisions 1 - 2) > + // of the flow for `XPIProvider.enableDefaultTheme()`. > + let restartState = !!REQUIRE_RESTART[gActiveTheme] || (expectRestart && gActiveTheme == DEFAULT_THEME); > + expectedEvents[gActiveTheme] = [ [ "onDisabling", restartState ] ]; > + if (!restartState) > + expectedEvents[gActiveTheme].push([ "onDisabled", false ]); > + } I've been reading through this a lot and still having trouble seeing if it is doing the right thing. I think there is a better way to write this so it is easier to understand. Fundamentally you're always going to see one theme disable and another theme enable because there must always be one enabled at a time. So, up front figure out which is getting disabled and which enabled: let themeToDisable = disabled ? which : gActiveTheme; let themeToEnable = disabled ? DEFAULT_THEME : which; let expectRestart = REQUIRE_RESTART[themeToDisable] || REQUIRE_RESTART[themeToEnable]; expectedStates[themeToDisable] = true; expectedEvents[themeToDisable] = ... expectedStates[themeToEnable] = false; expectedEvents[themeToEnable] = ... See if something like that works and makes more sense here.

Comment on attachment 8839967 [details] Bug 1330349 - Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. https://reviewboard.mozilla.org/r/114544/#review117168 ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:133 (Diff revisions 1 - 3) > - expected[THEME_IDS[3]] = !disabled; > + // the events fired for the default theme will get the 'restart required' flag > + // as well. This has to do with the fact that the default theme is also a > + // complete theme, which are the ones to require a restart, normally. See > + // XPIProvider::enableRequiresRestart() for more info. > + let restartToDisable = !!REQUIRE_RESTART[themeToDisable] || (expectRestart && themeToDisable == DEFAULT_THEME); > + let restartToEnable = !!REQUIRE_RESTART[themeToEnable] || (expectRestart && themeToEnable == DEFAULT_THEME); Something is wrong here. If it takes a restart to enable a new theme then it must take a restart to disable the old theme. I added an assert here to verify that and it fails so there is a bug in the XPIProvider code. I suspect you'll need to patch enableRequiresRestart etc. Please add tests for the operationsRequiringRestart property of the themes that are being enabled/disabled to check that they are correct before making the change. Also test pendingOperations after the change but before the restart.

Well, I think you also noticed that I needed to add this exception for the case when you switch from default theme to theme1, which is a complete theme that requires a restart. This means that this is an 'ancient' code path that I haven't introduced in any of my patches here. That doesn't mean I won't look into it and fix it - it's just that I'm not sure if it's worth the investment? IOW: I needed to code the default theme exceptions _only_ for the test_dss() complete theme test. The others run fine without it. I see now, btw, that I forgot to mention that this is only about `XPIProvider::enableRequiresRestart()` called from the flow started from `XPIProvider::enableDefaultTheme()`.

(In reply to Mike de Boer [:mikedeboer] from comment #81) > Well, I think you also noticed that I needed to add this exception for the > case when you switch from default theme to theme1, which is a complete theme > that requires a restart. This means that this is an 'ancient' code path that > I haven't introduced in any of my patches here. > That doesn't mean I won't look into it and fix it - it's just that I'm not > sure if it's worth the investment? I'm actually seeing the failure from the test that switches from a full theme (theme1) to the webextension theme (theme3). Switching between full themes is tested pretty thoroughly elsewhere so I'd be surprised if there was a mistake in the needs-restartness of them > IOW: I needed to code the default theme exceptions _only_ for the test_dss() > complete theme test. The others run fine without it. Sure, but that's exactly where I'd expect you to need to make changes to XPIProvider.enableRequiresRestart etc.

Alright, I get it. Will fix.

Comment on attachment 8837548 [details] Bug 1330349 - Part 4 - make sure theme type WebExtension support is covered by the existing test suite. https://reviewboard.mozilla.org/r/112686/#review117248 ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:440 (Diff revision 7) > + do_check_eq(addon.creator, "Some author"); > + do_check_eq(addon.version, "1.0"); > + do_check_eq(addon.name, "Web Extension Name"); > + do_check_true(addon.isCompatible); > + do_check_false(addon.appDisabled); > + do_check_true(addon.isActive); Sorry but I've realised that this actually isn't correct. Installing a theme through the API shouldn't enable it immediately (see https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_theme.js#661). Instead they get enabled only when installed from a webpage (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#2102)

(In reply to Dave Townsend [:mossop] from comment #84) > Sorry but I've realised that this actually isn't correct. Installing a theme > through the API shouldn't enable it immediately (see > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > test/xpcshell/test_theme.js#661). Instead they get enabled only when > installed from a webpage > (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > AddonManager.jsm#2102) Where should I add tests to make sure this behavior is covered? I see an unused theme.xpi in toolkit/mozapps/extensions/test/xpinstall/... does that mean there was a test before but now it's gone?

Comment on attachment 8830759 [details] Bug 1330349 - Part 3 - make sure only one theme may be selected at the same time. Substantial amount of changes in restart-flag fetching - re-requesting review.

(In reply to Mike de Boer [:mikedeboer] from comment #85) > (In reply to Dave Townsend [:mossop] from comment #84) > > Sorry but I've realised that this actually isn't correct. Installing a theme > > through the API shouldn't enable it immediately (see > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > > test/xpcshell/test_theme.js#661). Instead they get enabled only when > > installed from a webpage > > (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > > AddonManager.jsm#2102) > > Where should I add tests to make sure this behavior is covered? I see an > unused theme.xpi in toolkit/mozapps/extensions/test/xpinstall/... does that > mean there was a test before but now it's gone? Phew, that took some serious history diving. This was implemented in bug 579779 and is tested here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug553455.js#800 You shouldn't need to add tests for this specific case, it is enough to verify that webextension themes install through the API as disabled by default and then all the tests for enabling them in the presence of other themes covers us just fine.

Comment on attachment 8837548 [details] Bug 1330349 - Part 4 - make sure theme type WebExtension support is covered by the existing test suite. https://reviewboard.mozilla.org/r/112686/#review117538 ::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:313 (Diff revision 8) > do_check_true(addon.isActive); > do_check_eq(addon.type, "extension"); > + do_check_true(addon.isWebExtension); > do_check_eq(addon.signedState, mozinfo.addon_signing ? AddonManager.SIGNEDSTATE_SIGNED : AddonManager.SIGNEDSTATE_NOT_REQUIRED); > > + // test reloading a webextension with the same name, but a different type. Huh. I didn't notice this the last time around. I don't think we've ever considered the case where an add-on changes from an extension to a theme. Retaining the userDisabled state is an open question in this case but I guess that is what would happen for full themes so I won't ask you to change it. I imagine there is a bug that no-one has stumbled across before that you've fixed with the notifyAddonChange calls you added in the other patch. Nice work! ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:443 (Diff revision 8) > + do_check_eq(addon.creator, "Some author"); > + do_check_eq(addon.version, "1.0"); > + do_check_eq(addon.name, "Web Extension Name"); > + do_check_true(addon.isCompatible); > + do_check_false(addon.appDisabled); > + do_check_false(addon.isActive); Can you add a test that addon.userDisabled is true here please.

Comment on attachment 8830759 [details] Bug 1330349 - Part 3 - make sure only one theme may be selected at the same time. https://reviewboard.mozilla.org/r/107488/#review117528 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4129 (Diff revision 10) > oldAddon ? oldAddon.wrapper : null, > false); > AddonManagerPrivate.callAddonListeners("onInstalled", addon.wrapper); > > + // Notify providers that a new theme has been enabled. > + if (isTheme(addon.type)) Add a check for addon.active, we shouldn't be notifying otherwise. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4319 (Diff revision 10) > + if (!isChangedAddon) > + this.updateAddonDisabledState(theme, true, undefined, aPendingRestart); > + } else if (isChangedAddon) > newSkin = theme.internalName; > else if (theme.userDisabled == false && !theme.pendingUninstall) > previousTheme = theme; Current bracing style in add-ons manager is a bit weird but once one block has bracing add them to the other blocks too.

Comment on attachment 8839967 [details] Bug 1330349 - Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. https://reviewboard.mozilla.org/r/114544/#review117548 Almost there, but test_undothemeuninstall.js is failing for me because it hasn't been updated for the theme being disabled on install. ::: toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js:368 (Diff revision 4) > + }); > + > + let [ t1, d ] = yield promiseAddonsByIDs([addon.id, "default@tests.mozilla.org"]); > + > + Assert.ok(t1, "Addon should be there"); > + Assert.ok(t1.isActive); This check is failing now as I'd expect. ::: toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js:403 (Diff revision 4) > + Assert.ok(!t1.userDisabled); > + Assert.equal(t1.pendingOperations, AddonManager.PENDING_NONE); > + > + yield promiseRestartManager(); > + > + [ t1, d ] = yield promiseAddonsByIDs([addon.id, "default@tests.mozilla.org"]); addon becomes unusable after the restart. ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:141 (Diff revision 4) > + let theme = yield promiseAddonByID(which); > + prepare_test(expectedEvents); > + theme.userDisabled = disabled; > + > + for (theme of yield promiseAddonsByIDs(THEME_IDS)) { > + disabled = (theme.id in expectedStates) ? expectedStates[theme.id] : true; Clobbering the argument is bad form, can you change here and below to use a different variable name, say isDisabled. ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:143 (Diff revision 4) > + theme.userDisabled = disabled; > + > + for (theme of yield promiseAddonsByIDs(THEME_IDS)) { > + disabled = (theme.id in expectedStates) ? expectedStates[theme.id] : true; > + Assert.equal(theme.userDisabled, disabled, > + `Theme '${theme.id}' should be ${disabled ? "dis" : "en"}abled`); Please add tests for pendingOperations here (should be PENDING_ENABLE for themeToEnable if expectRestart and PENDING_DISABLE for themeToDisable if expectRestart). ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:159 (Diff revision 4) > + for (theme of yield promiseAddonsByIDs(THEME_IDS)) { > + disabled = (theme.id in expectedStates) ? expectedStates[theme.id] : true; > + Assert.equal(theme.userDisabled, disabled, > + `Theme '${theme.id}' should be ${disabled ? "dis" : "en"}abled`); > + Assert.equal(theme.isActive, !disabled, > + `Theme '${theme.id}' should be ${disabled ? "in" : ""}active`); Add tests for pendingOperations here (should always be PENDING_NONE).

(In reply to Dave Townsend [:mossop] from comment #92) > Phew, that took some serious history diving. This was implemented in bug > 579779 and is tested here: > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/ > general/browser_bug553455.js#800 Thanks for the detective work! Since it's an ongoing effort to clean up browser/base/content/test/general/, perhaps it's good if I file a good-first-bug to move it to toolkit/mozapps/extensions/test/xpinstall/ ? (In reply to Dave Townsend [:mossop] from comment #94) > Add a check for addon.active, we shouldn't be notifying otherwise. I didn't add it here, because `installAddonFromLocation` always sets `active = true`: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4080 > Current bracing style in add-ons manager is a bit weird but once one block > has bracing add them to the other blocks too. I didn't feel it was that weird at all! :) I just forgot to add 'em.

(In reply to Mike de Boer [:mikedeboer] from comment #96) > (In reply to Dave Townsend [:mossop] from comment #92) > > Phew, that took some serious history diving. This was implemented in bug > > 579779 and is tested here: > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/ > > general/browser_bug553455.js#800 > > Thanks for the detective work! Since it's an ongoing effort to clean up > browser/base/content/test/general/, perhaps it's good if I file a > good-first-bug to move it to toolkit/mozapps/extensions/test/xpinstall/ ? I think it could certainly be moved somewhere but most of that test is about checking the popup notifications for add-on installation so I wonder if it should stay somewhere in browser. > (In reply to Dave Townsend [:mossop] from comment #94) > > Add a check for addon.active, we shouldn't be notifying otherwise. > > I didn't add it here, because `installAddonFromLocation` always sets `active > = true`: > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > internal/XPIProvider.jsm#4080 Good catch

Comment on attachment 8839967 [details] Bug 1330349 - Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. https://reviewboard.mozilla.org/r/114544/#review117892 Ship it! Sorry for the prolonged back and forth on this review, the theme selection code is one of the more complex pieces of the add-ons manager, well done for navigating it so well. ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js:151 (Diff revisions 4 - 5) > + Assert.ok(hasFlag(theme.pendingOperations, expectedFlag), > + "When expecting a restart, the pending operation flags should match"); > continue; > - Assert.equal(theme.isActive, !disabled, > - `Theme '${theme.id} should be ${disabled ? "in" : ""}active`); > - } > + } Stick an else on here and verify that pendingOperations is PENDING_NONE for the other cases.

(In reply to Dave Townsend [:mossop] from comment #101) > Ship it! > > Sorry for the prolonged back and forth on this review, the theme selection > code is one of the more complex pieces of the add-ons manager, well done for > navigating it so well. Thanks! :-) Once in a while you have these bugs that a bit more challenging, which keeps me sharp. ;-)

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5c2a88e35ffc Part 1 - move the new theme WebExtension API to the toolkit collection of APIs. r=aswan https://hg.mozilla.org/integration/autoland/rev/055b01695448 Part 2 - install and present theme type WebExtensions as themes in the Addon Manager. r=mossop https://hg.mozilla.org/integration/autoland/rev/48e95d0a923f Part 3 - make sure only one theme may be selected at the same time. r=mossop https://hg.mozilla.org/integration/autoland/rev/493cda68b22d Part 4 - make sure theme type WebExtension support is covered by the existing test suite. r=mossop https://hg.mozilla.org/integration/autoland/rev/052d09a75fca Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. r=mossop

Backed out for failing xpcshell test test_dss.js: https://hg.mozilla.org/integration/autoland/rev/a4e3d57b982520d05169b950ff9ac6a9f2786f04 https://hg.mozilla.org/integration/autoland/rev/9dee11cb31bb7a5d097504b18a4b4f5d7f7d9643 https://hg.mozilla.org/integration/autoland/rev/a4317227f3d72e86ff65e59e6a3eb29f3ff83b4f https://hg.mozilla.org/integration/autoland/rev/adaf8e5b6808888d3d834555633f8a619850434c https://hg.mozilla.org/integration/autoland/rev/28cdd3a636747d5ab6f630722dbdaad5ec8719b0 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=052d09a75fca21b2c62e78c0f9f62c2af36271fb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=80916206&repo=autoland 10:54:47 INFO - TEST-PASS | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_dss.js | onDisabled - [onDisabled : 851] true == true 10:54:47 INFO - "Got onEnabling event for 2@personas.mozilla.org" 10:54:47 INFO - TEST-PASS | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_dss.js | onEnabling - [onEnabling : 820] "onEnabling" == "onEnabling" 10:54:47 INFO - TEST-PASS | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_dss.js | onEnabling - [onEnabling : 821] true == true 10:54:47 INFO - TEST-PASS | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_dss.js | onEnabling - [onEnabling : 823] true == true 10:54:47 INFO - TEST-PASS | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_dss.js | onEnabling - [onEnabling : 824] true == true 10:54:47 INFO - PROCESS | 9184 | 1488394486729 DeferredSave.extensions.json DEBUG Save changes 10:54:47 INFO - "Got onDisabling event for theme2@tests.mozilla.org" 10:54:47 INFO - TEST-PASS | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_dss.js | onDisabling - [onDisabling : 839] "onDisabling" == "onDisabling" 10:54:47 WARNING - TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_dss.js | onDisabling - [onDisabling : 840] true == false 10:54:47 INFO - c:/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/head_addons.js:onDisabling:840 10:54:47 INFO - resource://gre/modules/AddonManager.jsm:callAddonListeners:1736 10:54:47 INFO - resource://gre/modules/AddonManager.jsm:callAddonListeners:3196 10:54:47 INFO - resource://gre/modules/addons/XPIProvider.jsm:updateAddonDisabledState:5063 10:54:47 INFO - resource://gre/modules/addons/XPIProvider.jsm:addonChanged:4346 10:54:47 INFO - resource://gre/modules/AddonManager.jsm:callProvider:272 10:54:47 INFO - resource://gre/modules/AddonManager.jsm:notifyAddonChanged:1784 10:54:47 INFO - resource://gre/modules/AddonManager.jsm:notifyAddonChanged:3177 10:54:47 INFO - resource://gre/modules/LightweightThemeManager.jsm:_setCurrentTheme:705 10:54:47 INFO - resource://gre/modules/LightweightThemeManager.jsm:set currentTheme:151 10:54:47 INFO - resource://gre/modules/LightweightThemeManager.jsm:set userDisabled:568 10:54:47 INFO - c:/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_dss.js:run_test_5/<:392 10:54:47 INFO - resource://gre/modules/AddonManager.jsm:safeCall:196 10:54:47 INFO - resource://gre/modules/AddonManager.jsm:makeSafe/<:211 10:54:47 INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:process:922 10:54:47 INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:walkerLoop:806 10:54:47 INFO - exiting test

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/86fae94513cd Part 1 - move the new theme WebExtension API to the toolkit collection of APIs. r=aswan https://hg.mozilla.org/integration/autoland/rev/6e857ab96cda Part 2 - install and present theme type WebExtensions as themes in the Addon Manager. r=mossop https://hg.mozilla.org/integration/autoland/rev/3e2083974a13 Part 3 - make sure only one theme may be selected at the same time. r=mossop https://hg.mozilla.org/integration/autoland/rev/b55dd9178815 Part 4 - make sure theme type WebExtension support is covered by the existing test suite. r=mossop https://hg.mozilla.org/integration/autoland/rev/3a149a84ce47 Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. r=mossop

Backed out for failing mochitest browser_bug591465.js and xpcshell test test_webextension_theme.js: https://hg.mozilla.org/integration/autoland/rev/c92a981a067a1137dc6404d772b866bf1a22beb2 https://hg.mozilla.org/integration/autoland/rev/2eb17ac804ff83df8301ff52626e615114bf9a68 https://hg.mozilla.org/integration/autoland/rev/62879a0799e3ebda99768b5559908fbc954b911e https://hg.mozilla.org/integration/autoland/rev/e4892021456db27fc569175f85cb1b279f37f7cc https://hg.mozilla.org/integration/autoland/rev/79cfb75f621a059c82bbc0b77174437a27b1d97b Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3a149a84ce479c5afb8c4e9b965fc1facd86a612&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log bc: https://treeherder.mozilla.org/logviewer.html#?job_id=80974959&repo=autoland [task 2017-03-01T22:33:50.245136Z] 22:33:50 INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js | 'Install' should be hidden - [task 2017-03-01T22:33:50.247464Z] 22:33:50 INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js | Element should not be null, when checking visibility - [task 2017-03-01T22:33:50.249274Z] 22:33:50 INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js | 'Show More Information' should be visible in list view - [task 2017-03-01T22:33:50.251067Z] 22:33:50 INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js | Element should not be null, when checking visibility - [task 2017-03-01T22:33:50.252779Z] 22:33:50 INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js | Menu separator should be visible with multiple menu items - [task 2017-03-01T22:33:50.254336Z] 22:33:50 INFO - Test 7 took 293ms [task 2017-03-01T22:33:50.255901Z] 22:33:50 INFO - Running test 8 [task 2017-03-01T22:33:50.257429Z] 22:33:50 INFO - Exception thrown: TypeError: aData is null [task 2017-03-01T22:33:50.258995Z] 22:33:50 INFO - Buffered messages finished [task 2017-03-01T22:33:50.260895Z] 22:33:50 INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js | uncaught exception - TypeError: aData is null at _setCurrentTheme@resource://gre/modules/LightweightThemeManager.jsm:665:7 [task 2017-03-01T22:33:50.262491Z] 22:33:50 INFO - set currentTheme@resource://gre/modules/LightweightThemeManager.jsm:151:12 [task 2017-03-01T22:33:50.264134Z] 22:33:50 INFO - @chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js:261:3 [task 2017-03-01T22:33:50.265791Z] 22:33:50 INFO - log_exceptions@chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/test-window/head.js:190:12 [task 2017-03-01T22:33:50.267493Z] 22:33:50 INFO - run_next_test/<@chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/test-window/head.js:231:21 [task 2017-03-01T22:33:50.269082Z] 22:33:50 INFO - run@chrome://mochikit/content/browser-test.js:937:9 [task 2017-03-01T22:33:50.270365Z] 22:33:50 INFO - [task 2017-03-01T22:33:50.271865Z] 22:33:50 INFO - Stack trace: [task 2017-03-01T22:33:50.273306Z] 22:33:50 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1648 [task 2017-03-01T22:33:50.274991Z] 22:33:50 INFO - JavaScript error: resource://gre/modules/LightweightThemeManager.jsm, line 665: TypeError: aData is null [task 2017-03-01T22:33:50.276699Z] 22:33:50 INFO - Console message: 1488407629283 DeferredSave.extensions.json DEBUG Starting write [task 2017-03-01T22:33:50.280952Z] 22:33:50 INFO - 1488407629375 DeferredSave.extensions.json DEBUG Write succeeded [task 2017-03-01T22:33:50.284142Z] 22:33:50 INFO - Console message: [JavaScript Error: "TypeError: aData is null" {file: "resource://gre/modules/LightweightThemeManager.jsm" line: 665}] [task 2017-03-01T22:33:50.287351Z] 22:33:50 INFO - _setCurrentTheme@resource://gre/modules/LightweightThemeManager.jsm:665:7 [task 2017-03-01T22:33:50.289008Z] 22:33:50 INFO - set currentTheme@resource://gre/modules/LightweightThemeManager.jsm:151:12 [task 2017-03-01T22:33:50.290644Z] 22:33:50 INFO - @chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/test-window/browser_bug591465.js:261:3 [task 2017-03-01T22:33:50.292244Z] 22:33:50 INFO - log_exceptions@chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/test-window/head.js:190:12 [task 2017-03-01T22:33:50.293817Z] 22:33:50 INFO - run_next_test/<@chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/test-window/head.js:231:21 [task 2017-03-01T22:33:50.295496Z] 22:33:50 INFO - run@chrome://mochikit/content/browser-test.js:937:9 [task 2017-03-01T22:33:50.296934Z] 22:33:50 INFO - [task 2017-03-01T22:33:50.298580Z] 22:33:50 INFO - Console message: 1488407629375 DeferredSave.extensions.json DEBUG Write succeeded [task 2017-03-01T22:34:03.734002Z] 22:34:03 INFO - Console message: [JavaScript Warning: "The video on this page can’t be played. Your system may not have the required video codecs for: audio/mp4, video/mp4" {file: "chrome://browser/content/browser.xul" line: 0}] [task 2017-03-01T22:34:31.944117Z] 22:34:31 INFO - Longer timeout required, waiting longer... Remaining timeouts: 1 [task 2017-03-01T22:35:16.944587Z] 22:35:16 INFO - Not taking screenshot here: see the one that was previously logged Failure log xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=80981424&repo=autoland [task 2017-03-01T23:02:03.824305Z] 23:02:03 INFO - TEST-START | services/sync/tests/unit/test_prefs_store.js [task 2017-03-01T23:02:05.585390Z] 23:02:05 WARNING - TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_prefs_store.js | xpcshell return code: 0 ... [task 2017-03-01T23:02:05.737720Z] 23:02:05 INFO - PROCESS | 13391 | 1488409325320 addons.xpi-utils DEBUG Rebuilding XPI database with no extensions [task 2017-03-01T23:02:05.739314Z] 23:02:05 INFO - TEST-PASS | services/sync/tests/unit/test_prefs_store.js | run_test - [run_test : 113] "0.9755513381299946" == "0.9755513381299946" [task 2017-03-01T23:02:05.740653Z] 23:02:05 INFO - TEST-PASS | services/sync/tests/unit/test_prefs_store.js | run_test - [run_test : 114] true == true [task 2017-03-01T23:02:05.740923Z] 23:02:05 INFO - PROCESS | 13391 | Disable persona [task 2017-03-01T23:02:05.747704Z] 23:02:05 INFO - TypeError: aData is null at resource://gre/modules/LightweightThemeManager.jsm:665 [task 2017-03-01T23:02:05.748813Z] 23:02:05 INFO - _setCurrentTheme@resource://gre/modules/LightweightThemeManager.jsm:665:7 [task 2017-03-01T23:02:05.750082Z] 23:02:05 INFO - set currentTheme@resource://gre/modules/LightweightThemeManager.jsm:151:12 [task 2017-03-01T23:02:05.751995Z] 23:02:05 INFO - _updateLightWeightTheme@resource://gre/modules/services-sync/engines/prefs.js:119:5 [task 2017-03-01T23:02:05.755651Z] 23:02:05 INFO - _setAllPrefs@resource://gre/modules/services-sync/engines/prefs.js:160:7 [task 2017-03-01T23:02:05.756753Z] 23:02:05 INFO - update@resource://gre/modules/services-sync/engines/prefs.js:205:5 [task 2017-03-01T23:02:05.758217Z] 23:02:05 INFO - run_test@/home/worker/workspace/build/tests/xpcshell/tests/services/sync/tests/unit/test_prefs_store.js:122:5 [task 2017-03-01T23:02:05.759933Z] 23:02:05 INFO - _execute_test@/home/worker/workspace/build/tests/xpcshell/head.js:536:7

Please also check the other failures for that push. E.g. the browser-chrome failures for asan also start with that push.

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d4b1e6c6197 Part 1 - move the new theme WebExtension API to the toolkit collection of APIs. r=aswan https://hg.mozilla.org/integration/autoland/rev/5e294c7848fc Part 2 - install and present theme type WebExtensions as themes in the Addon Manager. r=mossop https://hg.mozilla.org/integration/autoland/rev/db77e56b25d5 Part 3 - make sure only one theme may be selected at the same time. r=mossop https://hg.mozilla.org/integration/autoland/rev/210b3dc908a7 Part 4 - make sure theme type WebExtension support is covered by the existing test suite. r=mossop https://hg.mozilla.org/integration/autoland/rev/2c581e1a3c44 Part 5 - add tests for new theme type WebExtensions uninstall, enable and disable behavior. r=mossop

https://hg.mozilla.org/mozilla-central/rev/3d4b1e6c6197 https://hg.mozilla.org/mozilla-central/rev/5e294c7848fc https://hg.mozilla.org/mozilla-central/rev/db77e56b25d5 https://hg.mozilla.org/mozilla-central/rev/210b3dc908a7 https://hg.mozilla.org/mozilla-central/rev/2c581e1a3c44