Closed
Bug 1447506
Opened 7 years ago
Closed 7 years ago
Do LazyFC more often.
Categories
(Core :: Layout, enhancement, P3)
Core
Layout
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
I was going to do this today, but our build are busted now, bug 1447907.
Comment 7•7 years ago
|
||
C-C try https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f77333b11d7e0c1ec0aebcca96a43720d12666f based on https://hg.mozilla.org/try/rev/50608b448149e061973e6138c181d82ed965e089.
Flags: needinfo?(jorgk)
Comment 8•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 10•7 years ago
|
||
(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 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c094743582b2 https://hg.mozilla.org/mozilla-central/rev/cfbca76973a4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•