Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1551573002:
Clear box from percentage descendants when it is split off from the ancestor (Closed)
|
Created:
4 years, 12 months ago by rhogan Modified:
4 years, 11 months ago Reviewers:
mstensho (USE GERRIT) CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear box from percentage descendants when it is split off from the ancestor
Ensure that ancestors that no longer need to track percentage descendants
after the split have them removed from their tracking lists.
BUG=568387
Committed: https://crrev.com/8f566488e5c3f107d2931ff64bdc68a12dfa0340
Cr-Commit-Position: refs/heads/master@{#371791}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updated #
Total comments: 5
Patch Set 3 : Updated #
Total comments: 3
Patch Set 4 : Updated #Patch Set 5 : Updated #Patch Set 6 : Updated #
Total comments: 1
Patch Set 7 : Updated #Patch Set 8 : Updated #
Messages
Total messages: 31 (13 generated)
robhogan@gmail.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-1-expected.txt (right): https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-1-expected.txt:1: layer at (0,0) size 800x600 Can you make reftests, crash tests or whatever instead? Anything but PNG / dump-render-tree tests. If you really want to check the layout object structure (I don't really think you need to), a unit test could be a better choice. https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:661: if (child->beingDestroyed()) Do you really need this? collapseAnonymousBlockChild() performs the same check anyway. But you might end up returning true without the check, of course. Not sure if that's a problem, though. https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:663: if (child->nextSibling() || child->continuation()) Looks like we're missing a child->previousSibling() check. Not that it matters with the current usage of this method, but this one's gonna sneak up on us from behind at some point in the future when we least expect it. :) https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:666: if (parent->isRubyBase() || parent->isRubyRun()) Ah... ruby, out of the blue. But I guess that's how it has to be. :( There's canCollapseAnonymousBlockChild(), but no exception for ruby there... https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlock.h:296: bool collapseLoneAnonymousBlock(LayoutBlock* child); Should be static? And should it be named collapseIfLoneAnonymousBlock()? https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left): https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4664: } What if collapseLoneAnonymousBlock() did something here and returned true? Then |beforeChild| isn't updated. Aren't you going to get stuck in this loop then? https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4663: if (LayoutBlock::hasPercentHeightContainerMap()) Can this be moved to a separate CL? This change isn't mentioned in the description. If everything really has to be done in one and the same CL (not that it's a lot, but it's two things, nonetheless), better update the description. https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4668: // If a lone anonymous block is now unnecessary. Collapse it away and skip to its parent. "If" without "then". :-P Did you mean to write this in one sentence? "... unnecessary, collapse ..." https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4669: if (!boxToSplit->isLayoutBlock() || !toLayoutBlock(boxToSplit)->collapseLoneAnonymousBlock(toLayoutBlock(boxToSplit))) I hope collapseLoneAnonymousBlock() can be static. This looks silly. :)
Description was changed from ========== Remove unnecessary anonymous blocks when splitting anonymous boxes BUG=568387 ========== to ========== Remove unnecessary anonymous blocks when splitting anonymous boxes Also ensure that ancestors that no longer need to track percentage descendants after the split have them removed from their tracking lists. BUG=568387 ==========
On 2016/01/18 at 20:06:24, mstensho wrote: > https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-1-expected.txt (right): > > https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-1-expected.txt:1: layer at (0,0) size 800x600 > Can you make reftests, crash tests or whatever instead? Anything but PNG / dump-render-tree tests. If you really want to check the layout object structure (I don't really think you need to), a unit test could be a better choice. Addressed all your comments I think.
https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4663: if (LayoutBlock::hasPercentHeightContainerMap()) On 2016/01/18 20:06:24, mstensho wrote: > Can this be moved to a separate CL? This change isn't mentioned in the > description. If everything really has to be done in one and the same CL (not > that it's a lot, but it's two things, nonetheless), better update the > description. Well? :) https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:95: LayoutRectOutsets marginBoxOutsets; What's this? Please submit master rebases and your changes as separate patch sets, so I can more easily diff each patch set. https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4680: // We're splitting from our previousSibling(). It's not lone if it has a previousSibling(). Please add a test, or at least an assertion that previousSibling() is NULL. https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4722: // grandparent its parent so no need to set it before looping. Thanks for explaining!
On 2016/01/21 at 19:12:33, mstensho wrote: > https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/1551573002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:4663: if (LayoutBlock::hasPercentHeightContainerMap()) > On 2016/01/18 20:06:24, mstensho wrote: > > Can this be moved to a separate CL? This change isn't mentioned in the > > description. If everything really has to be done in one and the same CL (not > > that it's a lot, but it's two things, nonetheless), better update the > > description. > > Well? :) OK, I'll split them. I'll make this CL about the percent height descendants.
Description was changed from ========== Remove unnecessary anonymous blocks when splitting anonymous boxes Also ensure that ancestors that no longer need to track percentage descendants after the split have them removed from their tracking lists. BUG=568387 ========== to ========== Clear box from percentage descendants when it is split off from the ancestor Ensure that ancestors that no longer need to track percentage descendants after the split have them removed from their tracking lists. BUG=568387 ==========
https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4680: // We're splitting from our previousSibling(). On 2016/01/21 at 19:12:33, mstensho wrote: > It's not lone if it has a previousSibling(). Please add a test, or at least an assertion that previousSibling() is NULL. No, but it's about to split from the previousSibling so it will become a lone anonymous block. Test 2 covers this.
https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4680: // We're splitting from our previousSibling(). On 2016/01/24 12:31:37, rhogan wrote: > On 2016/01/21 at 19:12:33, mstensho wrote: > > It's not lone if it has a previousSibling(). Please add a test, or at least an > assertion that previousSibling() is NULL. > > No, but it's about to split from the previousSibling so it will become a lone > anonymous block. Test 2 covers this. That's making an assumption about what the caller is doing. If we want to do that, maybe skip the childBlock->beingDestroyed() check as well? And maybe just put the code inside splitAnonymousBoxesAroundChild(), since we're making scary assumptions anyway. https://codereview.chromium.org/1551573002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-1.html (right): https://codereview.chromium.org/1551573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-1.html:1: <!DOCTYPE html> Can you add a "-crash" suffix to the file names, since this is a crash test? Not sure if the "-1" suffix is helpful. https://codereview.chromium.org/1551573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4700: LayoutBlock::clearPercentHeightDescendantsFrom(postBox); This will process the entire postBox subtree, which could be expensive. But I assume this is the only safe thing to do? We could at least avoid some work if we only do this only *once* per splitAnonymousBoxesAroundChild() call, i.e. do it on the topmost postBox that we create.
https://codereview.chromium.org/1551573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4700: LayoutBlock::clearPercentHeightDescendantsFrom(postBox); On 2016/01/25 at 09:17:07, mstensho wrote: > This will process the entire postBox subtree, which could be expensive. But I assume this is the only safe thing to do? > > We could at least avoid some work if we only do this only *once* per splitAnonymousBoxesAroundChild() call, i.e. do it on the topmost postBox that we create. :$ Updated this and the test. Thanks.
lgtm with one nit (but just ignore it if you disagree). https://codereview.chromium.org/1551573002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1551573002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4409: if (boxAtTopOfNewBranch) We could if for |boxAtTopOfNewBranch| only once. if (boxAtTopOfNewBranch) { // bla bla if (LayoutBlock::hasPercentHeightContainerMap()) LayoutBlock::clearPercentHeightDescendantsFrom(boxAtTopOfNewBranch); markBoxForRelayoutAfterSplit(this); }
The CQ bit was checked by robhogan@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com Link to the patchset: https://codereview.chromium.org/1551573002/#ps120001 (title: "Updated")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551573002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551573002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551573002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551573002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by robhogan@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com Link to the patchset: https://codereview.chromium.org/1551573002/#ps140001 (title: "Updated")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551573002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551573002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551573002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551573002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Clear box from percentage descendants when it is split off from the ancestor Ensure that ancestors that no longer need to track percentage descendants after the split have them removed from their tracking lists. BUG=568387 ========== to ========== Clear box from percentage descendants when it is split off from the ancestor Ensure that ancestors that no longer need to track percentage descendants after the split have them removed from their tracking lists. BUG=568387 Committed: https://crrev.com/8f566488e5c3f107d2931ff64bdc68a12dfa0340 Cr-Commit-Position: refs/heads/master@{#371791} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8f566488e5c3f107d2931ff64bdc68a12dfa0340 Cr-Commit-Position: refs/heads/master@{#371791} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
