1590573 - Unify how we cache fragments with parseXULToFragment in our Custom Elements

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Brian Grinstead [:bgrins]]

We currently have a few different patterns for caching fragments. At this point we should migrate our chrome custom elements to use the same one.

A bunch of examples can be found from callers https://searchfox.org/mozilla-central/search?q=parseXULToFragment&case=true&regexp=false, and usually actually happens with a call to importNode or cloneNode:

• https://searchfox.org/mozilla-central/search?q=importNode&case=true&regexp=false&path=toolkit%2Fcontent%2Fwidgets
• https://searchfox.org/mozilla-central/search?q=cloneNode&case=true&regexp=false&path=toolkit%2Fcontent%2Fwidgets

The latest elements have been doing something like: https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/toolkit/content/widgets/arrowscrollbox.js#143-150

    get fragment() {
      if (!this.constructor.hasOwnProperty("_fragment")) {
        this.constructor._fragment = MozXULElement.parseXULToFragment(
          this.markup
        );
      }
      return document.importNode(this.constructor._fragment, true);
    }

which is good because it caches once per class and also supports subclasses (using hasOwnProperty instead of just checking this.constructor["_fragment"]).

There are also ideas around caching it per-process and not per window (in a JSM instead of in a Custom Element definition). That may or may not happen here but worth keeping in mind: https://phabricator.services.mozilla.com/D50137#1521553.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Given the improvements reported in bug 1590554 comment 7, I suspect this is worth doing and could have a positive impact on startup times.

Comment 2

[Emma Malysz]

Attachment: Bug 1590573, create a unified approach for caching fragments in our Custom Elements

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b7c81561ecc
create a unified approach for caching fragments in our Custom Elements r=emilio

Comment 4

[Razvan Maries]

Backed out for marionette perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=296738238&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C10%2Cx64%2Cdebug%2Ctest-windows10-64%2Fdebug-marionette-e10s%2C%28mn%29&revision=6b7c81561ecc6e8d4e13890da92768e7edce0e55

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296738238&repo=autoland&lineNumber=69973

Backout: https://hg.mozilla.org/integration/autoland/rev/33c92ceddf3989a00f40152c15e555989e48b704

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70b0b35496a5
create a unified approach for caching fragments in our Custom Elements r=emilio

Comment 6

[Emma Malysz]

Attachment: Bug 1590573 - Followup: Revert changes on elements that don't use MozXULElement.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ccafa875240
Followup: Revert changes on elements that don't use MozXULElement. CLOSED TREE

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/70b0b35496a5
https://hg.mozilla.org/mozilla-central/rev/4ccafa875240