Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2025543002:
Fix when orthogonal writing mode roots have floating siblings (Closed)
DescriptionFix when orthogonal writing mode roots have floating siblings
When orthogonal writing mode roots have floating siblings, its
containing block may still have old or even deleted LayoutObjects.
This occurs when LayoutMultiColumnFlowThread::populate(),
LayoutBoxModelObject::moveChildrenTo() with !fullRemoveInsert,
or more, for the optimization purposes.
This patch clears such objects to be re-created when the containing
block is laid out.
BUG=604095, 633409, 646178
Committed: https://crrev.com/5c43382692e6b687373576984181e52c30ccd142
Cr-Commit-Position: refs/heads/master@{#419376}
Patch Set 1 #Patch Set 2 : Fix removeFloatingOrPositionedChildFromBlockLists #
Total comments: 1
Patch Set 3 : 3rd option #
Total comments: 2
Patch Set 4 : Approach 3, with more restrictive condition #Patch Set 5 : Approach 1, restrict the condition more, add comments #Patch Set 6 : Update comments as per robhogan review #
Messages
Total messages: 69 (37 generated)
Description was changed from ========== orthogonal-floats BUG= ========== to ========== Fix when orthgonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even detached LayoutObjects. This patch clears such objects to be re-created when the containing block layouts. BUG=604095, 613869 ==========
kojii@chromium.org changed reviewers: + robhogan@chromium.org
PTAL. A few notes: 1. I'm not sure whether it's ok just to removeFloatingObjects(). Should it safely re-created when the cb is laid out? 2. Maybe we should setNeedsLayout(cb) in case only the orthogonal root was invalidated? Or that will not happen since we setNeedsLayout its containing block automatically? 3. I'm not sure whether this should also apply to subtree root or not. Looking at the code quickly, inline blocks are unlikely to be subtree roots, but not really certain.
Description was changed from ========== Fix when orthgonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even detached LayoutObjects. This patch clears such objects to be re-created when the containing block layouts. BUG=604095, 613869 ========== to ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even detached LayoutObjects. This patch clears such objects to be re-created when the containing block layouts. BUG=604095, 613869 ==========
Description was changed from ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even detached LayoutObjects. This patch clears such objects to be re-created when the containing block layouts. BUG=604095, 613869 ========== to ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This patch clears such objects to be re-created when the containing block layouts. BUG=604095, 613869 ==========
Description was changed from ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This patch clears such objects to be re-created when the containing block layouts. BUG=604095, 613869 ========== to ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This patch clears such objects to be re-created when the containing block is laid out. BUG=604095, 613869 ==========
kojii@chromium.org changed reviewers: + eae@chromium.org
+eae@, does this look right?
On 2016/05/29 at 17:05:53, kojii wrote:
> PTAL.
>
> A few notes:
> 1. I'm not sure whether it's ok just to removeFloatingObjects(). Should it
safely re-created when the cb is laid out?
> 2. Maybe we should setNeedsLayout(cb) in case only the orthogonal root was
invalidated? Or that will not happen since we setNeedsLayout its containing
block automatically?
> 3. I'm not sure whether this should also apply to subtree root or not. Looking
at the code quickly, inline blocks are unlikely to be subtree roots, but not
really certain.
I'd prefer if you isolated the reason why the deleted float has not already been
removed from the containing block's float list. It should have been if it was
deleted. See LayoutObjectChildList::removeChildNode():
LayoutObject* LayoutObjectChildList::removeChildNode(LayoutObject* owner,
LayoutObject* oldChild, bool notifyLayoutObject)
{
ASSERT(oldChild->parent() == owner);
ASSERT(this == owner->virtualChildren());
if (oldChild->isFloatingOrOutOfFlowPositioned())
toLayoutBox(oldChild)->removeFloatingOrPositionedChildFromBlockLists();
On 2016/05/30 at 18:10:25, robhogan wrote:
> On 2016/05/29 at 17:05:53, kojii wrote:
> > PTAL.
> >
> > A few notes:
> > 1. I'm not sure whether it's ok just to removeFloatingObjects(). Should it
safely re-created when the cb is laid out?
> > 2. Maybe we should setNeedsLayout(cb) in case only the orthogonal root was
invalidated? Or that will not happen since we setNeedsLayout its containing
block automatically?
> > 3. I'm not sure whether this should also apply to subtree root or not.
Looking at the code quickly, inline blocks are unlikely to be subtree roots, but
not really certain.
>
> I'd prefer if you isolated the reason why the deleted float has not already
been removed from the containing block's float list. It should have been if it
was deleted. See LayoutObjectChildList::removeChildNode():
>
> LayoutObject* LayoutObjectChildList::removeChildNode(LayoutObject* owner,
LayoutObject* oldChild, bool notifyLayoutObject)
> {
> ASSERT(oldChild->parent() == owner);
> ASSERT(this == owner->virtualChildren());
>
> if (oldChild->isFloatingOrOutOfFlowPositioned())
>
toLayoutBox(oldChild)->removeFloatingOrPositionedChildFromBlockLists();
Oh, thanks for the pointer, I was looking for where we clear without finding it.
I'll look into removeFloatingOrPositionedChildFromBlockLists().
But it does not solve 604095 where LayoutObject was not removed and is not
dirtied properly, correct? Is my understanding that we dirty and re-create the
list when containing block layouts incorrect?
On 2016/05/30 at 18:22:04, kojii wrote: > But it does not solve 604095 where LayoutObject was not removed and is not dirtied properly, correct? Yes. > Is my understanding that we dirty and re-create the list when containing block layouts incorrect? No it's correct. It gets recreated at layout. As long as your writing mode root's containing block needs layout it should be safe to clear down it's float list before laying out the root. You would need to check for/assert that here. For 604095 we need to understand better what the right behaviour should be. Per my comment at https://bugs.chromium.org/p/chromium/issues/detail?id=604095#c8 I still think it does need to avoid the float, even though its not in the same formatting context.
On 2016/05/30 at 18:22:04, kojii wrote:
> On 2016/05/30 at 18:10:25, robhogan wrote:
> > I'd prefer if you isolated the reason why the deleted float has not already
been removed from the containing block's float list. It should have been if it
was deleted. See LayoutObjectChildList::removeChildNode():
> >
> > LayoutObject* LayoutObjectChildList::removeChildNode(LayoutObject* owner,
LayoutObject* oldChild, bool notifyLayoutObject)
> > {
> > ASSERT(oldChild->parent() == owner);
> > ASSERT(this == owner->virtualChildren());
> >
> > if (oldChild->isFloatingOrOutOfFlowPositioned())
> >
toLayoutBox(oldChild)->removeFloatingOrPositionedChildFromBlockLists();
>
> Oh, thanks for the pointer, I was looking for where we clear without finding
it. I'll look into removeFloatingOrPositionedChildFromBlockLists().
That was easy. In this case, the containingBlock is LayoutView(), and
removeFloatingOrPositionedChildFromBlockLists() does not handle such case
correctly. Fixed in PS2.
On 2016/05/30 at 18:39:36, robhogan wrote:
> For 604095 we need to understand better what the right behaviour should be.
Per my comment at
https://bugs.chromium.org/p/chromium/issues/detail?id=604095#c8 I still think it
does need to avoid the float, even though its not in the same formatting
context.
I have to admit that I don't fully understand your https://crbug.com/604095#c8
comment, sorry. I suppose either of us (or both of us?) misunderstand something
but can't figure out what it is. Let's get back to crbug for this, I need to
attach a screenshot to understand better.
robhogan@, did the screenshot answered to your question, or do I still not understand your concerns? I came to guess that you may assume that once orthogonal roots were laid out, they won't move, and you're worried about overlap because of wrong positions. If it is the case, orthogonal roots layout can be re-layout if needed, such as cases like this. The purpose of orthogonal roots layout pass is to compute their heights before we compute preferred logical widths. They can then be re-layout or moved as we do to other LayoutObjects. But without this step, orthogonal roots compute their preferred logical width to zero (because heights are zero before layout.) In this case, we do need preferred logical width, so we can't exclude them from orthogonal roots layout pass. If I still misunderstand your concern, I'd appreciate you to point out what I'm missing. Thanks!
robhogan@gmail.com changed reviewers: + robhogan@gmail.com
https://codereview.chromium.org/2025543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2025543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:1787: toLayoutBlockFlow(cb)->removeFloatingObjects(); Per my note on the bug, we should skip orthogonal layout on the child in this situation and let main layout look after it. Removing floats is expensive. (Also, you would only want to do it when cb needsLayout().) The thing causing us a headache here is that the writing mode on both child and container have been made orthogonal but neither have had a layout in their new writing modes yet. Detecting this situation in layoutOrthogonalWritingModeRoots() and skipping orthogonal layout on affected roots would be ideal - they will get relayout in main layout anyway so it will be less work overall too. Can you look at what would be involved in skipping orthogonal layout on the child? That is definitely the better solution here.
On 2016/06/06 at 09:41:02, robhogan wrote: > https://codereview.chromium.org/2025543002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2025543002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.cpp:1787: toLayoutBlockFlow(cb)->removeFloatingObjects(); > Per my note on the bug, we should skip orthogonal layout on the child in this situation and let main layout look after it. > > Removing floats is expensive. (Also, you would only want to do it when cb needsLayout().) > > The thing causing us a headache here is that the writing mode on both child and container have been made orthogonal but neither have had a layout in their new writing modes yet. Detecting this situation in layoutOrthogonalWritingModeRoots() and skipping orthogonal layout on affected roots would be ideal - they will get relayout in main layout anyway so it will be less work overall too. > > Can you look at what would be involved in skipping orthogonal layout on the child? That is definitely the better solution here. I can try, but what you suggested will make orthogonal layout incorrect if preferred width is used; e.g., fit-content. I understand orthogonal layouts is not cheap, rather expensive, but that is the only way to do the correct layout AFAIU. If you have suggestions to make it faster while still doing correct layout, that'd be great. I'd be happy to review your patch then.
On 2016/06/06 at 11:01:56, kojii wrote: > On 2016/06/06 at 09:41:02, robhogan wrote: > > https://codereview.chromium.org/2025543002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > > > https://codereview.chromium.org/2025543002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/FrameView.cpp:1787: toLayoutBlockFlow(cb)->removeFloatingObjects(); > > Per my note on the bug, we should skip orthogonal layout on the child in this situation and let main layout look after it. > > > > Removing floats is expensive. (Also, you would only want to do it when cb needsLayout().) > > > > The thing causing us a headache here is that the writing mode on both child and container have been made orthogonal but neither have had a layout in their new writing modes yet. Detecting this situation in layoutOrthogonalWritingModeRoots() and skipping orthogonal layout on affected roots would be ideal - they will get relayout in main layout anyway so it will be less work overall too. > > > > Can you look at what would be involved in skipping orthogonal layout on the child? That is definitely the better solution here. > > I can try, but what you suggested will make orthogonal layout incorrect if preferred width is used; e.g., fit-content. Won't orthogonal layout be incorrect in this situation anyway? It won't avoid the floats it needs to avoid because their correct position isn't known. That will get sorted out in main layout when it shrinks to avoid them. Do you have a test case for what you mean? > I understand orthogonal layouts is not cheap, rather expensive, but that is the only way to do the correct layout AFAIU. If you have suggestions to make it faster while still doing correct layout, that'd be great. I'd be happy to review your patch then. I don't plan to work on this. The float-clearing approach doesn't look right to me but maybe it's necessary. I will defer to an OWNER on that.
On 2016/06/06 at 11:16:06, robhogan wrote: > Won't orthogonal layout be incorrect in this situation anyway? It won't avoid the floats it needs to avoid because their correct position isn't known. That will get sorted out in main layout when it shrinks to avoid them. No, as far as I observed. Can you look at the screenshot in https://crbug.com/604095#c15 and identify how it's layout is incorrect? > Do you have a test case for what you mean? If you apply 'display: inline-block' to the parent of the orthogonal flow, the layout depends on preferred width of the orthogonal flow. If we don't layout orthogonal roots first, computePreferredWidth() will return 0.
BTW, if you object to this CL, I don't think it's a good idea to ask OWNERs to override it. I think OWNERs are there when area experts are happy. So we're stuck with this two P1 bugs. Any suggestions to unblock us would be greatly appreciated.
On 2016/06/06 at 11:19:25, kojii wrote: > On 2016/06/06 at 11:16:06, robhogan wrote: > > Won't orthogonal layout be incorrect in this situation anyway? It won't avoid the floats it needs to avoid because their correct position isn't known. That will get sorted out in main layout when it shrinks to avoid them. > > No, as far as I observed. Can you look at the screenshot in https://crbug.com/604095#c15 and identify how it's layout is incorrect? > > > Do you have a test case for what you mean? > > If you apply 'display: inline-block' to the parent of the orthogonal flow, the layout depends on preferred width of the orthogonal flow. If we don't layout orthogonal roots first, computePreferredWidth() will return 0. You mean like this? <!DOCTYPE html> <div id="header" style="display: inline-block;"> <span style="float:left;"> crbug.com/604095: Passes if it doesn't crash crbug.com/604095: Passes if it doesn't crash crbug.com/604095: Passes if it doesn't crash crbug.com/604095: Passes if it doesn't crash</span> <script> if (window.testRunner) testRunner.dumpAsText(); document.body.offsetTop; </script> <style> html { writing-mode:vertical-rl; } #h { writing-mode: horizontal-tb; background-color: green; } </style> <div id="h">Text</div> </div> We lay this out correctly at the moment, to my understanding, because we end up relayouting #h in main layout anyway. That will continue to happen. We just need to avoid the orthogonal layout in this particular situation because it could stumble into stale float lists with deleted pointers or bad co-ordinates, i.e. where the container and the child have changed writing direction, are now orthogonal to each other, and they haven't laid out in their new direction yet.
On 2016/06/06 at 11:31:28, robhogan wrote: > We lay this out correctly at the moment, to my understanding, because we end up relayouting #h in main layout anyway. Yes. > That will continue to happen. If we skip orthogonal root layouts, #header will shrink too much because #header will get the logical height of #h as 0. Orthogonal roots layout is to give the correct preferred width to their parents. It's part of the assumption if they're re-layout during the main layout, but at the point of the main layout, we can't update preferred width of the parent any longer, so we pay dual-layout cost to solve this.
Here's a suggestion. 1. Skipping orthogonal writing-mode roots layout can produce incorrect layout if shrink-to-fit. 2. removeFloatingObjects() does not look right to rob. ==> So we need other alternatives. What are other options? Orthogonal roots layout is the only case in CSS where the spec requires children to layout before parent's preferred width: https://drafts.csswg.org/css-writing-modes-3/#orthogonal-shrink-to-fit and I'm not surprised if we need more special cases to handle this situation. Somewhere in shrinkLogicalWidthToAvoidFloats(), we can skip looking at floats if cb->needsLayout(). Does this look better to you? My preference is not to make non-orthogonal layouts slower, even if it results in slower orthogonal layouts, because orthogonal layouts is rare, hence my patch. We could add more "if"s to several places to check if parent/cb->needsLayout(). Would that make you happy?
PTAL, the 3rd option. I found LayoutTable avoids to call shrinkLogicalWidthToAvoidFloats() if cb is perpendicular: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... but don't do this in LayoutBox. This seems to fix. This will add two new "if"s in non-orthogonal code path, but keeps orthogonal path not being slow.
huh, no, the 3rd option fails two tests...we need 4th option, but I don't have that atm...
eae@chromium.org changed reviewers: + szager@chromium.org, wkorman@chromium.org
+szager,wkorman as they've been thinking about writing modes recently
While PS1/PS2 can fix both https://crbug.com/604095 and https://crbug.com/613869, the root cause of the latter is different as in comment #12 and can be fixed separately. I'm removing the latter.
Description was changed from ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This patch clears such objects to be re-created when the containing block is laid out. BUG=604095, 613869 ========== to ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This patch clears such objects to be re-created when the containing block is laid out. BUG=604095 ==========
It sounds like you are working on a 4th approach, or does this in fact fix some test(s) and you would like to land this in interim? https://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:164: for (LayoutObject* curr = parent(); curr; curr = curr->parent()) { This line diff seems to already be in ToT. Update the CL description? So this CL now has only the check for parallel writing modes added to logical width/margin computing. https://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2321: && cb->style()->isHorizontalWritingMode() == style()->isHorizontalWritingMode()) { Perhaps use static helper method like isParallelWritingMode(style1, style2) here and below just to help readability/maintainability. Or invert logic and call it isPerpendicularWritingMode(), and could use it in LayoutTable as you found as well: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
Thank you for having a look. On 2016/06/08 at 18:23:34, wkorman wrote: > It sounds like you are working on a 4th approach, or does this in fact fix some test(s) and you would like to land this in interim? > > https://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:164: for (LayoutObject* curr = parent(); curr; curr = curr->parent()) { > This line diff seems to already be in ToT. Update the CL description? So this CL now has only the check for parallel writing modes added to logical width/margin computing. Yeah, just landed that part separately. I'll update the description. >http://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode2321 > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2321: && cb->style()->isHorizontalWritingMode() == style()->isHorizontalWritingMode()) { > Perhaps use static helper method like isParallelWritingMode(style1, style2) here and below just to help readability/maintainability. Or invert logic and call it isPerpendicularWritingMode(), and could use it in LayoutTable as you found as well: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... We have two types of child-first layout; subtree roots and orthogonal roots. In both cases, it conflicts with the assumption floats makes--cb is in layout before it's children. 3rd option broke 2 layout tests so we can't take it. 4th option I just came up is to detect if we're in one of subtree layout, and do things differently. This doesn't look pretty, breaks our assumption that subtree can be laid out independently, but when PS2 was rejected, that may be the last option. Does this look sane?
On 2016/06/09 at 01:57:59, kojii wrote: > Thank you for having a look. > > On 2016/06/08 at 18:23:34, wkorman wrote: > > It sounds like you are working on a 4th approach, or does this in fact fix some test(s) and you would like to land this in interim? > > > > https://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > https://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:164: for (LayoutObject* curr = parent(); curr; curr = curr->parent()) { > > This line diff seems to already be in ToT. Update the CL description? So this CL now has only the check for parallel writing modes added to logical width/margin computing. > > Yeah, just landed that part separately. I'll update the description. > > >http://codereview.chromium.org/2025543002/diff/40001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode2321 > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2321: && cb->style()->isHorizontalWritingMode() == style()->isHorizontalWritingMode()) { > > Perhaps use static helper method like isParallelWritingMode(style1, style2) here and below just to help readability/maintainability. Or invert logic and call it isPerpendicularWritingMode(), and could use it in LayoutTable as you found as well: > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > We have two types of child-first layout; subtree roots and orthogonal roots. In both cases, it conflicts with the assumption floats makes--cb is in layout before it's children. > > 3rd option broke 2 layout tests so we can't take it. > > 4th option I just came up is to detect if we're in one of subtree layout, and do things differently. This doesn't look pretty, breaks our assumption that subtree can be laid out independently, but when PS2 was rejected, that may be the last option. > > Does this look sane? Is there a different change, or did you accidentally forget to update this one? I am still only seeing patch 3.
Oh, wow, someone has fixed this, so this CL is no longer necessary ;) Thank you wkorman@ for your time.
Re-opening for crbug.com/646178
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This patch clears such objects to be re-created when the containing block is laid out. BUG=604095 ========== to ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This occurs when LayoutMultiColumnFlowThread::populate(), LayoutBoxModelObject::moveChildrenTo() with !fullRemoveInsert, or more, for the optimization purposes. This patch clears such objects to be re-created when the containing block is laid out. BUG=604095, 633409, 646178 ==========
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK but please allow robhogan to comment before landing. LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
robhogan@, could you mind to have a look? There's a minor improvement, and I have better assessment than before. The fix now checks if cb->needsLayout(). If this was false, such as due to subtree invalidations, we clear floats but the cb will not be laid out, so that was wrong. With this, IIUC, the behavior is correct, just that we may re-compute floats more than necessary. My current best assessment is that, when we populate multicol (or maybe other cases too), we call moveChildTo for its direct children. Those direct children go through removeChildNode() and thus will be removed from FloatingObjects as you pointed out, but when descendants of the direct children are floating, we don't traverse all of them to remove. This still works in normal layout because FloatingObjets will be updated when the cb lays out. So the choice is: a. Traverse all descendants and remove them from floats. This makes removeChildNode more expensive. b. Mark a flag or remove all floats from cb in removeChildNode. This does not make removeChildNode() expensive, but since we re-compute all floats whenever a child moves, this makes such layout more expensive. c. Clear cb's FloatingObjects in orthogonal root layout (this patch.) This makes orthogonal subtree layout more expensive when its cb has floats. I believe c is the rarest among the three and hence it's the one we should make expensive, if we had to choose one. I admit that there are still a lot of code I haven't read yet, but I hope my assessment above makes sense to you.
On 2016/09/16 at 10:11:34, kojii wrote: > Those direct children go through removeChildNode() and thus will be removed from FloatingObjects as you pointed out, but when descendants of the direct children are floating, we don't traverse all of them to remove. I only have two reservations and I think they can be dealt with later: - We could possibly change LayoutBlockFlow::markAllDescendantsWithFloatsForLayout() to cope with this rather than nuking the floats. There are probably corner cases where the performance will suffer with this approach but I doubt they're very common! - We seem to have a bit of a chicken and egg situation when it comes to making floats in an orthogonal writing mode root avoid floats in the main writing mode root. I think in most cases the main layout rectifies the situation and does the right thing but there are probably cases where it doesn't quite work as intended. Per https://bugs.chromium.org/p/chromium/issues/detail?id=604095#c8, the correct behaviour isn't particularly clear. So yes, this is a step forward and LGTM but we should add some comments to the patch to reflect the fact this is something of a WIP!
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for the review! Updated the comment according to your advice. On 2016/09/16 at 18:39:02, robhogan wrote: > On 2016/09/16 at 10:11:34, kojii wrote: > > Those direct children go through removeChildNode() and thus will be removed from FloatingObjects as you pointed out, but when descendants of the direct children are floating, we don't traverse all of them to remove. > > I only have two reservations and I think they can be dealt with later: > > - We could possibly change LayoutBlockFlow::markAllDescendantsWithFloatsForLayout() to cope with this rather than nuking the floats. There are probably corner cases where the performance will suffer with this approach but I doubt they're very common! > - We seem to have a bit of a chicken and egg situation when it comes to making floats in an orthogonal writing mode root avoid floats in the main writing mode root. I think in most cases the main layout rectifies the situation and does the right thing but there are probably cases where it doesn't quite work as intended. Per https://bugs.chromium.org/p/chromium/issues/detail?id=604095#c8, the correct behaviour isn't particularly clear. I admit that I'm also aware, when the spec says orthogonal writing modes create BFC, in a few places we don't do this accurately. Also we're still not 100% certain that current method to handle orthogonal shrink-to-fit is good. So I agree that we haven't done quite well yet when all these are combined, this is only a step.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robhogan@gmail.com, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2025543002/#ps180001 (title: "Update comments as per robhogan review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This occurs when LayoutMultiColumnFlowThread::populate(), LayoutBoxModelObject::moveChildrenTo() with !fullRemoveInsert, or more, for the optimization purposes. This patch clears such objects to be re-created when the containing block is laid out. BUG=604095, 633409, 646178 ========== to ========== Fix when orthogonal writing mode roots have floating siblings When orthogonal writing mode roots have floating siblings, its containing block may still have old or even deleted LayoutObjects. This occurs when LayoutMultiColumnFlowThread::populate(), LayoutBoxModelObject::moveChildrenTo() with !fullRemoveInsert, or more, for the optimization purposes. This patch clears such objects to be re-created when the containing block is laid out. BUG=604095, 633409, 646178 Committed: https://crrev.com/5c43382692e6b687373576984181e52c30ccd142 Cr-Commit-Position: refs/heads/master@{#419376} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c43382692e6b687373576984181e52c30ccd142 Cr-Commit-Position: refs/heads/master@{#419376}
Message was sent while issue was closed.
For "the correct behaviour isn't particularly clear" part, it'd be probably easier to discuss with a whiteboard. If you will attend BlinkOn, I look forward to seeing you there. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
