1462618 - The restyle root machinery could work a bit better.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

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 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1462618: The restyle root machinery could work better.

Consider the test-case where we have:

<div>
  <span id=a />
  <span id=b />
</div>

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.

Review commit: https://reviewboard.mozilla.org/r/245100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245100/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb463810b4026c0c9eb9fdcd5a738257fef245b3

Comment 3

[Marco Zehe (:MarcoZ)]

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

[Cameron McCormack (:heycam)]

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?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(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

[Cameron McCormack (:heycam)]

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?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8108248011
The restyle root machinery could work better. r=heycam

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/6c8108248011