Closed Bug 1447506 Opened 2 years ago Closed 2 years ago

Do LazyFC more often.

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

Servo knows how to walk anon nodes.
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.
Attachment #8960845 - Flags: review?(bgrinstead) → review+
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.
Flags: needinfo?(jorgk)
I was going to do this today, but our build are busted now, bug 1447907.
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 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?
Attachment #8960830 - Flags: review?(bzbarsky) → review+
(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 :)
[Triage 2018/03/23 - P3]
Priority: -- → P3
Pushed by ecoal95@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/c094743582b2
https://hg.mozilla.org/mozilla-central/rev/cfbca76973a4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.