|
|
Created:
4 years, 1 month ago by Gleb Lanbin Modified:
4 years, 1 month ago CC:
chromium-reviews, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, glebl+reviews_chromium.org, atotic+reviews_chromium.org, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, zoltan1, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove redundant 'derived constraint space' setters from NGConstraintSpace
Remove
SetOverflowTriggersScrollbar
SetFixedSize
SetFragmentationType
SetIsNewFormattingContext
SetSize
Size
from NGConstraintSpace. Use NGConstraintSpaceBuilder instead.
BUG=635619
Committed: https://crrev.com/2bd351a87b733b8b17f6507adb9b6d0829ca978f
Cr-Commit-Position: refs/heads/master@{#433404}
Patch Set 1 #
Total comments: 5
Patch Set 2 : sync to the head #
Messages
Total messages: 32 (18 generated)
glebl@chromium.org changed reviewers: + ikilpatrick@chromium.org
The CQ bit was checked by glebl@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...
lgtm
cbiesinger@chromium.org changed reviewers: + cbiesinger@chromium.org
https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:189: constraint_space_->SetSize( Why is the new code still correct? This was necessary for correctness...? https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc (right): https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:32: opportunity.size = space.AvailableSize(); Why is this correct?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by glebl@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: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by glebl@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...
https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:189: constraint_space_->SetSize( On 2016/11/18 23:44:23, cbiesinger wrote: > Why is the new code still correct? This was necessary for correctness...? ConstraintSpace::Size() was only used in NGLayoutOpportunity::CreateLayoutOpportunityFromConstraintSpace. It always equals to AvailableSize. So this patch removes redundant Size() method because we already store the size information in physical constraint space. https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc (right): https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:32: opportunity.size = space.AvailableSize(); On 2016/11/18 23:44:23, cbiesinger wrote: > Why is this correct? Acknowledged.
https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:189: constraint_space_->SetSize( On 2016/11/19 04:20:47, Gleb Lanbin wrote: > On 2016/11/18 23:44:23, cbiesinger wrote: > > Why is the new code still correct? This was necessary for correctness...? > > ConstraintSpace::Size() was only used in > NGLayoutOpportunity::CreateLayoutOpportunityFromConstraintSpace. It always > equals to AvailableSize. So this patch removes redundant Size() method because > we already store the size information in physical constraint space. So I'm confused. I thought this specific SetSize call was necessary because we will use constraint_space_ to create the opportunity iterator, and nothing else makes sure that the size is correct (especially, I don't see a call to set AvailableSize on constraint_space_). What am I missing...?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/19 04:28:47, cbiesinger wrote: > https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > (left): > > https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:189: > constraint_space_->SetSize( > On 2016/11/19 04:20:47, Gleb Lanbin wrote: > > On 2016/11/18 23:44:23, cbiesinger wrote: > > > Why is the new code still correct? This was necessary for correctness...? > > > > ConstraintSpace::Size() was only used in > > NGLayoutOpportunity::CreateLayoutOpportunityFromConstraintSpace. It always > > equals to AvailableSize. So this patch removes redundant Size() method because > > we already store the size information in physical constraint space. > > So I'm confused. I thought this specific SetSize call was necessary because we > will use constraint_space_ to create the opportunity iterator, and nothing else > makes sure that the size is correct (especially, I don't see a call to set > AvailableSize on constraint_space_). What am I missing...? here we set AvailableSize to the builder https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... then for each child we create its own PhysicalConstraintSpace here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... because of that we can remove a redundant method that set/get the size from NGConstraintSpace and use AvailableSize instead.
On 2016/11/19 04:44:03, Gleb Lanbin wrote: > On 2016/11/19 04:28:47, cbiesinger wrote: > > > https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > > (left): > > > > > https://codereview.chromium.org/2515923002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:189: > > constraint_space_->SetSize( > > On 2016/11/19 04:20:47, Gleb Lanbin wrote: > > > On 2016/11/18 23:44:23, cbiesinger wrote: > > > > Why is the new code still correct? This was necessary for correctness...? > > > > > > ConstraintSpace::Size() was only used in > > > NGLayoutOpportunity::CreateLayoutOpportunityFromConstraintSpace. It always > > > equals to AvailableSize. So this patch removes redundant Size() method > because > > > we already store the size information in physical constraint space. > > > > So I'm confused. I thought this specific SetSize call was necessary because we > > will use constraint_space_ to create the opportunity iterator, and nothing > else > > makes sure that the size is correct (especially, I don't see a call to set > > AvailableSize on constraint_space_). What am I missing...? > > here we set AvailableSize to the builder > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... > then for each child we create its own PhysicalConstraintSpace here > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... > because of that we can remove a redundant method that set/get the size from > NGConstraintSpace and use AvailableSize instead. we started using children_space to create the iterator that's why we don't need to rely on the parent logical size anymore. That's it's safe to remove it. All tests passed. I still think it's correct.
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2515923002/#ps40001 (title: "sync to the head")
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 #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove redundant 'derived constraint space' setters from NGConstraintSpace Remove SetOverflowTriggersScrollbar SetFixedSize SetFragmentationType SetIsNewFormattingContext SetSize Size from NGConstraintSpace. Use NGConstraintSpaceBuilder instead. BUG=635619 ========== to ========== Remove redundant 'derived constraint space' setters from NGConstraintSpace Remove SetOverflowTriggersScrollbar SetFixedSize SetFragmentationType SetIsNewFormattingContext SetSize Size from NGConstraintSpace. Use NGConstraintSpaceBuilder instead. BUG=635619 Committed: https://crrev.com/2bd351a87b733b8b17f6507adb9b6d0829ca978f Cr-Commit-Position: refs/heads/master@{#433404} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2bd351a87b733b8b17f6507adb9b6d0829ca978f Cr-Commit-Position: refs/heads/master@{#433404}
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 433404 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/2514693004/ by sadrul@chromium.org. The reason for reverting is: Causing wide-spread build failures, with errors like: ../../third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:74:13: error: no member named 'Size' in 'blink::NGConstraintSpace' space.Size().ConvertToPhysical(space.WritingMode()); ~~~~~ ^ .
Message was sent while issue was closed.
On 2016/11/19 06:00:13, sadrul wrote: > A revert of this CL (patchset #2 id:40001) has been created in > https://codereview.chromium.org/2514693004/ by mailto:sadrul@chromium.org. > > The reason for reverting is: Causing wide-spread build failures, with errors > like: > > ../../third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:74:13: > error: no member named 'Size' in 'blink::NGConstraintSpace' > space.Size().ConvertToPhysical(space.WritingMode()); > ~~~~~ ^ > . Some links to failing bots: https://build.chromium.org/p/chromium.mac/builders/Mac%20Builder%20%28dbg%29/... https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder/builds/7... https://build.chromium.org/p/chromium.win/builders/Win%20Builder/builds/32244 etc.
Message was sent while issue was closed.
Sorry, you are absolutely correct. I didn't realize that https://chromium.googlesource.com/chromium/src/+/07703ad5e0afd141d4a6da847ffe... had changed this. |