Bug 1653520 - Remove code replication in the compatibility actor by moving UserSettings to shared folder r=daisuke,mtigley,devtools-backward-compat-reviewers

Instead of replicationg the browser list fetch using the
preference on the server side, we send the browser list from the
client side to the CompatibilityActor to get the declaration block
compatibility issues.
This way the pref setting is retained completely on the client
side and server doesn't have code duplication.

Differential Revision: https://phabricator.services.mozilla.com/D87613

devtools/client/fronts/compatibility.js

@@ -10,10 +10,57 @@ const {
 } = require("devtools/shared/protocol");
 const { compatibilitySpec } = require("devtools/shared/specs/compatibility");
 
+const PREF_TARGET_BROWSERS = "devtools.inspector.compatibility.target-browsers";
+
 class CompatibilityFront extends FrontClassWithSpec(compatibilitySpec) {
-  // Update the object given a form representation off the wire.
-  form(json) {
-    this.actorID = json.actor;
+  constructor(client, targetFront, parentFront) {
+    super(client, targetFront, parentFront);
+    this.client = client;
+  }
+
+  async initialize() {
+    // Backward-compatibility: can be removed when FF81 is on the release
+    // channel. See 1659866.
+    const preferenceFront = await this.client.mainRoot.getFront("preference");
+    try {
+      await preferenceFront.getCharPref(PREF_TARGET_BROWSERS);
+    } catch (e) {
+      // If `getCharPref` failed, the preference is not set on the server (most
+      // likely Fenix). Set a default value, otherwise compatibility actor
+      // methods might throw.
+      await preferenceFront.setCharPref(PREF_TARGET_BROWSERS, "");
+    }
+  }
+
+  /*
+   * Backwards compatibility wrapper for getCSSDeclarationBlockIssues.
+   * This can be removed once FF82 hits the release channel
+   */
+  async _getTraits() {
+    if (!this._traits) {
+      try {
+        // From FF82+, getTraits() is supported
+        const { traits } = await super.getTraits();
+        this._traits = traits;
+      } catch (e) {
+        this._traits = {};
+      }
+    }
+
+    return this._traits;
+  }
+
+  /*
+   * Backwards compatibility wrapper for getCSSDeclarationBlockIssues.
+   * This can be removed once FF82 hits the release channel
+   */
+  async getCSSDeclarationBlockIssues(declarationBlock, targetBrowsers) {
+    const traits = await this._getTraits();
+    if (!traits.declarationBlockIssueComputationEnabled) {
+      return [];
+    }
+
+    return super.getCSSDeclarationBlockIssues(declarationBlock, targetBrowsers);
   }
 }
 

devtools/client/fronts/styles.js

@@ -192,9 +192,6 @@ class StyleRuleFront extends FrontClassWithSpec(styleRuleSpec) {
       ? this._form.authoredText
       : this._form.cssText;
   }
-  get compatibilityIssues() {
-    return this._form.compatibilityIssues || [];
-  }
   get declarations() {
     return this._form.declarations || [];
   }

devtools/client/inspector/compatibility/README.md

@@ -8,6 +8,7 @@ The compatibility panel consists of the following files:
 * Shared:
   * MDN compatibility dataset: `devtools/shared/compatibility/dataset/`
   * MDN compatibility library: `devtools/server/actors/compatibility/lib/MDNCompatibility.js`
+  * User setting file - `devtools/client/inspector/shared/compatibility-user-settings.js`
 * Server:
   * Actor: `devtools/server/actors/compatibility.js`
   * Front: `devtools/client/fronts/compatibility.js`

devtools/client/inspector/compatibility/actions/compatibility.js

@@ -6,7 +6,7 @@
 
 const nodeConstants = require("devtools/shared/dom-node-constants");
 
-const UserSettings = require("devtools/client/inspector/compatibility/UserSettings");
+const UserSettings = require("devtools/client/inspector/shared/compatibility-user-settings");
 
 const {
   COMPATIBILITY_APPEND_NODE_START,

devtools/client/inspector/compatibility/moz.build

@@ -17,7 +17,6 @@ XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell/xpcshell.ini']
 DevToolsModules(
     'CompatibilityView.js',
     'types.js',
-    'UserSettings.js',
 )
 
 with Files('**'):

devtools/client/inspector/compatibility/test/xpcshell/test_default-browsers.js

@@ -6,7 +6,7 @@
 
 const {
   getDefaultTargetBrowsers,
-} = require("devtools/client/inspector/compatibility/UserSettings");
+} = require("devtools/client/inspector/shared/compatibility-user-settings");
 
 add_task(() => {
   info("Check whether each default browsers data are unique by id and status");

devtools/client/inspector/rules/models/rule.js

@@ -10,6 +10,12 @@ const { ELEMENT_STYLE } = require("devtools/shared/specs/styles");
 const TextProperty = require("devtools/client/inspector/rules/models/text-property");
 const Services = require("Services");
 
+loader.lazyRequireGetter(
+  this,
+  "getTargetBrowsers",
+  "devtools/client/inspector/shared/compatibility-user-settings",
+  true
+);
 loader.lazyRequireGetter(
   this,
   "updateSourceLink",
@@ -56,6 +62,7 @@ class Rule {
   constructor(elementStyle, options) {
     this.elementStyle = elementStyle;
     this.domRule = options.rule;
+    this.compatibilityIssues = null;
     this.matchedSelectors = options.matchedSelectors || [];
     this.pseudoElement = options.pseudoElement || "";
     this.isSystem = options.isSystem;
@@ -102,6 +109,7 @@ class Rule {
     }
 
     this.domRule.off("location-changed", this.onLocationChanged);
+    this.compatibilityIssues = null;
   }
 
   get declarations() {
@@ -235,6 +243,37 @@ class Rule {
     return this.domRule ? this.domRule.column : null;
   }
 
+  /**
+   * Get the declaration block issues from the compatibility actor
+   * @returns An Array of JSON objects with compatibility information in following form:
+   *    {
+   *      // Type of compatibility issue
+   *      type: <string>,
+   *      // The CSS declaration that has compatibility issues
+   *      property: <string>,
+   *      // Alias to the given CSS property
+   *      alias: <Array>,
+   *      // Link to MDN documentation for the particular CSS rule
+   *      url: <string>,
+   *      deprecated: <boolean>,
+   *      experimental: <boolean>,
+   *      // An array of all the browsers that don't support the given CSS rule
+   *      unsupportedBrowsers: <Array>,
+   *    }
+   */
+  async getCompatibilityIssues() {
+    if (!this.compatibilityIssues) {
+      const targetBrowsers = getTargetBrowsers();
+      const compatibility = await this.inspector.inspectorFront.getCompatibilityFront();
+      this.compatibilityIssues = await compatibility.getCSSDeclarationBlockIssues(
+        this.domRule.declarations,
+        targetBrowsers
+      );
+    }
+
+    return this.compatibilityIssues;
+  }
+
   /**
    * Returns the TextProperty with the given id or undefined if it cannot be found.
    *

devtools/client/inspector/rules/models/text-property.js

@@ -107,6 +107,10 @@ class TextProperty {
    * if any.
    */
   updateEditor() {
+    // When the editor updates, reset the saved
+    // compatibility issues list as any updates
+    // may alter the compatibility status of declarations
+    this.rule.compatibilityIssues = null;
     if (this.editor) {
       this.editor.update();
     }
@@ -311,12 +315,13 @@ class TextProperty {
       return { isCompatible: true };
     }
 
-    if (!this.rule.domRule.compatibilityIssues.length) {
+    const compatibilityIssues = await this.rule.getCompatibilityIssues();
+    if (!compatibilityIssues.length) {
       return { isCompatible: true };
     }
 
     const property = this.name;
-    const indexOfProperty = this.rule.domRule.compatibilityIssues.findIndex(
+    const indexOfProperty = compatibilityIssues.findIndex(
       issue => issue.property === property || issue.aliases?.includes(property)
     );
 
@@ -330,7 +335,7 @@ class TextProperty {
       experimental,
       url,
       unsupportedBrowsers,
-    } = this.rule.domRule.compatibilityIssues[indexOfProperty];
+    } = compatibilityIssues[indexOfProperty];
 
     let msgId = COMPATIBILITY_TOOLTIP_MESSAGE.default;
     if (deprecated && experimental && !unsupportedBrowsers.length) {

devtools/client/inspector/rules/test/browser_rules_css-compatibility-toggle-rules.js

@@ -24,15 +24,9 @@ const TEST_URI = `
 
 const TEST_DATA_INITIAL = [
   {
-    selector: ".test-inline",
+    selector: ".test-class-linked",
     rules: [
-      {
-        color: { value: "pink" },
-        "user-select": {
-          value: "none",
-          expected: COMPATIBILITY_TOOLTIP_MESSAGE.default,
-        },
-      },
+      {},
       {
         color: { value: "green" },
         "background-color": { value: "black" },
@@ -44,9 +38,15 @@ const TEST_DATA_INITIAL = [
     ],
   },
   {
-    selector: ".test-class-linked",
+    selector: ".test-inline",
     rules: [
-      {},
+      {
+        color: { value: "pink" },
+        "user-select": {
+          value: "none",
+          expected: COMPATIBILITY_TOOLTIP_MESSAGE.default,
+        },
+      },
       {
         color: { value: "green" },
         "background-color": { value: "black" },
@@ -60,6 +60,17 @@ const TEST_DATA_INITIAL = [
 ];
 
 const TEST_DATA_TOGGLE_CLASS_DECLARATION = [
+  {
+    selector: ".test-class-linked",
+    rules: [
+      {},
+      {
+        color: { value: "green" },
+        "background-color": { value: "black" },
+        cursor: { value: "pointer" },
+      },
+    ],
+  },
   {
     selector: ".test-inline",
     rules: [
@@ -77,6 +88,9 @@ const TEST_DATA_TOGGLE_CLASS_DECLARATION = [
       },
     ],
   },
+];
+
+const TEST_DATA_TOGGLE_INLINE = [
   {
     selector: ".test-class-linked",
     rules: [
@@ -88,9 +102,6 @@ const TEST_DATA_TOGGLE_CLASS_DECLARATION = [
       },
     ],
   },
-];
-
-const TEST_DATA_TOGGLE_INLINE = [
   {
     selector: ".test-inline",
     rules: [
@@ -105,17 +116,6 @@ const TEST_DATA_TOGGLE_INLINE = [
       },
     ],
   },
-  {
-    selector: ".test-class-linked",
-    rules: [
-      {},
-      {
-        color: { value: "green" },
-        "background-color": { value: "black" },
-        cursor: { value: "pointer" },
-      },
-    ],
-  },
 ];
 
 add_task(async function() {

devtools/client/inspector/rules/test/head.js

@@ -667,6 +667,8 @@ async function openEyedropper(view, swatch) {
  *        The rule-view instance.
  * @param {Number} ruleIndex
  *        The index we expect the rule to have in the rule-view.
+ * @param {boolean} addCompatibilityData
+ *        Optional argument to add compatibility dat with the property data
  *
  * @returns A map containing stringified property declarations e.g.
  *          [
@@ -683,15 +685,24 @@ async function openEyedropper(view, swatch) {
  *            ...
  *          ]
  */
-function getPropertiesForRuleIndex(view, ruleIndex) {
+function getPropertiesForRuleIndex(
+  view,
+  ruleIndex,
+  addCompatibilityData = false
+) {
   const declaration = new Map();
   const ruleEditor = getRuleViewRuleEditor(view, ruleIndex);
 
   for (const currProp of ruleEditor.rule.textProps) {
     const icon = currProp.editor.unusedState;
     const unused = currProp.editor.element.classList.contains("unused");
-    const compatibilityData = currProp.isCompatible();
-    const compatibilityIcon = currProp.editor.compatibilityState;
+
+    let compatibilityData;
+    let compatibilityIcon;
+    if (addCompatibilityData) {
+      compatibilityData = currProp.isCompatible();
+      compatibilityIcon = currProp.editor.compatibilityState;
+    }
 
     declaration.set(`${currProp.name}:${currProp.value}`, {
       propertyName: currProp.name,
@@ -700,9 +711,13 @@ function getPropertiesForRuleIndex(view, ruleIndex) {
       data: currProp.isUsed(),
       warning: unused,
       used: !unused,
-      isCompatible: compatibilityData.then(data => data.isCompatible),
-      compatibilityData,
-      compatibilityIcon,
+      ...(addCompatibilityData
+        ? {
+            isCompatible: compatibilityData.then(data => data.isCompatible),
+            compatibilityData,
+            compatibilityIcon,
+          }
+        : {}),
     });
   }
 
@@ -822,11 +837,15 @@ async function checkDeclarationCompatibility(
   declaration,
   expected
 ) {
-  const declarations = getPropertiesForRuleIndex(view, ruleIndex);
+  const declarations = getPropertiesForRuleIndex(view, ruleIndex, true);
   const [[name, value]] = Object.entries(declaration);
   const dec = `${name}:${value}`;
   const { isCompatible, compatibilityData } = declarations.get(dec);
   const compatibilityStatus = await isCompatible;
+  // Await on compatibilityData even if the rule is
+  // compatible. Otherwise, the test browser may close
+  // while waiting for the response from the compatibility actor.
+  const messageId = (await compatibilityData).msgId;
 
   is(
     !expected,
@@ -834,10 +853,9 @@ async function checkDeclarationCompatibility(
     `"${dec}" has the correct compatibility status in the payload`
   );
 
-  if (expected) {
-    const messageId = (await compatibilityData).msgId;
-    is(messageId, expected, `"${dec}" has expected message ID`);
+  is(messageId, expected, `"${dec}" has expected message ID`);
 
+  if (expected) {
     await checkInteractiveTooltip(
       view,
       "compatibility-tooltip",
@@ -909,7 +927,11 @@ function checkDeclarationIsActive(view, ruleIndex, declaration) {
  */
 async function checkInteractiveTooltip(view, type, ruleIndex, declaration) {
   // Get the declaration
-  const declarations = getPropertiesForRuleIndex(view, ruleIndex);
+  const declarations = getPropertiesForRuleIndex(
+    view,
+    ruleIndex,
+    type === "compatibility-tooltip"
+  );
   const [[name, value]] = Object.entries(declaration);
   const dec = `${name}:${value}`;
 

devtools/client/inspector/shared/moz.build

@@ -5,6 +5,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 DevToolsModules(
+    'compatibility-user-settings.js',
     'highlighters-overlay.js',
     'node-reps.js',
     'node-types.js',

devtools/server/actors/compatibility/compatibility.js

@@ -4,34 +4,15 @@
 
 "use strict";
 
-const Services = require("Services");
 var protocol = require("devtools/shared/protocol");
 const { compatibilitySpec } = require("devtools/shared/specs/compatibility");
 
-loader.lazyRequireGetter(
-  this,
-  "browsersDataset",
-  "devtools/shared/browsers.json"
-);
-
 loader.lazyGetter(this, "mdnCompatibility", () => {
   const MDNCompatibility = require("devtools/server/actors/compatibility/lib/MDNCompatibility");
   const cssPropertiesCompatData = require("devtools/shared/compatibility/dataset/css-properties.json");
   return new MDNCompatibility(cssPropertiesCompatData);
 });
 
-const TARGET_BROWSER_ID = [
-  "firefox",
-  "firefox_android",
-  "chrome",
-  "chrome_android",
-  "safari",
-  "safari_ios",
-  "edge",
-];
-const TARGET_BROWSER_STATUS = ["esr", "current", "beta", "nightly"];
-const TARGET_BROWSER_PREF = "devtools.inspector.compatibility.target-browsers";
-
 const CompatibilityActor = protocol.ActorClassWithSpec(compatibilitySpec, {
   /**
    * Create a CompatibilityActor.
@@ -59,52 +40,21 @@ const CompatibilityActor = protocol.ActorClassWithSpec(compatibilitySpec, {
     this.inspector = null;
   },
 
-  _getDefaultTargetBrowsers() {
-    // Retrieve the information that matches to the browser id and the status
-    // from the browsersDataset.
-    // For the structure of then browsersDataset,
-    // see https://github.com/mdn/browser-compat-data/blob/master/browsers/firefox.json
-    const targets = [];
-
-    for (const id of TARGET_BROWSER_ID) {
-      const { name, releases } = browsersDataset[id];
-
-      for (const version in releases) {
-        const { status } = releases[version];
-
-        if (!TARGET_BROWSER_STATUS.includes(status)) {
-          continue;
-        }
-
-        // MDN compat data might have browser data that have the same id and status.
-        // e.g. https://github.com/mdn/browser-compat-data/commit/53453400ecb2a85e7750d99e2e0a1611648d1d56#diff-31a16f09157f13354db27821261604aa
-        // In this case, replace to the browser that has newer version to keep uniqueness
-        // by id and status.
-        const target = { id, name, version, status };
-        const index = targets.findIndex(
-          t => target.id === t.id && target.status === t.status
-        );
-
-        if (index < 0) {
-          targets.push(target);
-          continue;
-        }
-
-        const existingTarget = targets[index];
-        if (parseFloat(existingTarget.version) < parseFloat(target.version)) {
-          targets[index] = target;
-        }
-      }
-    }
-
-    return targets;
+  form() {
+    return {
+      actor: this.actorID,
+    };
   },
 
-  _getTargetBrowsers() {
-    const targetsString = Services.prefs.getCharPref(TARGET_BROWSER_PREF, "");
-    return targetsString
-      ? JSON.parse(targetsString)
-      : this._getDefaultTargetBrowsers();
+  getTraits() {
+    return {
+      traits: {
+        // Indicates the function for compatibility check exists
+        // This is to preserve backwards compatibility and can be
+        // removed once FF82 hits release channel
+        declarationBlockIssueComputationEnabled: true,
+      },
+    };
   },
 
   /**
@@ -117,9 +67,9 @@ const CompatibilityActor = protocol.ActorClassWithSpec(compatibilitySpec, {
    *      // Declaration value
    *      value: <string>,
    *    }
-   * @param object options
-   *  `targetBrowsers`: Array of target browsers to be used to check CSS compatibility against.
-   *     It is an Array of the following form
+   * @param array targetBrowsers
+   *  Array of target browsers to be used to check CSS compatibility against.
+   *  It is an Array of the following form
    *     {
    *       // Browser id as specified in `devtools/shared/compatibility/datasets/browser.json`
    *       id: <string>,
@@ -144,9 +94,7 @@ const CompatibilityActor = protocol.ActorClassWithSpec(compatibilitySpec, {
    *      unsupportedBrowsers: <Array>,
    *    }
    */
-  getCSSDeclarationBlockIssues: function(declarationBlock, options) {
-    const targetBrowsers =
-      (options && options.targetBrowsers) || this._getTargetBrowsers();
+  getCSSDeclarationBlockIssues: function(declarationBlock, targetBrowsers) {
     return mdnCompatibility.getCSSDeclarationBlockIssues(
       declarationBlock,
       targetBrowsers

devtools/server/actors/styles.js

@@ -1671,16 +1671,6 @@ var StyleRuleActor = protocol.ActorClassWithSpec(styleRuleSpec, {
         return decl;
       });
 
-      // Associate all the compatibility issues for the declarations with the
-      // form. Once Bug 1648339
-      // https://bugzilla.mozilla.org/show_bug.cgi?id=1648339
-      // is solved we can directly associate compatibility issue with the
-      // declaration themselves.
-      const compatibility = this.pageStyle.inspector.getCompatibility();
-      form.compatibilityIssues = compatibility.getCSSDeclarationBlockIssues(
-        declarations
-      );
-
       // Cache parsed declarations so we don't needlessly re-parse authoredText every time
       // we need to check previous property names and values when tracking changes.
       this._declarations = declarations;

devtools/shared/specs/compatibility.js

@@ -28,10 +28,26 @@ types.addDictType("compatibilityissues", {
   unsupportedBrowsers: "array:browsertype",
 });
 
+types.addDictType("declaration", {
+  name: "string",
+  value: "string",
+});
+
 const compatibilitySpec = generateActorSpec({
   typeName: "compatibility",
 
   methods: {
+    getTraits: {
+      request: {},
+      response: { traits: RetVal("json") },
+    },
+    getCSSDeclarationBlockIssues: {
+      request: {
+        declarationBlock: Arg(0, "array:declaration"),
+        targetBrowsers: Arg(1, "array:browsertype"),
+      },
+      response: RetVal("array:compatibilityissues"),
+    },
     getNodeCssIssues: {
       request: {
         node: Arg(0, "domnode"),