The restyle root machinery could work a bit better.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

While debugging bug 1419903 I noticed that we were setting the restyle root to the document way more often than what we should.

This doesn't fix it of course, but it's an easy improvement.
Comment hidden (mozreview-request)
Priority: -- → P3
Triggered a Windows try build to see if this has any positive impact for accessibility, as I've seen some serious sluggishness on web applications like the new Gmail interface in Nightly recently. Want to investigate whether this patch improves things since too much restyling causes too many accessibles to be recreated which, in turn, causes Windows screen readers to rerender big parts of a virtual representation of the web content unnecessarily. The try build will appear here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e149ff29dc12d24337b417c517a8fb83a22e74e

Comment 4

a year ago
mozreview-review
Comment on attachment 8976950 [details]
Bug 1462618: The restyle root machinery could work better.

https://reviewboard.mozilla.org/r/245100/#review253214

::: commit-message-8b1c2:14
(Diff revision 1)
> +
> +We try to set one bit on "a", and a different one on "b".
> +
> +Ideally we'll end up with <div> as the root with both bits. But with the current
> +code we'd go all the way to the document unnecessarily. This fixes it by
> +checking the bit's we've propagated up to the top instead of existingBits.

bits

::: dom/base/Element.cpp:4391
(Diff revision 1)
>  static inline Element*
> -PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt)
> +PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt, uint32_t aBitsToStopAt)

Can we just check if any of kAllServoDescendantBits is set, rather than pass in aBitsToStopAt?
(In reply to Cameron McCormack (:heycam) from comment #4)
> Comment on attachment 8976950 [details]
> Bug 1462618: The restyle root machinery could work better.
> 
> https://reviewboard.mozilla.org/r/245100/#review253214
> 
> ::: commit-message-8b1c2:14
> (Diff revision 1)
> > +
> > +We try to set one bit on "a", and a different one on "b".
> > +
> > +Ideally we'll end up with <div> as the root with both bits. But with the current
> > +code we'd go all the way to the document unnecessarily. This fixes it by
> > +checking the bit's we've propagated up to the top instead of existingBits.
> 
> bits
> 
> ::: dom/base/Element.cpp:4391
> (Diff revision 1)
> >  static inline Element*
> > -PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt)
> > +PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt, uint32_t aBitsToStopAt)
> 
> Can we just check if any of kAllServoDescendantBits is set, rather than pass
> in aBitsToStopAt?

For the caller in NoteDirtyElement, yes, for the invalidation one, not, unfortunately.

Comment 6

a year ago
mozreview-review
Comment on attachment 8976950 [details]
Bug 1462618: The restyle root machinery could work better.

https://reviewboard.mozilla.org/r/245100/#review253240

::: dom/base/Element.cpp:4389
(Diff revision 1)
>  // Sets |aBits| on aElement and all of its flattened-tree ancestors up to and
>  // including aStopAt or the root element (whichever is encountered first).

Please document aBitsToStopAt in the comment here.

::: dom/base/Element.cpp:4528
(Diff revision 1)
>      Element* rootParent = existingRoot->GetFlattenedTreeParentElementForStyle();
> -    if (Element* commonAncestor = PropagateBits(rootParent, existingBits, aElement)) {
> +    if (Element* commonAncestor = PropagateBits(rootParent, existingBits, aElement, aBits)) {

Can you add a comment in here saying that we need to pass in aBits as that is how we detect the nearest common ancestor, or something like that?
Attachment #8976950 - Flags: review?(cam) → review+

Comment 7

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8108248011
The restyle root machinery could work better. r=heycam

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c8108248011
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.