Closed
Bug 563241
Opened 14 years ago
Closed 14 years ago
about:addons ignores extensions.dss.enabled
Categories
(Toolkit :: Add-ons Manager, defect, P2)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla1.9.3a5
People
(Reporter: neil, Assigned: neil)
References
Details
(Whiteboard: [rewrite])
Attachments
(3 files, 2 obsolete files)
5.81 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
5.58 KB,
patch
|
Details | Diff | Splinter Review | |
11.14 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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.)
Updated•14 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
||
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-
Assignee | ||
Comment 4•14 years ago
|
||
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 5•14 years ago
|
||
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+
Assignee | ||
Comment 6•14 years ago
|
||
Assignee | ||
Comment 7•14 years ago
|
||
Pushed changeset 61984c0c4e5f to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 8•14 years ago
|
||
Can this be completely tested with an automated test?
Flags: in-testsuite?
Flags: in-litmus?
Target Milestone: --- → mozilla1.9.3a5
Assignee | ||
Comment 9•14 years ago
|
||
Well, I guess we can test that enableRequiresRestart returns false; actually testing the switch needs a build with two themes ;-)
Comment 10•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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?
Assignee | ||
Comment 13•14 years ago
|
||
Well, it would help if I knew which file the existing theme test is in...
Comment 14•14 years ago
|
||
(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.
Assignee | ||
Comment 15•14 years ago
|
||
Ah, no wonder I didn't find it; I didn't think to look under xpcshell.
Assignee | ||
Comment 16•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 18•14 years ago
|
||
I take it the xpcshell tests don't actually load the chrome registry, so it's safe to "switch" themes?
Comment 19•14 years ago
|
||
(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.
Assignee | ||
Comment 20•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #448304 -
Flags: feedback?(dtownsend) → feedback+
Comment 21•14 years ago
|
||
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(); > }); > }
Assignee | ||
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
... 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.
Assignee | ||
Comment 24•14 years ago
|
||
Same as previous patch, but copying to a new file.
Attachment #448304 -
Attachment is obsolete: true
Attachment #449084 -
Flags: review?(dtownsend)
Updated•14 years ago
|
Attachment #449084 -
Flags: review?(dtownsend) → review+
Comment 25•14 years ago
|
||
Comment on attachment 449084 [details] [diff] [review] Basic test Sorry for the delay, looks good
Assignee | ||
Comment 26•14 years ago
|
||
Comment on attachment 449084 [details] [diff] [review] Basic test Pushed changeset 35b389db8369 to mozilla-central.
Updated•14 years ago
|
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Comment 27•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
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
Comment 30•13 years ago
|
||
(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•13 years ago
|
||
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.
Description
•