Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2679343004:
Add left_offset to NGFloatingObject and set it on legacy FloatingObject (Closed)
|
Created:
3 years, 10 months ago by Gleb Lanbin Modified:
3 years, 10 months ago CC:
chromium-reviews, cbiesinger, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, glebl+reviews_chromium.org, dgrogan+ng_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. |
DescriptionAdd left_offset to NGFloatingObject and set it on legacy FloatingObject
This patch adds the support of the next use case:
When a float are surrounded by a bunch of empty divs we attach it to the
next in-flow child that can determine its position in space. We need to
copy 2 offsets to the legacy tree:
1) Float's LayoutObject offset relative to its gunuine parent
2) Float's FloatingObject offset relative to its real parent so
BlockFlowPainter can paint it correctly
List of changes:
- Add left_offset to NGFloatingObject. This will be set to
FloatingObject
- Add parent_space to NGFloatingObject. This is used to calculate
offset relative to the genuine parent.
- PositionFloat uses origin_point/from_offset with inline_offset
that comes from float's and float's parent constraint spaces.
- Enable NGBlockLayoutAlgorithmTest::PositionFloatInsideEmptyBlocks
BUG=635619
TEST=NGBlockLayoutAlgorithmTest::PositionFloatInsideEmptyBlocks
Review-Url: https://codereview.chromium.org/2679343004
Cr-Commit-Position: refs/heads/master@{#449774}
Committed: https://chromium.googlesource.com/chromium/src/+/57ac6dc1c5b8d3ffa431bc641a6ccab3200751d3
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix comments #
Total comments: 8
Patch Set 3 : fix comments #
Total comments: 6
Messages
Total messages: 28 (17 generated)
Patchset #1 (id:1) has been deleted
glebl@chromium.org changed reviewers: + cbiesinger@chromium.org, 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...
It seems you're mixing left and inline in this patch. Doesn't this cause wrong results in vertical writing modes? https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:232: void UpdateFloatingObjectLeftOffset( Left -> Inline, no? https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:247: LayoutUnit from_offset_block_offset = I think "from_offset_block" reads better https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:251: auto float_space = floating_object->space; auto*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:232: void UpdateFloatingObjectLeftOffset( On 2017/02/08 22:39:01, cbiesinger wrote: > Left -> Inline, no? you're right. We should use physical coordinates here. I added TODO At this moment writing modes do not work for me even with regular blocks. Example: I see logical block_size calculated incorrectly, constantly hitting ASSERT in ng_length_utils.cc with a simple example like <style> #container { height: 80px; width: 200px; writing-mode: vertical-lr; padding-left: 90px; } #div1 { margin-top: 5px; padding-top: 7px; height: 10px; width: 10px; } </style> <div id='container'> <div id='div1'> </div> </div> So my plan was to land this patch that enables NGBlockLayoutAlgorithmTest::PositionFloatInsideEmptyBlocks test which is based on the correct expectations, investigate some of TODOs, ASSERTs related to writing modes and then fix UpdateFloatingObjectLeftOffset. Sounds good? https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:247: LayoutUnit from_offset_block_offset = On 2017/02/08 22:39:01, cbiesinger wrote: > I think "from_offset_block" reads better Done. https://codereview.chromium.org/2679343004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:251: auto float_space = floating_object->space; On 2017/02/08 22:39:01, cbiesinger wrote: > auto* done. it's Member<const NGConstraintSpace> which is a wrapper around our pointer.
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: 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_...)
https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:778: if (ConstraintSpace().IsNewFormattingContext()) { I assume this was intentional? Don't you still want to do this for FC's? https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:233: // TODO(glebl): We should use physical offset here. By this you mean the value we copy back into the old layout tree should be in physical coordinates right? https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_floating_object.h (right): https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:21: : public GarbageCollectedFinalized<NGFloatingObject> { Also I assume we are performing copies of this object as it bubbles up? https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:45: Member<const NGConstraintSpace> parent_space; This doesn't need to be stored here right?
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/2679343004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:778: if (ConstraintSpace().IsNewFormattingContext()) { On 2017/02/10 01:53:41, ikilpatrick wrote: > I assume this was intentional? Don't you still want to do this for FC's? I don't remember why I added that. I ran tests that we have today and they passed without this statement. So I decided to delete this. https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:233: // TODO(glebl): We should use physical offset here. On 2017/02/10 01:53:41, ikilpatrick wrote: > By this you mean the value we copy back into the old layout tree should be in > physical coordinates right? yes https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_floating_object.h (right): https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:21: : public GarbageCollectedFinalized<NGFloatingObject> { On 2017/02/10 01:53:41, ikilpatrick wrote: > Also I assume we are performing copies of this object as it bubbles up? yes, we're copying heap pointers https://codereview.chromium.org/2679343004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:45: Member<const NGConstraintSpace> parent_space; On 2017/02/10 01:53:41, ikilpatrick wrote: > This doesn't need to be stored here right? we use the original parent's inline BFC offset in our calculation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:229: void UpdateFloatingObjectLeftOffset( i find this a little weird that this is a one line function, but I see why. https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:250: floating_object->parent_space; didn't rename this to original_parent_space?
lgtm
thanks for the review https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:229: void UpdateFloatingObjectLeftOffset( On 2017/02/10 22:43:42, ikilpatrick wrote: > i find this a little weird that this is a one line function, but I see why. yes this function can grow to something more than 1 line + it's better to keep all these comments and explanations separated from the main logic. https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:250: floating_object->parent_space; On 2017/02/10 22:43:42, ikilpatrick wrote: > didn't rename this to original_parent_space? I thought we were talking about renaming parent_space variable at the line 242. I can rename floating_object->parent_space to floating_object->original_parent_space in my following cls or just use appropriate variables names like float_parent_space.
The CQ bit was checked by glebl@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:229: void UpdateFloatingObjectLeftOffset( On 2017/02/10 22:58:39, Gleb Lanbin wrote: > On 2017/02/10 22:43:42, ikilpatrick wrote: > > i find this a little weird that this is a one line function, but I see why. > > yes this function can grow to something more than 1 line + it's better to keep > all these comments and explanations separated from the main logic. Ah ok, sg then. https://codereview.chromium.org/2679343004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:250: floating_object->parent_space; On 2017/02/10 22:58:39, Gleb Lanbin wrote: > On 2017/02/10 22:43:42, ikilpatrick wrote: > > didn't rename this to original_parent_space? > > I thought we were talking about renaming parent_space variable at the line 242. > I can rename floating_object->parent_space to > floating_object->original_parent_space in my following cls or just use > appropriate variables names like float_parent_space. Yeah i think above would be good, (only if you agree it would be clearer).
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486767525362120,
"parent_rev": "1fdbf2251ecce5ee889f2cb7971e49b77b240f8d", "commit_rev":
"57ac6dc1c5b8d3ffa431bc641a6ccab3200751d3"}
Message was sent while issue was closed.
Description was changed from ========== Add left_offset to NGFloatingObject and set it on legacy FloatingObject This patch adds the support of the next use case: When a float are surrounded by a bunch of empty divs we attach it to the next in-flow child that can determine its position in space. We need to copy 2 offsets to the legacy tree: 1) Float's LayoutObject offset relative to its gunuine parent 2) Float's FloatingObject offset relative to its real parent so BlockFlowPainter can paint it correctly List of changes: - Add left_offset to NGFloatingObject. This will be set to FloatingObject - Add parent_space to NGFloatingObject. This is used to calculate offset relative to the genuine parent. - PositionFloat uses origin_point/from_offset with inline_offset that comes from float's and float's parent constraint spaces. - Enable NGBlockLayoutAlgorithmTest::PositionFloatInsideEmptyBlocks BUG=635619 TEST=NGBlockLayoutAlgorithmTest::PositionFloatInsideEmptyBlocks ========== to ========== Add left_offset to NGFloatingObject and set it on legacy FloatingObject This patch adds the support of the next use case: When a float are surrounded by a bunch of empty divs we attach it to the next in-flow child that can determine its position in space. We need to copy 2 offsets to the legacy tree: 1) Float's LayoutObject offset relative to its gunuine parent 2) Float's FloatingObject offset relative to its real parent so BlockFlowPainter can paint it correctly List of changes: - Add left_offset to NGFloatingObject. This will be set to FloatingObject - Add parent_space to NGFloatingObject. This is used to calculate offset relative to the genuine parent. - PositionFloat uses origin_point/from_offset with inline_offset that comes from float's and float's parent constraint spaces. - Enable NGBlockLayoutAlgorithmTest::PositionFloatInsideEmptyBlocks BUG=635619 TEST=NGBlockLayoutAlgorithmTest::PositionFloatInsideEmptyBlocks Review-Url: https://codereview.chromium.org/2679343004 Cr-Commit-Position: refs/heads/master@{#449774} Committed: https://chromium.googlesource.com/chromium/src/+/57ac6dc1c5b8d3ffa431bc641a6c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/57ac6dc1c5b8d3ffa431bc641a6c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
