Closed Bug 1343821 Opened 8 years ago Closed 8 years ago

Remove Dynamic Skin Switching (DSS) support

Categories

(WebExtensions :: Frontend, enhancement)

Product:
WebExtensions
Component:
Frontend
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Bug 1343821 - remove Dynamic Skin Switching (DSS) support.
59 bytes, text/x-review-board-request
mossop
: review+
Details 
DSS is a niche feature only useful for Complete Themes, disabled by default and known to be glitchy when used.

It's for the best to simply remove support for it. Great or dead, is it not?

Additional motivation is that the new Theming API add a new variation to the mix that make the DSS code paths more complex.
Comment on attachment 8842827 [details]
Bug 1343821 - remove Dynamic Skin Switching (DSS) support.

https://reviewboard.mozilla.org/r/116606/#review118320

I didn't do a full review here but it looks like you've actually turned on dynamic theme switching by accident because you've gone too far and removed prefs that are required for theme switching with a restart to work. I tested a build of this and was able to switch themes without a restart.

::: browser/app/profile/firefox.js
(Diff revision 3)
>  pref("extensions.update.background.url", "https://versioncheck-bg.addons.mozilla.org/update/VersionCheck.php?reqVersion=%REQ_VERSION%&id=%ITEM_ID%&version=%ITEM_VERSION%&maxAppVersion=%ITEM_MAXAPPVERSION%&status=%ITEM_STATUS%&appID=%APP_ID%&appVersion=%APP_VERSION%&appOS=%APP_OS%&appABI=%APP_ABI%&locale=%APP_LOCALE%&currentAppVersion=%CURRENT_APP_VERSION%&updateType=%UPDATE_TYPE%&compatMode=%COMPATIBILITY_MODE%");
>  pref("extensions.update.interval", 86400);  // Check for updates to Extensions and
>                                              // Themes every day
> -// Non-symmetric (not shared by extensions) extension-specific [update] preferences
> -pref("extensions.dss.enabled", false);          // Dynamic Skin Switching
> -pref("extensions.dss.switchPending", false);    // Non-dynamic switch pending after next

This pref is still used.

::: browser/components/nsBrowserGlue.js:2036
(Diff revision 3)
> +    if (currentUIVersion < 44) {
> +      if (Services.prefs.prefHasUserValue("extensions.dss.enabled"))
> +        Services.prefs.clearUserPref("extensions.dss.enabled");
> +      if (Services.prefs.prefHasUserValue("extensions.dss.switchPending"))
> +        Services.prefs.clearUserPref("extensions.dss.switchPending");
> +    }

Is there any need to do this? We'll just ignore dss.enabled from now on.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
(Diff revision 3)
>  const PREF_XPI_STATE                  = "extensions.xpiState";
>  const PREF_BOOTSTRAP_ADDONS           = "extensions.bootstrappedAddons";
>  const PREF_PENDING_OPERATIONS         = "extensions.pendingOperations";
> -const PREF_EM_DSS_ENABLED             = "extensions.dss.enabled";
> -const PREF_DSS_SWITCHPENDING          = "extensions.dss.switchPending";
> -const PREF_DSS_SKIN_TO_SELECT         = "extensions.lastSelectedSkin";

Don't let the pref names fool you, SKIN_TO_SELECT and SWITCHPENDING are both required for theme switches with a restart to work.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
(Diff revision 3)
>        this.defaultSkin = defaultPrefs.get(PREF_GENERAL_SKINS_SELECTEDSKIN,
>                                            "classic/1.0");
>        this.currentSkin = Preferences.get(PREF_GENERAL_SKINS_SELECTEDSKIN,
>                                           this.defaultSkin);
>        this.selectedSkin = this.currentSkin;
> -      this.applyThemeChange();

This is where theme changes after a restart are actually applied so is required to stay.

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini:171
(Diff revision 3)
>  [test_langpack.js]
>  [test_disable.js]
>  [test_distribution.js]
> -[test_dss.js]
>  # Bug 676992: test consistently fails on Android
>  fail-if = os == "android"

You need to remove this line too
Attachment #8842827 - Flags: review?(dtownsend) → review-
AH! That explains all the things I didn't understand so far. Is it OK to keep the names, or would it be good to take this opportunity and rename the prefs as well to not contain 'dss' anymore?
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> AH! That explains all the things I didn't understand so far. Is it OK to
> keep the names, or would it be good to take this opportunity and rename the
> prefs as well to not contain 'dss' anymore?

You can rename the constant names for sure, renaming the pref names themselves though would require some migration code and I don't think it's worth it
Comment on attachment 8842827 [details]
Bug 1343821 - remove Dynamic Skin Switching (DSS) support.

https://reviewboard.mozilla.org/r/116606/#review118802

So the problem here is that now browser_bug592338.js is no longer testing what it is meant to test. I can't think of a way to make it work either. I guess we should just remove that test entirely and live with it since complete themes are going away.

::: browser/app/profile/firefox.js
(Diff revision 4)
>  pref("extensions.update.interval", 86400);  // Check for updates to Extensions and
>                                              // Themes every day
> -// Non-symmetric (not shared by extensions) extension-specific [update] preferences
> -pref("extensions.dss.enabled", false);          // Dynamic Skin Switching
> -pref("extensions.dss.switchPending", false);    // Non-dynamic switch pending after next
> -                                                // restart.

This pref is still used
Attachment #8842827 - Flags: review?(dtownsend) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48c4281a7308
remove Dynamic Skin Switching (DSS) support. r=mossop
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9f6c1e46e8c
Backed out changeset 48c4281a7308 for eslint failures
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec6261d6fe47
remove Dynamic Skin Switching (DSS) support. r=mossop
Backed out for failing browser_parsable_css.js | missing chrome://global/skin/arrow/arrow-lft-hov.gif referenced from chrome://global/skin/arrow.css:

https://hg.mozilla.org/integration/autoland/rev/5d32870dc04459028f304c107900efcf88f0aff4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ec6261d6fe47308f5ba6a63c138798732eb074d3&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=83741801&repo=autoland

[task 2017-03-14T16:48:42.182863Z] 16:48:42     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_parsable_css.js | missing chrome://global/skin/arrow/arrow-lft-hov.gif referenced from chrome://global/skin/arrow.css - 
[task 2017-03-14T16:48:42.183135Z] 16:48:42     INFO - Stack trace:
[task 2017-03-14T16:48:42.186614Z] 16:48:42     INFO -     chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_css.js:checkAllTheCSS:343
[task 2017-03-14T16:48:42.187011Z] 16:48:42     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-03-14T16:48:42.187352Z] 16:48:42     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-03-14T16:48:42.189197Z] 16:48:42     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
Flags: needinfo?(mdeboer)
Comment on attachment 8842827 [details]
Bug 1343821 - remove Dynamic Skin Switching (DSS) support.

https://reviewboard.mozilla.org/r/116606/#review122160

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1436
(Diff revision 7)
> -      let activeTheme = _findAddon(
> +    let activeTheme = _findAddon(
> -        this.addonDB,
> +      this.addonDB,
> -        aAddon => (aAddon.type == "theme") &&
> +      aAddon => (aAddon.type == "theme") &&
> -                  (aAddon.internalName == XPIProvider.selectedSkin));
> +                (aAddon.internalName == XPIProvider.selectedSkin));
> -      if (activeTheme) {
> +    if (activeTheme) {
> -        themes.push(activeTheme);
> +      text += "Extension" + (fullCount++) + "=" + activeTheme.descriptor + "\r\n";

The line needs to be:

    Extension0=<descriptor>\r\n

But fullCount may be greater than 0 so you need to move the increment of that somewhere else.
Comment on attachment 8842827 [details]
Bug 1343821 - remove Dynamic Skin Switching (DSS) support.

https://reviewboard.mozilla.org/r/116606/#review122164

I'd like to look over that change again when done.
Attachment #8842827 - Flags: review+
(In reply to Dave Townsend [:mossop] from comment #19)
> The line needs to be:
> 
>     Extension0=<descriptor>\r\n
> 
> But fullCount may be greater than 0 so you need to move the increment of
> that somewhere else.

Almost. The structure of an array in an ini file like this is that for each `[section]`, the counter needs to start over from 0. However, the array is 1-indexed, so the line needs to be:

    Extension1=<descriptor>\r\n

Using `fullCount` here at all was the mistake. Instead, I need to make sure that
1. I reset `count` before I add items to a new section
2. Use `count++` to appropriately name the the entries
3. Increment `fullCount` with `count` after I'm done adding entries to a section.

I should've been more careful here, noticing this earlier.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #22)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3e8c7cbbbf2

Looking _much_ better :)
Comment on attachment 8842827 [details]
Bug 1343821 - remove Dynamic Skin Switching (DSS) support.

https://reviewboard.mozilla.org/r/116606/#review122564
Attachment #8842827 - Flags: review?(dtownsend) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a480d181f3f
remove Dynamic Skin Switching (DSS) support. r=mossop
https://hg.mozilla.org/mozilla-central/rev/5a480d181f3f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: