1976876 - CompatibilityActor#getNodeCssIssues could be faster

Status: RESOLVED FIXED

(DevTools :: Inspector: Compatibility, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

The function is defined in https://searchfox.org/mozilla-central/rev/38e462fe13ea42ae6cc391fb36e8b9e82e842b00/devtools/server/actors/compatibility/compatibility.js#124-159

async getNodeCssIssues(node, targetBrowsers) {
  const pageStyle = await this.inspector.getPageStyle();
  const styles = await pageStyle.getApplied(node, {
    skipPseudo: false,
  });

  const declarationBlocks = styles.entries
    .map(({ rule }) => {
      // Replace form() with a function to get minimal subset
      // of declarations from StyleRuleActor when such a
      // function lands in the StyleRuleActor
      const declarations = rule.form().declarations;
      if (!declarations) {
        return null;
      }
      return declarations.filter(d => !d.commentOffsets);
    })
    .filter(declarations => declarations && declarations.length);

  return declarationBlocks
    .map(declarationBlock =>
      mdnCompatibility.getCSSDeclarationBlockIssues(
        declarationBlock,
        targetBrowsers
      )
    )
    .flat()
    .reduce((issues, issue) => {
      // Get rid of duplicate issue
      return issues.find(
        i => i.type === issue.type && i.property === issue.property
      )
        ? issues
        : [...issues, issue];
    }, []);
}

There are a few things that could be better:

• we're creating a lot of intermediary arrays (mapping, filtering, reducing, …)
• we're calling getCSSDeclarationBlockIssues for each matching rules, although we could build an array of declarations and only call it once
• we're filtering duplicated issues, but we could avoid having them in the first place by only pass one decoration per property name (since we're only checking the property name for compatibility issues)
• we're using StyleActor#form just to get the declarations, but there's overhead as it computed other data we won't use
• the declarations we're getting for the form also include the one in comments, which we then filter out (checking commentOffsets)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1976876 - [devtools] Refactor CompatibilityActor#getNodeCssIssues. r=#devtools.

The function was building multiple arrays to return a list of issues, which could
be costly.
We're now only build a list of declarations that we pass only once to getCSSDeclarationBlockIssues.
Since we're only checking the compatibility based on the property name, we don't
add declarations if we already added one in the array previously (instead of filtering
the list afterward to remove duplicated issues, like we were doing before).
Finally we extract a StyleRuleActor#parseRuleDeclarations method from what was
previously done in the StyleRuleActor form, and call that directly from getNodeCssIssues,
so we don't pay the cost for other data being computed in the form method, which
we wouldn't need here.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/36591e5f3a94
https://hg.mozilla.org/integration/autoland/rev/d7b09c631cab
[devtools] Refactor CompatibilityActor#getNodeCssIssues. r=devtools-reviewers,ochameau.

Comment 3

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/d7b09c631cab