Bug 1472740 - Remove support for deprecated lwt aliases from WebExtensions theme API. r=ntim,robwu
authorLuca Greco <lgreco@mozilla.com>
Fri, 19 Jul 2019 11:30:48 +0000
changeset 483473 40d1a7318a944bcb1af9b48b52a2fc3a00c11248
parent 483472 87346cd601faf0310a32d8ac8b147d463ff7162f
child 483474 eb7f4d56f54b3283fc15983ee859b5e62fcb9f3b
push id36319
push userncsoregi@mozilla.com
push dateFri, 19 Jul 2019 15:30:41 +0000
treeherdermozilla-central@eb7f4d56f54b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersntim, robwu
bugs1472740
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1472740 - Remove support for deprecated lwt aliases from WebExtensions theme API. r=ntim,robwu Removing the lwt aliases from the theme API schema is not going to be enough, because the images and colors properties are very permissive on the unknown properties (likely to prevent a property supported on chrome but not on firefox to prevent the theme from being installed), and so removing the lwt aliases from the schema would not raise any error (the theme API implementation would just be silently ignoring the deprecated lwt aliases). For the above reason the following patch use the following approach: - kept the deprecated lwt aliases in the schema, but changes the deprecation warning message to mention that the property is now completely ignored by Firefox, and which property should be used instead - removed the deprecation warning from the toolbar_text theme colors property, as we decided that we are not going to deprecate it anymore - changed the theme API implementation to ignore the deprecated lwt alias property - repurposed browser_ext_themes_lwtsupport.js test file to verify that the lwt aliases are ignored as expected A separate addons-linter pull request is going to be created, to ensure that the addons-linter will raise linting errors (instead of linting warning) when these deprecated lwt aliases are being used in a theme (to prevent that newly submitted theme versions including the aliases will go unnoticed). Depends on D37890 Differential Revision: https://phabricator.services.mozilla.com/D37891
toolkit/components/extensions/parent/ext-theme.js
toolkit/components/extensions/schemas/theme.json
toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js
toolkit/components/extensions/test/browser/browser_ext_themes_lwtsupport.js
toolkit/components/extensions/test/browser/browser_ext_themes_toolbars.js
--- a/toolkit/components/extensions/parent/ext-theme.js
+++ b/toolkit/components/extensions/parent/ext-theme.js
@@ -171,24 +171,22 @@ class Theme {
 
       let cssColor = val;
       if (Array.isArray(val)) {
         cssColor =
           "rgb" + (val.length > 3 ? "a" : "") + "(" + val.join(",") + ")";
       }
 
       switch (color) {
-        case "accentcolor":
         case "frame":
           styles.accentcolor = cssColor;
           break;
         case "frame_inactive":
           styles.accentcolorInactive = cssColor;
           break;
-        case "textcolor":
         case "tab_background_text":
           styles.textcolor = cssColor;
           break;
         case "toolbar":
           styles.toolbarColor = cssColor;
           break;
         case "toolbar_text":
         case "bookmark_text":
@@ -266,17 +264,16 @@ class Theme {
       }
 
       switch (image) {
         case "additional_backgrounds": {
           let backgroundImages = val.map(img => baseURI.resolve(img));
           styles.additionalBackgrounds = backgroundImages;
           break;
         }
-        case "headerURL":
         case "theme_frame": {
           let resolvedURL = baseURI.resolve(val);
           styles.headerURL = resolvedURL;
           break;
         }
         default: {
           if (
             this.experiment &&
--- a/toolkit/components/extensions/schemas/theme.json
+++ b/toolkit/components/extensions/schemas/theme.json
@@ -84,17 +84,17 @@
                 "type": "array",
                 "items": { "$ref": "ImageDataOrExtensionURL" },
                 "maxItems": 15,
                 "optional": true
               },
               "headerURL": {
                 "$ref": "ImageDataOrExtensionURL",
                 "optional": true,
-                "deprecated": "Please use <em>theme.images.theme_frame</em>, this alias will be removed in Firefox 69."
+                "deprecated": "Unsupported images property, use 'theme.images.theme_frame', this alias is ignored in Firefox >= 70."
               },
               "theme_frame": {
                 "$ref": "ImageDataOrExtensionURL",
                 "optional": true
               }
             },
             "additionalProperties": { "$ref": "ImageDataOrExtensionURL" }
           },
@@ -104,30 +104,30 @@
             "properties": {
               "tab_selected": {
                 "$ref": "ThemeColor",
                 "optional": true
               },
               "accentcolor": {
                 "$ref": "ThemeColor",
                 "optional": true,
-                "deprecated": "Please use <em>theme.colors.frame</em>, this alias will be removed in Firefox 69."
+                "deprecated": "Unsupported colors property, use 'theme.colors.frame', this alias is ignored in Firefox >= 70."
               },
               "frame": {
                 "$ref": "ThemeColor",
                 "optional": true
               },
               "frame_inactive": {
                 "$ref": "ThemeColor",
                 "optional": true
               },
               "textcolor": {
                 "$ref": "ThemeColor",
                 "optional": true,
-                "deprecated": "Please use <em>theme.colors.tab_background_text</em>, this alias will be removed in Firefox 69."
+                "deprecated": "Unsupported color property, use 'theme.colors.tab_background_text', this alias is ignored in Firefox >= 70."
               },
               "tab_background_text": {
                 "$ref": "ThemeColor",
                 "optional": true
               },
               "tab_background_separator": {
                 "$ref": "ThemeColor",
                 "optional": true
@@ -146,17 +146,17 @@
               },
               "toolbar": {
                 "$ref": "ThemeColor",
                 "optional": true
               },
               "toolbar_text": {
                 "$ref": "ThemeColor",
                 "optional": true,
-                "deprecated": "Please use <em>theme.colors.bookmark_text</em>, this alias will be removed in Firefox 69."
+                 "description": "This color property is an alias of 'bookmark_text'."
               },
               "bookmark_text": {
                 "$ref": "ThemeColor",
                 "optional": true
               },
               "toolbar_field": {
                 "$ref": "ThemeColor",
                 "optional": true
--- a/toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js
+++ b/toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js
@@ -1,13 +1,13 @@
 "use strict";
 
 add_task(async function test_support_theme_frame() {
   const FRAME_COLOR = [71, 105, 91];
-  const TAB_TEXT_COLOR = [207, 221, 192];
+  const TAB_TEXT_COLOR = [0, 0, 0];
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       theme: {
         images: {
           theme_frame: "face.png",
         },
         colors: {
           frame: FRAME_COLOR,
@@ -19,19 +19,25 @@ add_task(async function test_support_the
       "face.png": imageBufferFromDataURI(ENCODED_IMAGE_DATA),
     },
   });
 
   await extension.startup();
 
   let docEl = window.document.documentElement;
   Assert.ok(docEl.hasAttribute("lwtheme"), "LWT attribute should be set");
+
+  Assert.ok(
+    docEl.hasAttribute("lwtheme-image"),
+    "LWT image attribute should be set"
+  );
+
   Assert.equal(
     docEl.getAttribute("lwthemetextcolor"),
-    "bright",
+    "dark",
     "LWT text color attribute should be set"
   );
 
   let style = window.getComputedStyle(docEl);
   Assert.ok(
     style.backgroundImage.includes("face.png"),
     `The backgroundImage should use face.png. Actual value is: ${
       style.backgroundImage
@@ -46,16 +52,26 @@ add_task(async function test_support_the
     style.color,
     "rgb(" + TAB_TEXT_COLOR.join(", ") + ")",
     "Expected correct text color"
   );
 
   await extension.unload();
 
   Assert.ok(!docEl.hasAttribute("lwtheme"), "LWT attribute should not be set");
+
+  Assert.ok(
+    !docEl.hasAttribute("lwtheme-image"),
+    "LWT image attribute should not be set"
+  );
+
+  Assert.ok(
+    !docEl.hasAttribute("lwthemetextcolor"),
+    "LWT text color attribute should not be set"
+  );
 });
 
 add_task(async function test_support_theme_frame_inactive() {
   const FRAME_COLOR = [71, 105, 91];
   const FRAME_COLOR_INACTIVE = [255, 0, 0];
   const TAB_TEXT_COLOR = [207, 221, 192];
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
--- a/toolkit/components/extensions/test/browser/browser_ext_themes_lwtsupport.js
+++ b/toolkit/components/extensions/test/browser/browser_ext_themes_lwtsupport.js
@@ -1,11 +1,14 @@
 "use strict";
 
-add_task(async function test_support_LWT_properties() {
+const DEFAULT_THEME_BG_COLOR = "rgb(255, 255, 255)";
+const DEFAULT_THEME_TEXT_COLOR = "rgb(0, 0, 0)";
+
+add_task(async function test_deprecated_LWT_properties_ignored() {
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       theme: {
         images: {
           headerURL: "image1.png",
         },
         colors: {
           accentcolor: ACCENT_COLOR,
@@ -20,125 +23,32 @@ add_task(async function test_support_LWT
 
   await extension.startup();
 
   let docEl = window.document.documentElement;
   let style = window.getComputedStyle(docEl);
 
   Assert.ok(docEl.hasAttribute("lwtheme"), "LWT attribute should be set");
   Assert.ok(
-    docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should be set"
+    !docEl.hasAttribute("lwtheme-image"),
+    "LWT image attribute should not be set on deprecated headerURL alias"
   );
   Assert.equal(
     docEl.getAttribute("lwthemetextcolor"),
-    "bright",
-    "LWT text color attribute should be set"
+    "dark",
+    "LWT text color attribute should not be set on deprecated textcolor alias"
   );
 
-  Assert.ok(
-    style.backgroundImage.includes("image1.png"),
-    "Expected background image"
-  );
   Assert.equal(
     style.backgroundColor,
-    "rgb(" + hexToRGB(ACCENT_COLOR).join(", ") + ")",
-    "Expected correct background color"
+    DEFAULT_THEME_BG_COLOR,
+    "Expected default theme background color"
   );
   Assert.equal(
     style.color,
-    "rgb(" + hexToRGB(TEXT_COLOR).join(", ") + ")",
-    "Expected correct text color"
+    DEFAULT_THEME_TEXT_COLOR,
+    "Expected default theme text color"
   );
 
   await extension.unload();
 
   Assert.ok(!docEl.hasAttribute("lwtheme"), "LWT attribute should not be set");
-  Assert.ok(
-    !docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should not be set"
-  );
 });
-
-add_task(async function test_LWT_image_attribute() {
-  let extension = ExtensionTestUtils.loadExtension({
-    manifest: {
-      theme: {
-        colors: {
-          accentcolor: ACCENT_COLOR,
-          textcolor: TEXT_COLOR,
-        },
-      },
-    },
-  });
-
-  await extension.startup();
-
-  let docEl = window.document.documentElement;
-  Assert.ok(docEl.hasAttribute("lwtheme"), "LWT attribute should be set");
-  Assert.ok(
-    !docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should not be set"
-  );
-  await extension.unload();
-  Assert.ok(!docEl.hasAttribute("lwtheme"), "LWT attribute should not be set");
-  Assert.ok(
-    !docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should not be set"
-  );
-});
-
-add_task(async function test_LWT_does_not_require_accentcolor_textcolor_only() {
-  let extension = ExtensionTestUtils.loadExtension({
-    manifest: {
-      theme: {
-        colors: {
-          textcolor: TEXT_COLOR,
-        },
-      },
-    },
-  });
-
-  await extension.startup();
-
-  let docEl = window.document.documentElement;
-  Assert.ok(docEl.hasAttribute("lwtheme"), "LWT attribute should be set");
-  Assert.ok(
-    !docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should not be set"
-  );
-  await extension.unload();
-  Assert.ok(!docEl.hasAttribute("lwtheme"), "LWT attribute should not be set");
-  Assert.ok(
-    !docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should not be set"
-  );
-});
-
-add_task(async function test_LWT_does_not_require_accentcolor_image_only() {
-  let extension = ExtensionTestUtils.loadExtension({
-    manifest: {
-      theme: {
-        images: {
-          headerURL: "image1.png",
-        },
-      },
-    },
-    files: {
-      "image1.png": BACKGROUND,
-    },
-  });
-
-  await extension.startup();
-
-  let docEl = window.document.documentElement;
-  Assert.ok(docEl.hasAttribute("lwtheme"), "LWT attribute should be set");
-  Assert.ok(
-    docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should be set"
-  );
-  await extension.unload();
-  Assert.ok(!docEl.hasAttribute("lwtheme"), "LWT attribute should not be set");
-  Assert.ok(
-    !docEl.hasAttribute("lwtheme-image"),
-    "LWT image attribute should not be set"
-  );
-});
--- a/toolkit/components/extensions/test/browser/browser_ext_themes_toolbars.js
+++ b/toolkit/components/extensions/test/browser/browser_ext_themes_toolbars.js
@@ -8,18 +8,16 @@ add_task(async function test_support_too
   const TOOLBAR_TEXT_COLOR = "#9400ff";
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       theme: {
         colors: {
           frame: ACCENT_COLOR,
           tab_background_text: TEXT_COLOR,
           toolbar: TOOLBAR_COLOR,
-          // NOTE: this property is going to be removed on Firefox 69
-          // (and bookmark_text is going to replace it).
           toolbar_text: TOOLBAR_TEXT_COLOR,
         },
       },
     },
   });
 
   let toolbox = document.querySelector("#navigator-toolbox");
   let toolbars = [