Closed Bug 563241 Opened 10 years ago Closed 10 years ago

about:addons ignores extensions.dss.enabled

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: neil, Assigned: neil)

References

Details

(Whiteboard: [rewrite])

Attachments

(3 files, 2 obsolete files)

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.
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.)
Priority: -- → P2
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #445066 - Flags: review?(dtownsend)
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?
Attachment #445066 - Flags: review?(dtownsend) → review-
Attached patch Possible patchSplinter Review
OK, so now when DSS is enabled I write out all the themes to extensions.ini which moves the restart requirement to install/uninstall.
Attachment #445066 - Attachment is obsolete: true
Attachment #445564 - Flags: review?(dtownsend)
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.
Attachment #445564 - Flags: review?(dtownsend) → review+
Pushed changeset 61984c0c4e5f to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Can this be completely tested with an automated test?
Flags: in-testsuite?
Flags: in-litmus?
Target Milestone: --- → mozilla1.9.3a5
Well, I guess we can test that enableRequiresRestart returns false; actually testing the switch needs a build with two themes ;-)
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
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.
(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?
Well, it would help if I knew which file the existing theme test is in...
(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.
Ah, no wonder I didn't find it; I didn't think to look under xpcshell.
(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...
(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.
I take it the xpcshell tests don't actually load the chrome registry, so it's safe to "switch" themes?
(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.
Attached patch WIP (obsolete) — Splinter Review
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
Attachment #448304 - Flags: feedback?(dtownsend)
Attachment #448304 - Flags: feedback?(dtownsend) → feedback+
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({
>     "2@personas.mozilla.org": [
>       "onEnabling",
>     ],
>     "theme2@tests.mozilla.org": [
>-      "onDisabling"
>+      ["onDisabling", false],
>+      "onDisabled"
>     ]
>   });
> 
>   AddonManager.getAddonsByIDs(["2@personas.mozilla.org",
>                                "theme2@tests.mozilla.org"], function([p2, t2]) {
>     p2.userDisabled = false;
> 
>     ensure_test_completed();
> 
>     prepare_test({
>       "2@personas.mozilla.org": [
>         "onOperationCancelled",
>       ],
>       "theme2@tests.mozilla.org": [
>-        "onOperationCancelled"
>+        ["onEnabling", false],
>+        "onEnabled"
>       ]
>     });
> 
>     t2.userDisabled = false;
> 
>     ensure_test_completed();
> 
>     prepare_test({
>       "2@personas.mozilla.org": [
>         "onEnabling",
>       ],
>       "theme2@tests.mozilla.org": [
>-        "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();
>   });
> }
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.
... 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.
Attached patch Basic testSplinter Review
Same as previous patch, but copying to a new file.
Attachment #448304 - Attachment is obsolete: true
Attachment #449084 - Flags: review?(dtownsend)
Attachment #449084 - Flags: review?(dtownsend) → review+
Comment on attachment 449084 [details] [diff] [review]
Basic test

Sorry for the delay, looks good
Comment on attachment 449084 [details] [diff] [review]
Basic test

Pushed changeset 35b389db8369 to mozilla-central.
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Dave, to test this manually I only have to install themes and Personas and switch between them without a restart required. Is that correct?
(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.
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
Status: RESOLVED → VERIFIED
(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.
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.
You need to log in before you can comment on or make changes to this bug.