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

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

+++ 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).

Comment 1

[alexander :surkov (:asurkov)]

Attachment: Bug 1528268 - rework Custom Elements inheritedAttributes, r=bgrins

Comment 2

[alexander :surkov (:asurkov)]

(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

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1528268 - Make initializeAttributeInheritance and incremental attribute changes do less work

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8770d7eebd1
Make initializeAttributeInheritance and incremental attribute changes do less work r=aswan

Comment 5

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1519502, bug 1528268) for Crashtest failures in toolkit/content/tests/chrome/test_popupincontent.xul. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241127128&repo=autoland&lineNumber=4281

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=904cc7903feb74eeb70b415403fa33f8df4c39ff

Backout:
https://hg.mozilla.org/integration/autoland/rev/90a95d56b18df1b2a658dd8b156486e31b3ee0ec

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9ffb38259ec
Make initializeAttributeInheritance and incremental attribute changes do less work r=aswan

Comment 7

[Arthur Iakab [arthur_iakab]]

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

Comment 8

[Intermittent Failures Robot]

3 failures in 2673 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 3

Platform breakdown:

• linux32: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1528268&startday=2019-04-15&endday=2019-04-21&tree=all

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 10

[Brian Grinstead [:bgrins]]

(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.