Simplify NAC and UA widget setup (make UA widgets NAC).
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox113 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(6 files, 1 obsolete file)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
This prevents the NAC -> not-nac -> NAC situation that bug 1824886 causes, and simplifies a bit the code.
Assignee | ||
Comment 1•1 year ago
|
||
Make all UA widgets also NAC.
Keep the UA widget flag but break at anonymous subtree boundaries, so
that only nodes inside the UA widget directly (and not NAC from those)
get the flag.
This is important because two callers depend on this difference:
-
The style system, since we still want to match content rules from
stylesheets in the UA widget. We also match user rules, which is a
bit sketchy, but that was the previous behavior, will file a
follow-up for that. -
The reflector code, since we want the scope for UA widgets to not
include the NAC nodes inside that UA widget. nsINode::IsInUAWidget
got it wrong.
After this patch, ChromeOnlyAccess is equivalent to
IsInNativeAnonymousSubtree, so we should probably unify the naming.
That's left for a follow-up patch because I don't have a strong
preference.
Assignee | ||
Comment 2•1 year ago
|
||
We can just use the node flag.
Depends on D174310
Assignee | ||
Comment 3•1 year ago
|
||
This doesn't change behavior but it's simpler. I found it while
debugging a failure with the previous patches.
Depends on D174316
Assignee | ||
Comment 4•1 year ago
|
||
To be clear the current behavior is worth preserving and not a bug, it just
happens that my previous patch breaks it subtly.
These patches on their own don't change behavior but they fail tests from the
precious changes.
The TLDR is that when using the standard walker we don't want to ignore all
anonymous children, just anonymous roots. This is important because now that
ShadowRoots can be anonymous roots, we were skipping their children altogether.
Also, don't return anonymous children that are really part of the shadow tree
and which now show up as anonymous because of that.
Depends on D174365
Assignee | ||
Updated•1 year ago
|
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aaf77a7d75ad Remove ShadowRoot::mIsUAWidget. r=smaug https://hg.mozilla.org/integration/autoland/rev/2f4d85a2b343 Simplify inspector node walking. r=smaug
Assignee | ||
Comment 6•1 year ago
|
||
It's always SHOW_ALL, and it doesn't work anyways, see the treewalker impl.
Depends on D174366
Assignee | ||
Comment 7•1 year ago
|
||
The tree devtools uses is the light dom + pseudo-elements + NAC, but sometimes
it wants to know stuff about the flat tree like assigned nodes. Previously it
was using a weird mix of the anonymous vs. non-anonymous walkers to get what it
wants, but that's needlessly complicated.
Instead, make InspectorUtils.getChildrenForNode do the right thing, and add
assigned nodes explicitly.
While _getChildren using a walker might seem like a good idea for performance,
realistically it was using InspectorUtils under the hood, and this is much
simpler.
Depends on D174490
Comment 8•1 year ago
|
||
bugherder |
Updated•1 year ago
|
Assignee | ||
Comment 9•1 year ago
|
||
Split out as per request in the other patches.
Comment 10•1 year ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b5e6c3cbe9a4 Remove unused whatToShow option from devtools inspector walker. r=devtools-reviewers,devtools-backward-compat-reviewers,nchevobbe
Comment 11•1 year ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a57ee7983d28 Remove unused scrollbarTreeWalkerFilter. r=nchevobbe,devtools-reviewers
Comment 12•1 year ago
|
||
bugherder |
Assignee | ||
Updated•1 year ago
|
Comment 13•1 year ago
|
||
Comment on attachment 9326667 [details]
Bug 1825825 - Simplify DevTools' walker. r=#devtools-reviewers
Revision D174491 was moved to bug 1826517. Setting attachment 9326667 [details] to obsolete.
Comment 14•1 year ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0d254cc9e9c4 Simplify NAC setup. r=smaug https://hg.mozilla.org/integration/autoland/rev/4d6b19e4c8bd Tweak DevTools walker to preserve behavior after the previous patches. r=devtools-reviewers,nchevobbe
Comment 15•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d254cc9e9c4
https://hg.mozilla.org/mozilla-central/rev/4d6b19e4c8bd
Comment 16•1 year ago
|
||
== Change summary for alert #38031 (as of Fri, 07 Apr 2023 18:06:18 GMT) ==
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
9% | damp inspector.mutations | linux1804-64-shippable-qr | e10s fission stylo webrender | 1,079.14 -> 977.24 |
9% | damp complicated.inspector.close.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender | 17.31 -> 15.77 |
8% | damp inspector.mutations | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 1,071.85 -> 982.09 |
8% | damp inspector.mutations | windows10-64-shippable-qr | e10s fission stylo webrender | 832.98 -> 765.02 |
5% | damp custom.inspector.collapseall.balanced | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 8.56 -> 8.14 |
... | ... | ... | ... | ... |
3% | damp custom.inspector.collapseall.manychildren | windows10-64-shippable-qr | e10s fission stylo webrender | 0.63 -> 0.61 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38031
Description
•