563241 - about:addons ignores extensions.dss.enabled

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[[e-mail]]

XPIProvider's enableRequiresRestart method doesn't test it. It's as simple as adding !Prefs.getBoolPref(PREF_EM_DSS_ENABLED) && to the condition.

disableRequiresRestart probably also needs changes.

Comment 1

[[e-mail]]

For some reason DSS works fine if you start with Modern (succeeds both in switching to Classic and back to Modern), but if you start with Classic then it appears to have no effect until you restart. Even directly poking the general.skins.selectedSkin preference in about:config has no effect. (In standard builds it immediately switches theme both to and from Classic.)

Comment 2

[[e-mail]]

Attachment: Proposed patch

There must have been something else wrong with the build I was previously using to test; I tried a completely different build and everything works as expected, both via about:addons and the SeaMonkey theme switching menu.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 445066 [details] [diff] [review]
Proposed patch

Comment 2 is accurate I think. The new code only writes the selected theme into the extensions.ini file so only that one's chrome.manifest gets parsed at startup. Might be able to solve that as a part of 564667 but for now to make this work properly we'll need to write all of the themes into extensions.ini again.

Do installRequiresRestart and uninstallRequiresRestart not need the same treatment?

Comment 4

[[e-mail]]

Attachment: Possible patch

OK, so now when DSS is enabled I write out all the themes to extensions.ini which moves the restart requirement to install/uninstall.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 445564 [details] [diff] [review]
Possible patch

>   /**
>    * Tests whether installing an add-on will require a restart.
>    *
>    * @param  aAddon
>    *         The add-on to test
>    * @return true if the operation requires a restart
>    */
>   installRequiresRestart: function XPI_installRequiresRestart(aAddon) {
>     // Themes not currently in use can be installed immediately
>     if (aAddon.type == "theme")
>-      return aAddon.internalName == this.currentSkin;
>+      return aAddon.internalName == this.currentSkin ||
>+             Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
> 
>     return !aAddon.bootstrap;
>   },

I was mistaken, I think we still need a restart for install and uninstall since extensions.ini needs to be recreated in those cases.

>     for (let row in resultRows(stmt)) {
>       text += "Extension" + (count++) + "=" + row.descriptor + "\r\n";
>       enabledAddons.push(row.id + ":" + row.version);
>     }
> 
>     // The selected skin may come from an inactive theme (the default theme
>     // when a lightweight theme is applied for example)
>     text += "\r\n[ThemeDirs]\r\n";
>-    stmt = this.getStatement("getActiveTheme");
>-    stmt.params.internalName = XPIProvider.selectedSkin;
>+    if (Prefs.getBoolPref(PREF_EM_DSS_ENABLED))
>+      stmt = this.getStatement("getThemes");

Nit: surround this with braces (because the else part has braces too)

>+    else {
>+      stmt = this.getStatement("getActiveTheme");
>+      stmt.params.internalName = XPIProvider.selectedSkin;
>+    }
>     count = 0;
>     for (let row in resultRows(stmt)) {
>       text += "Extension" + (count++) + "=" + row.descriptor + "\r\n";
>-      enabledAddons.push(row.id + ":" + row.version);
>+      if (row.internalName == XPIProvider.selectedSkin)
>+        enabledAddons.push(row.id + ":" + row.version);
>     }

We used to include all the themes in enabledItems so lets still do that.

Comment 6

[[e-mail]]

Attachment: Updated for review comments

Comment 7

[[e-mail]]

Pushed changeset 61984c0c4e5f to mozilla-central.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can this be completely tested with an automated test?

Comment 9

[[e-mail]]

Well, I guess we can test that enableRequiresRestart returns false; actually testing the switch needs a build with two themes ;-)

Comment 10

[Pardal Freudenthal (:ShareBird)]

I'm working now on an update for my extension "SwitchThemes" that depends on dss enabled to work. I'm noticing some weird behaviors testing this patch. If I install a new theme or remove one, it stops to work properly. Although enabling a different theme doesn't require a new start, it seems always the default theme is loaded. Deleting extensions.ini and extensions.sqlite solves the problem, but if my extension is enabled I have to delete also XPC.mfl (the extension uses a JS Module).
I've uploaded my extensions folder if you think it can help to test the patch. It contains four different themes and the "SwitchThemes" extension (the other extension is just a companion for my two themes):
www.silvermel.net/dev/xpi/extensions.rar

Comment 11

[Pardal Freudenthal (:ShareBird)]

Sorry, it seems this was caused by the extension; I simply forgot to implement the new install listener. Please don't considerate my last comment.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #9)
> Well, I guess we can test that enableRequiresRestart returns false; actually
> testing the switch needs a build with two themes ;-)

We should be able to add an automated test for this, the current theme tests do verification that switching between multiple themes work (and need a restart) so adding some that test you don't need a restart with dss enabled shouldn't be a problem I think. Think you can handle than Neil?

Comment 13

[[e-mail]]

Well, it would help if I knew which file the existing theme test is in...

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> Well, it would help if I knew which file the existing theme test is in...

test_theme.js has the main tests.

Comment 15

[[e-mail]]

Ah, no wonder I didn't find it; I didn't think to look under xpcshell.

Comment 16

[[e-mail]]

(In reply to comment #9)
> Well, I guess we can test that enableRequiresRestart returns false; actually
> testing the switch needs a build with two themes ;-)
Oops; we don't expose enableRequiresRestart, so we can't actually test it...

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> (In reply to comment #9)
> > Well, I guess we can test that enableRequiresRestart returns false; actually
> > testing the switch needs a build with two themes ;-)
> Oops; we don't expose enableRequiresRestart, so we can't actually test it...

The AddonListener events onEnabling and onDisabling expose whether a restart is necessary. See f.e. the tests switching between the default theme and a lightweight theme.

Comment 18

[[e-mail]]

I take it the xpcshell tests don't actually load the chrome registry, so it's safe to "switch" themes?

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #18)
> I take it the xpcshell tests don't actually load the chrome registry, so it's
> safe to "switch" themes?

They load the registry, but it is perfectly safe to do whatever.

Comment 20

[[e-mail]]

Attachment: WIP

This patched test passes for me. I could
a) write some sort of loop to run test_themes twice
b) hg copy the file and patch the copy, and call that the test
c) only copy the tests that you think need to be copied

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 448304 [details] [diff] [review]
WIP

A rough skim says this looks good. I would copy it to a second test file I think. Let me know when you want a full review but there is at least one oddity I spotted (but I was only skimming so may have missed something in context).

> // Switching from a custom theme to a lightweight theme should require a restart
> function run_test_6() {
>   prepare_test({
>     "[e-mail]": [
>       "onEnabling",
>     ],
>     "[e-mail]": [
>-      "onDisabling"
>+      ["onDisabling", false],
>+      "onDisabled"
>     ]
>   });
> 
>   AddonManager.getAddonsByIDs(["[e-mail]",
>                                "[e-mail]"], function([p2, t2]) {
>     p2.userDisabled = false;
> 
>     ensure_test_completed();
> 
>     prepare_test({
>       "[e-mail]": [
>         "onOperationCancelled",
>       ],
>       "[e-mail]": [
>-        "onOperationCancelled"
>+        ["onEnabling", false],
>+        "onEnabled"
>       ]
>     });
> 
>     t2.userDisabled = false;
> 
>     ensure_test_completed();
> 
>     prepare_test({
>       "[e-mail]": [
>         "onEnabling",
>       ],
>       "[e-mail]": [
>-        "onDisabling"
>+        ["onDisabling", false],
>+        "onDisabled"
>       ]
>     });
> 
>     p2.userDisabled = false;
> 
>     ensure_test_completed();
> 
>     do_check_false(p2.isActive);

Why wouldn't p2 be active at this point?

>     do_check_false(p2.userDisabled);
>     do_check_true(hasFlag(AddonManager.PENDING_ENABLE, p2.pendingOperations));
>-    do_check_true(t2.isActive);
>+    do_check_false(t2.isActive);
>     do_check_true(t2.userDisabled);
>-    do_check_true(hasFlag(AddonManager.PENDING_DISABLE, t2.pendingOperations));
>+    do_check_false(hasFlag(AddonManager.PENDING_DISABLE, t2.pendingOperations));
> 
>     check_test_6();
>   });
> }

Comment 22

[[e-mail]]

I don't really understand Personas but they don't appear to expect DSS to be enabled, which means that they assume a restart is required to switch theme.

Comment 23

[[e-mail]]

... which was one reason why I was hoping you would choose between b) and c) i.e. whether I should include Personas in the DSS tests.

Comment 24

[[e-mail]]

Attachment: Basic test

Same as previous patch, but copying to a new file.

Comment 25

[Dave Townsend [:mossop]]

Comment on attachment 449084 [details] [diff] [review]
Basic test

Sorry for the delay, looks good

Comment 26

[[e-mail]]

Comment on attachment 449084 [details] [diff] [review]
Basic test

Pushed changeset 35b389db8369 to mozilla-central.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, to test this manually I only have to install themes and Personas and switch between them without a restart required. Is that correct?

Comment 28

[Dave Townsend [:mossop]]

(In reply to comment #27)
> Dave, to test this manually I only have to install themes and Personas and
> switch between them without a restart required. Is that correct?

Just themes, personas don't work so well with this at the moment. But I think you shouldn't be spending time verifying this right now as it is not a standard behaviour for the majority of Firefox users.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Too late. I have already run tests around this particular feature, because I also haven't known it until now. Switching themes works perfectly, only for Personas we have problems, as you have also mentioned. I have filed other bugs for those issues.

Marking as verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100617 Minefield/3.7a6pre

Comment 30

[Tony Mechelynck [:tonymec]]

(In reply to comment #8)
> Can this be completely tested with an automated test?

(In reply to comment #9)
> Well, I guess we can test that enableRequiresRestart returns false; actually
> testing the switch needs a build with two themes ;-)

If this may help you, SeaMonkey (which, on trunk, includes the new addons manager) is shipped with two themes built in.

Comment 31

[Justin Wood (:Callek)]

In which case SeaMonkey can/should possibly create a test for this. However tests should also be as close to the feature's code they are testing as possible, so I'd argue that this should _really_ be tested in toolkit, and not relying on SeaMonkey.

But it is a low priority test anyway.