Bug 1661123 - Make LightWeightThemeConsumer.jsm sanitize colors faster. r=Gijs,jwatt
☠☠ backed out by 3081e15cb974 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 27 Aug 2020 10:50:54 +0000
changeset 546572 8fe9ffffccda1f4eddebe0c4dda3edaacbaa9104
parent 546571 7342399037194f8436cd9c3e08da64ec0dbe41f2
child 546573 3081e15cb9743821d0e64ecb08836c5cf0a47afb
push id37735
push userabutkovits@mozilla.com
push dateThu, 27 Aug 2020 21:29:40 +0000
treeherdermozilla-central@109f3a4de567 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, jwatt
bugs1661123
milestone82.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 1661123 - Make LightWeightThemeConsumer.jsm sanitize colors faster. r=Gijs,jwatt Instead of creating an element, flushing styles and getting the computed value back just to receive, use the existing InspectorUtils.colorToRGBA. With some refactoring, we can completely get rid of parsing rgba strings in LightWeightThemeConsumer too, as a benefit. This should be much faster. This patch tweaks the InspectorUtils API to allow taking a document, so that system colors keep working. We could probably get away without supporting system colors, but it'd technically be a regression, and since we want this patch to be uplifted, and it's easy, let's avoid breaking changes. Differential Revision: https://phabricator.services.mozilla.com/D88200
dom/chrome-webidl/InspectorUtils.webidl
layout/inspector/InspectorUtils.cpp
layout/inspector/InspectorUtils.h
layout/inspector/tests/test_color_to_rgba.html
toolkit/modules/LightweightThemeConsumer.jsm
--- a/dom/chrome-webidl/InspectorUtils.webidl
+++ b/dom/chrome-webidl/InspectorUtils.webidl
@@ -33,17 +33,17 @@ namespace InspectorUtils {
       unsigned long selectorIndex,
       optional [TreatNullAs=EmptyString] DOMString pseudo = "",
       optional boolean includeVisitedStyle = false);
   boolean isInheritedProperty(UTF8String property);
   sequence<DOMString> getCSSPropertyNames(optional PropertyNamesOptions options = {});
   sequence<PropertyPref> getCSSPropertyPrefs();
   [Throws] sequence<DOMString> getCSSValuesForProperty(UTF8String property);
   [Throws] DOMString rgbToColorName(octet r, octet g, octet b);
-  InspectorRGBATuple? colorToRGBA(UTF8String colorString);
+  InspectorRGBATuple? colorToRGBA(UTF8String colorString, optional Document? doc = null);
   boolean isValidCSSColor(UTF8String colorString);
   [Throws] sequence<DOMString> getSubpropertiesForCSSProperty(UTF8String property);
   [Throws] boolean cssPropertyIsShorthand(UTF8String property);
 
   [Throws] boolean cssPropertySupportsType(UTF8String property, InspectorPropertyType type);
 
   boolean isIgnorableWhitespace(CharacterData dataNode);
   Node? getParentForNode(Node node, boolean showingAnonymousContent);
--- a/layout/inspector/InspectorUtils.cpp
+++ b/layout/inspector/InspectorUtils.cpp
@@ -478,22 +478,29 @@ void InspectorUtils::RgbToColorName(Glob
     aRv.Throw(NS_ERROR_INVALID_ARG);
     return;
   }
 
   aColorName.AssignASCII(color);
 }
 
 /* static */
-void InspectorUtils::ColorToRGBA(GlobalObject& aGlobalObject,
-                                 const nsACString& aColorString,
+void InspectorUtils::ColorToRGBA(GlobalObject&, const nsACString& aColorString,
+                                 const Document* aDoc,
                                  Nullable<InspectorRGBATuple>& aResult) {
   nscolor color = NS_RGB(0, 0, 0);
 
-  if (!ServoCSSParser::ComputeColor(nullptr, NS_RGB(0, 0, 0), aColorString,
+  ServoStyleSet* styleSet = nullptr;
+  if (aDoc) {
+    if (PresShell* ps = aDoc->GetPresShell()) {
+      styleSet = ps->StyleSet();
+    }
+  }
+
+  if (!ServoCSSParser::ComputeColor(styleSet, NS_RGB(0, 0, 0), aColorString,
                                     &color)) {
     aResult.SetNull();
     return;
   }
 
   InspectorRGBATuple& tuple = aResult.SetValue();
   tuple.mR = NS_GET_R(color);
   tuple.mG = NS_GET_G(color);
--- a/layout/inspector/InspectorUtils.h
+++ b/layout/inspector/InspectorUtils.h
@@ -128,17 +128,18 @@ class InspectorUtils {
   static void RgbToColorName(GlobalObject& aGlobal, uint8_t aR, uint8_t aG,
                              uint8_t aB, nsAString& aResult, ErrorResult& aRv);
 
   // Convert a given CSS color string to rgba. Returns null on failure or an
   // InspectorRGBATuple on success.
   //
   // NOTE: Converting a color to RGBA may be lossy when converting from some
   // formats e.g. CMYK.
-  static void ColorToRGBA(GlobalObject& aGlobal, const nsACString& aColorString,
+  static void ColorToRGBA(GlobalObject&, const nsACString& aColorString,
+                          const Document*,
                           Nullable<InspectorRGBATuple>& aResult);
 
   // Check whether a given color is a valid CSS color.
   static bool IsValidCSSColor(GlobalObject& aGlobal,
                               const nsACString& aColorString);
 
   // Utilities for obtaining information about a CSS property.
 
--- a/layout/inspector/tests/test_color_to_rgba.html
+++ b/layout/inspector/tests/test_color_to_rgba.html
@@ -20,16 +20,25 @@
   testColor("rgba(50%,75%,60%)", {r:128, g:191, b:153, a:1});
   testColor("rgb(100%,50%,25%,0.7)", {r:255, g:128, b:64, a:0.7});
   testColor("rgba(100%,50%,25%,0.7)", {r:255, g:128, b:64, a:0.7});
   testColor("hsl(320,30%,10%)", {r:33, g:18, b:28, a:1});
   testColor("hsla(320,30%,10%)", {r:33, g:18, b:28, a:1});
   testColor("hsl(170,60%,40%,0.9)", {r:41, g:163, b:143, a:0.9});
   testColor("hsla(170,60%,40%,0.9)", {r:41, g:163, b:143, a:0.9});
 
+  // NOTE: LightweightThemeConsumer is the only consumer of this API, for
+  // backwards compat reasons... But it doesn't seem like it'd really need to
+  // care about system colors, so maybe we can remove it in the future.
+  isnot(
+    InspectorUtils.colorToRGBA("ButtonText", document),
+    null,
+    "Should support system colors when given a displayed document"
+  );
+
   function testColor(color, expected) {
     let rgb = InspectorUtils.colorToRGBA(color);
 
     if (rgb === null) {
       ok(expected === null, "color: " + color + " returns null");
       return;
     }
 
--- a/toolkit/modules/LightweightThemeConsumer.jsm
+++ b/toolkit/modules/LightweightThemeConsumer.jsm
@@ -337,19 +337,18 @@ LightweightThemeConsumer.prototype = {
     if (!active || !experiment) {
       return;
     }
 
     let usedVariables = [];
     if (properties.colors) {
       for (const property in properties.colors) {
         const cssVariable = experiment.colors[property];
-        const value = _sanitizeCSSColor(
-          root.ownerDocument,
-          properties.colors[property]
+        const value = _rgbaToString(
+          _cssColorToRGBA(root.ownerDocument, properties.colors[property])
         );
         usedVariables.push([cssVariable, value]);
       }
     }
 
     if (properties.images) {
       for (const property in properties.images) {
         const cssVariable = experiment.images[property];
@@ -385,17 +384,17 @@ LightweightThemeConsumer.prototype = {
 
 function _getContentProperties(doc, active, data) {
   if (!active) {
     return {};
   }
   let properties = {};
   for (let property in data) {
     if (ThemeContentPropertyList.includes(property)) {
-      properties[property] = _parseRGBA(_sanitizeCSSColor(doc, data[property]));
+      properties[property] = _cssColorToRGBA(doc, data[property]);
     }
   }
   return properties;
 }
 
 function _setImage(aRoot, aActive, aVariableName, aURLs) {
   if (aURLs && !Array.isArray(aURLs)) {
     aURLs = [aURLs];
@@ -412,89 +411,64 @@ function _setProperty(elem, active, vari
   if (active && value) {
     elem.style.setProperty(variableName, value);
   } else {
     elem.style.removeProperty(variableName);
   }
 }
 
 function _setProperties(root, active, themeData) {
-  let properties = [];
   let propertyOverrides = new Map();
 
   for (let map of [toolkitVariableMap, ThemeVariableMap]) {
     for (let [cssVarName, definition] of map) {
       const {
         lwtProperty,
         fallbackProperty,
         optionalElementID,
         processColor,
         isColor = true,
       } = definition;
       let elem = optionalElementID
         ? root.ownerDocument.getElementById(optionalElementID)
         : root;
       let val = propertyOverrides.get(lwtProperty) || themeData[lwtProperty];
       if (isColor) {
-        val = _sanitizeCSSColor(root.ownerDocument, val);
+        val = _cssColorToRGBA(root.ownerDocument, val);
         if (!val && fallbackProperty) {
-          val = _sanitizeCSSColor(
+          val = _cssColorToRGBA(
             root.ownerDocument,
             themeData[fallbackProperty]
           );
         }
         if (processColor) {
-          val = processColor(_parseRGBA(val), elem, propertyOverrides);
+          val = processColor(val, elem, propertyOverrides);
+        } else {
+          val = _rgbaToString(val);
         }
       }
-      properties.push([elem, cssVarName, val]);
+      _setProperty(elem, active, cssVarName, val);
     }
   }
-
-  // Set all the properties together, since _sanitizeCSSColor flushes.
-  for (const [elem, cssVarName, val] of properties) {
-    _setProperty(elem, active, cssVarName, val);
-  }
 }
 
-function _sanitizeCSSColor(doc, cssColor) {
+const kInvalidColor = { r: 0, g: 0, b: 0, a: 1 };
+
+function _cssColorToRGBA(doc, cssColor) {
   if (!cssColor) {
     return null;
   }
-  const HTML_NS = "http://www.w3.org/1999/xhtml";
-  // style.color normalizes color values and makes invalid ones black, so a
-  // simple round trip gets us a sanitized color value.
-  // Use !important so that the theme's stylesheets cannot override us.
-  let div = doc.createElementNS(HTML_NS, "div");
-  div.style.setProperty("color", "black", "important");
-  div.style.setProperty("display", "none", "important");
-  let span = doc.createElementNS(HTML_NS, "span");
-  span.style.setProperty("color", cssColor, "important");
-
-  // CSS variables are not allowed and should compute to black.
-  if (span.style.color.includes("var(")) {
-    span.style.color = "";
-  }
-
-  div.appendChild(span);
-  doc.documentElement.appendChild(div);
-  cssColor = doc.defaultView.getComputedStyle(span).color;
-  div.remove();
-  return cssColor;
+  return (
+    doc.defaultView.InspectorUtils.colorToRGBA(cssColor, doc) || kInvalidColor
+  );
 }
 
-function _parseRGBA(aColorString) {
-  if (!aColorString) {
+function _rgbaToString(parsedColor) {
+  if (!parsedColor) {
     return null;
   }
-  var rgba = aColorString.replace(/(rgba?\()|(\)$)/g, "").split(",");
-  rgba = rgba.map(x => parseFloat(x));
-  return {
-    r: rgba[0],
-    g: rgba[1],
-    b: rgba[2],
-    a: 3 in rgba ? rgba[3] : 1,
-  };
+  let { r, g, b, a } = parsedColor;
+  return `rgba(${r}, ${g}, ${b}, ${a})`;
 }
 
 function _isColorDark(r, g, b) {
   return 0.2125 * r + 0.7154 * g + 0.0721 * b <= 110;
 }