Closed Bug 1528268 Opened 8 months ago Closed 6 months ago

Limit calls to `inheritAttribute` when using `inheritedAttributes` helper with MozElement Custom Elements

Categories

(Toolkit :: XUL Widgets, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1527680 +++

In https://phabricator.services.mozilla.com/D19702#524381 Paolo pointed out that we could drop the caching added in Bug 1523429 (to prevent overriding attrs that were unchanged on the host but had changed on the child).

Once we add the helper in Bug 1527680 we could change the behavior such that inheritAttribute is less smart, and the helper keeps track of when it needs to do a full sync (on initialization) vs only a changed attr (on attributeChangedCallback).

Priority: -- → P5

(In reply to alexander :surkov (:asurkov) from comment #1)

Created attachment 9050668 [details]
Bug 1528268 - rework Custom Elements inheritedAttributes, r=bgrins

I doesn't show any good perf numbers for about_preferences_basic [1] I hoped to get for checkbox pref regression bug 1532651, by getting rid of early access to DOM (which forces <label> binding constructed), but the patch might be useful for this bug.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=755c2fee7975f63b02d21ce6573ae3075bed31c6&newProject=try&newRevision=1a42023fb431559df49900f6fc07c1276d4644ea&framework=1&showOnlyImportant=1

This provides a flipped data structure based on the provided inheritedAttributes,
which looks like:

Object<selector, attrs_to_inherit_comma_separated>

To one that looks like:

Object<attr, Array<Array<selector, attr_to_inherit>>

This should improve performance because:

  1. We only fetch element at connectedCallback that actually will have attributes inherited.
  2. When an attribute changes we can quickly inherit only that one.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8770d7eebd1
Make initializeAttributeInheritance and incremental attribute changes do less work r=aswan
Attachment #9050668 - Attachment is obsolete: true
Blocks: 1519502
Blocks: 1538983
Flags: needinfo?(bgrinstead)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9ffb38259ec
Make initializeAttributeInheritance and incremental attribute changes do less work r=aswan
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1545765
Depends on: 1545865
Regressions: 1546024
Regressions: 1546036
Regressions: 1546060

Given this many regressions that QA found in Nightly, is there more testing we could ask for here? Any added automated tests?

Flags: needinfo?(bgrinstead)

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #9)

Given this many regressions that QA found in Nightly, is there more testing we could ask for here? Any added automated tests?

A lot of these were duplicates of Bug 1546024, which did add automated test cases to toolkit/content/tests/chrome/test_custom_element_base.xul: https://hg.mozilla.org/mozilla-central/rev/4bc97c0629fa#l2.2. Bug 1545765 also added automated test cases to the same file: https://hg.mozilla.org/mozilla-central/rev/c9e14c54335a#l2.2.

Since this change was affecting the shared base class for our Custom Elements across the UI, the downside is that it's hard to point at any single place in the UI for additional manual testing. The upside is that the landed fixes apply across the entire UI. If we could get manual QA verification on all the regression bugs (even the ones that were duped), we should be in a pretty good place.

Flags: needinfo?(bgrinstead)
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.