1418456 - stylo: Cleanup the XBL binding style resolution stuff.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

It's a bit of a mess, and it's not sound.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1418456: Cleanup the XBL stuff.

We not only need to care about children getting inserted in the flat tree, but
also about children moving _out_ of the flat tree.

In particular, as of right now we may leave stale data on elements when they
disappear from the flattened tree.

We're lucky enough that in 99% of the situations we enter in[1] and that clears
all the stuff, including servo data. But my assertions for bug 1414999 caught
the template / observes case.

Thus, just clear the whole bound element subtree data, and restyle it in the
end, no need for StyleNewChildren. This matches what we do for shadow DOM
(though in the shadow DOM case we do it async in DestroyFramesForAndRestyle).

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1418456: Remove the last call to StyleNewlyBoundElement.

If the element has no data, it's pointless to reconstruct frames on it, or post
a reconstruct. There's no need to particularly force to have data in this case.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1418456: Get rid of unstyled children only traversals.

They're useless now, provided we remove the hack to not traverse XBL-bound
elements on initial styling.

This also allows us to get rid of the fallback case.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

The first commit message is missing a link to https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/dom/xbl/nsXBLBinding.cpp#368

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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

Green!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ee31d60cf5e2de0b39e75a33f7d70d444b483d96
> 
> Green!

Well, there's an assert there, which is from the patch from bug 1414999, but bug 1418560 fixes it.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8929597 [details]
Bug 1418456: Cleanup the XBL stuff.

https://reviewboard.mozilla.org/r/200870/#review206264

::: commit-message-810fa:1
(Diff revision 1)
> +Bug 1418456: Cleanup the XBL stuff. r?heycam

s/Cleanup the XBL stuff/Clear subtree element data when applying an XBL binding/ or something like that?

::: commit-message-810fa:9
(Diff revision 1)
> +also about children moving _out_ of the flat tree.
> +
> +In particular, as of right now we may leave stale data on elements when they
> +disappear from the flattened tree.
> +
> +We're lucky enough that in 99% of the situations we enter in[1] and that clears

Missing footnote?

::: dom/xbl/nsXBLService.cpp:385
(Diff revision 1)
>  // RAII class to invoke StyleNewChildren for Elements in Servo-backed documents
>  // on destruction.

Update this comment?

::: dom/xbl/nsXBLService.cpp:447
(Diff revision 1)
>      return NS_OK;
>    }
>  
>    // There are various places in this function where we shuffle content around
>    // the subtree and rebind things to and from insertion points. Once all that's
>    // done, we want to invoke StyleNewChildren to style any unstyled children

And this comment.

::: layout/style/ServoStyleSet.h:184
(Diff revision 1)
>    // Clear style data and resolve style for the given element and its
> -  // subtree for changes to -moz-binding. It returns the new style
> +  // subtree for changes to -moz-binding.

This comment needs updating too.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8929598 [details]
Bug 1418456: Remove the last call to StyleNewlyBoundElement.

https://reviewboard.mozilla.org/r/200872/#review206272

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8929599 [details]
Bug 1418456: Get rid of unstyled children only traversals.

https://reviewboard.mozilla.org/r/200874/#review206274

::: layout/base/nsCSSFrameConstructor.cpp:7498
(Diff revision 1)
> -    // already handled that parent.  In the common case of inserting elements
> -    // into a container that does not have an XBL binding or shadow tree with
> -    // distributed children, this boils down to a single call to
> -    // GetFlattenedTreeParent/StyleNewChildren, and traversing the list of
> -    // children checking HasServoData (which is fast).
>      if (child->IsElement() && !child->AsElement()->HasServoData()) {

Do we still need the HasServoData check now?

::: layout/style/ServoTypes.h:66
(Diff revision 1)
> -  UnstyledOnly = 1 << 2,
>    // A forgetful traversal ignores the previous state of the frame tree, and
>    // thus does not compute damage or maintain other state describing the styles
>    // pre-traversal. A forgetful traversal is usually the right thing if you
>    // aren't going to do a post-traversal.
>    Forgetful = 1 << 3,

Feel free to re-number the other values...

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/391937d9f0d7
Clear subtree element data when applying an XBL binding. r=heycam
https://hg.mozilla.org/integration/autoland/rev/24dea2c09495
Remove the last call to StyleNewlyBoundElement. r=heycam
https://hg.mozilla.org/integration/autoland/rev/5e9db66be7d4
Get rid of unstyled children only traversals. r=heycam

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae3583071976
followup: undo last-minute cleanup that busts the build. r=me

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e254d702b054
followup: Re-add the HasServoData check because I was too optimistic addressing review comments. r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66261eec36b3
Yet another followup: Always re-get the style context in the servo case after loading an XBL binding to avoid tripping otherwise useful assertions. r=me

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #9)
> Comment on attachment 8929599 [details]
> Bug 1418456: Get rid of unstyled children only traversals.
> 
> https://reviewboard.mozilla.org/r/200874/#review206274
> 
> ::: layout/base/nsCSSFrameConstructor.cpp:7498
> (Diff revision 1)
> > -    // already handled that parent.  In the common case of inserting elements
> > -    // into a container that does not have an XBL binding or shadow tree with
> > -    // distributed children, this boils down to a single call to
> > -    // GetFlattenedTreeParent/StyleNewChildren, and traversing the list of
> > -    // children checking HasServoData (which is fast).
> >      if (child->IsElement() && !child->AsElement()->HasServoData()) {
> 
> Do we still need the HasServoData check now?

And just to reply here, yes, we do, because we can reenter in ContentRangeInserted from ContentAppended. We can rejigger the code a bit to avoid this though.

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/391937d9f0d7
https://hg.mozilla.org/mozilla-central/rev/24dea2c09495
https://hg.mozilla.org/mozilla-central/rev/5e9db66be7d4
https://hg.mozilla.org/mozilla-central/rev/ae3583071976
https://hg.mozilla.org/mozilla-central/rev/e254d702b054
https://hg.mozilla.org/mozilla-central/rev/66261eec36b3