1447506 - Do LazyFC more often.

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Servo knows how to walk anon nodes.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447506: Do LazyFC for anonymous nodes too, and cleanup MaybeConstructLazily.

Servo knows how to walk through them via StyleChildrenIterator, so no reason
this is not done for them. The old style system only used FlattenedTreeIterator,
so couldn't find them.

Also, not check the container to do lazy fc on the children, it makes no sense.
The only reason we don't do lazy frame construction in XUL is because of XBL
bindings, and those are loaded for the parent already, if we're about to
construct frames for the children.

Also, assert more tightly, we don't insert NAC lazily, that makes no sense.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Make Devtools' TableWidget tests not rely on XBL bindings getting applied synchronously when XUL is mixed with HTML.

So TableWidget's columns does something where they mix HTML and XUL:

  https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/devtools/client/shared/widgets/TableWidget.js#1055

The label's .value is an XBL binding property:

  https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/toolkit/content/widgets/text.xml#19

This tests expects the XBL binding for the <label> to be applied synchronously,
which the other patch in this bug prevents.

Is there a reason that is a XUL label btw? We don't seem to use the binding
methods at all (except in this test, lol).

Anyway, just use getAttribute("label") in the test instead.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8960830 [details]
Bug 1447506: Do LazyFC for anonymous nodes too, and cleanup MaybeConstructLazily.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229572/diff/1-2/

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8960845 [details]
Make Devtools' TableWidget tests not rely on XBL bindings getting applied synchronously when XUL is mixed with HTML.

https://reviewboard.mozilla.org/r/229596/#review235642

Re: why we use <xul:label> - as you point it's not for the binding methods but is most likely for the layout. Mixing html and xul layouts is mysterious and causes hard to predict results with overflowing and flexing. Also, the fact that the table widget relying on XUL means most likely we'll replace it with a different shared widget whenever the storage UI moves to HTML. So, I'm OK with a narrow fix to the test instead of trying to move away from xul label entirely.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

PSA, this may break comm-central stuff if they mix HTML and XUL, which they do IIRC.

I'm not on a rush to land this, but if any of you could do a comm-central try run / try the patch locally to ensure there's no breakage / there isn't much it'd be appreciated.

Comment 6

[Jorg K (CEST = GMT+2)]

I was going to do this today, but our build are busted now, bug 1447907.

Comment 7

[Jorg K (CEST = GMT+2)]

C-C try
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f77333b11d7e0c1ec0aebcca96a43720d12666f

based on https://hg.mozilla.org/try/rev/50608b448149e061973e6138c181d82ed965e089.

Comment 8

[Jorg K (CEST = GMT+2)]

No visible failures. Windows builds are still busted, I'll check TB with that patch when they come back. No review for the patch yet, I noticed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8960830 [details]
Bug 1447506: Do LazyFC for anonymous nodes too, and cleanup MaybeConstructLazily.

https://reviewboard.mozilla.org/r/229572/#review235916

r=me modulo the comments about the commit message and the question about the assert in the insert case.

::: commit-message-ca154:1
(Diff revision 2)
> +Bug 1447506: Do LazyFC for anonymous nodes too, and cleanup MaybeConstructLazily. r?bz

"clean up"

::: commit-message-ca154:3
(Diff revision 2)
> +Bug 1447506: Do LazyFC for anonymous nodes too, and cleanup MaybeConstructLazily. r?bz
> +
> +Servo knows how to walk through them via StyleChildrenIterator, so no reason

How about:

  The old style system used FlattenedTreeIterator for lazy frame construction.  That could not find native anonymous nodes, so we had to construct eagerly in native anonymous subtrees.  Servo uses StyleChildrenIterator for the same purpose, and that knows about native anonymous content, so we can now do lazy construction for it.

::: commit-message-ca154:7
(Diff revision 2)
> +
> +Servo knows how to walk through them via StyleChildrenIterator, so no reason
> +this is not done for them. The old style system only used FlattenedTreeIterator,
> +so couldn't find them.
> +
> +Also, not check the container to do lazy fc on the children, it makes no sense.

It made sense when the lazy FC walk could not reach native anon content!  Inserts under that content could not be lazy, because they would never be reached.  That's the anon content check on the parent.

The XUL check on the parent... maybe that was just abundance of caution.  This probably handled mixed XUL-in-HTML-in-XUL cases, though even there it would fall down if you had XUL-in-HTML-in-HTML-in-XUL and did the insert on the inner HTML into the outer HTML...

Anyway, the check for XUL container is probably OK to remove, but we should just say explicitly in the commit message that we're doing that.

::: commit-message-ca154:8
(Diff revision 2)
> +Servo knows how to walk through them via StyleChildrenIterator, so no reason
> +this is not done for them. The old style system only used FlattenedTreeIterator,
> +so couldn't find them.
> +
> +Also, not check the container to do lazy fc on the children, it makes no sense.
> +The only reason we don't do lazy frame construction in XUL is because of XBL

Is XBL the only reason?  At one point there were weird XUL frame classes that had other dependencies, but maybe they're gone now.

::: commit-message-ca154:15
(Diff revision 2)
> +construct frames for the children.
> +
> +Also, assert more tightly, we don't insert NAC lazily, that makes no sense.
> +
> +I can bring back the XUL container check if you want, though the devtools test
> +for that widget is the only thing that broke, and I'd prefer to remove it.

Which "that widget"?  As a future reader of this commmit message that doesn't make much sense...

I think this whole last paragraph could probably go away.

::: layout/base/nsCSSFrameConstructor.cpp:6933
(Diff revision 2)
> -      aContainer->IsXULElement()) {
> -    return false;
> -  }
> -
>    if (aOperation == CONTENTINSERT) {
> -    if (aChild->IsRootOfAnonymousSubtree() || aChild->IsXULElement()) {
> +    MOZ_ASSERT(!aChild->IsRootOfAnonymousSubtree());

Hmm.  How does this hold given the games editor plays with native anon content?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> ::: layout/base/nsCSSFrameConstructor.cpp:6933
> (Diff revision 2)
> > -      aContainer->IsXULElement()) {
> > -    return false;
> > -  }
> > -
> >    if (aOperation == CONTENTINSERT) {
> > -    if (aChild->IsRootOfAnonymousSubtree() || aChild->IsXULElement()) {
> > +    MOZ_ASSERT(!aChild->IsRootOfAnonymousSubtree());
> 
> Hmm.  How does this hold given the games editor plays with native anon
> content?

The editor goes through PostRestyle(ReconstructFrame) instead of ContentInserted / ContentAppended. I also wish we didn't have two different ways of doing this :)

Comment 11

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c094743582b2
Make Devtools' TableWidget tests not rely on XBL bindings getting applied synchronously when XUL is mixed with HTML. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbca76973a4
Do LazyFC for anonymous nodes too, and clean up MaybeConstructLazily. r=bz

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c094743582b2
https://hg.mozilla.org/mozilla-central/rev/cfbca76973a4