Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(563)

Issue 2755143003: [LayoutNG] Make NGLineBuilder work with inline floats (Closed)

Created:
3 years, 9 months ago by Gleb Lanbin
Modified:
3 years, 9 months ago
Reviewers:
ikilpatrick, kojii
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/heads/master
Project:
chromium
Visibility:
Public.

Description

[LayoutNG] Make NGLineBuilder work with inline floats This patch teaches NGLineBuilder how to position inline floats. Below is a high level overview of the implemented logic: Consider this example: The quick <div id="float"></div> brown fox jumps over the lazy dog 1) NGLineBuilder/NGTextLayoutAlgorithm starts constructing a line 2) Every time when we hit a text break opportunity NGLineBuilder::SetEnd gets a corresponding NGLayoutInlineItem 3) If it's a float then it tries to position it on the current text line. 4) If the float does not fit it will be added to the UnpositionedFloat list 5) When we place the line we position all pending floats from the UnpositionedFloat list. BUG=635619 TEST=TextFloatsAroundInlineFloatThatFitsOnLine, TextFloatsAroundInlineFloatThatDoesNotFitOnLine Review-Url: https://codereview.chromium.org/2755143003 Cr-Commit-Position: refs/heads/master@{#459332} Committed: https://chromium.googlesource.com/chromium/src/+/fdd13e6a954714ba0cec7070080eacb057fdb3ff

Patch Set 1 #

Total comments: 8

Patch Set 2 : fix comments #

Total comments: 6

Patch Set 3 : update the doc string #

Total comments: 4

Patch Set 4 : fix doc comments #

Patch Set 5 : fix punctuations #

Patch Set 6 : make setIsInPlacedTree to be available for LayoutNG #

Unified diffs Side-by-side diffs Delta from patch set Stats (+245 lines, -32 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 2 chunks +9 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/FloatingObjects.h View 1 2 3 4 5 2 chunks +0 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc View 1 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_node.cc View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_line_builder.h View 1 2 3 4 3 chunks +11 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc View 1 6 chunks +90 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_space_utils.h View 1 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_space_utils.cc View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_text_layout_algorithm_test.cc View 1 1 chunk +114 lines, -0 lines 0 comments Download

Messages

Total messages: 32 (21 generated)
Gleb Lanbin
3 years, 9 months ago (2017-03-17 23:33:24 UTC) #2
ikilpatrick
https://codereview.chromium.org/2755143003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right): https://codereview.chromium.org/2755143003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc#newcode292 third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:292: NGConstraintSpace::CreateFromLayoutObject(*block_flow); it's probably worth building this ourselves and percentage ...
3 years, 9 months ago (2017-03-20 16:41:52 UTC) #3
Gleb Lanbin
PTAL https://codereview.chromium.org/2755143003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right): https://codereview.chromium.org/2755143003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc#newcode292 third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:292: NGConstraintSpace::CreateFromLayoutObject(*block_flow); On 2017/03/20 16:41:52, ikilpatrick wrote: > it's ...
3 years, 9 months ago (2017-03-21 21:04:06 UTC) #4
kojii
lgtm, great step moving forward, thank you for this work. Adjusting line break or pushing ...
3 years, 9 months ago (2017-03-23 04:54:13 UTC) #5
ikilpatrick
https://codereview.chromium.org/2755143003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right): https://codereview.chromium.org/2755143003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc#newcode36 third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:36: IsNewFormattingContextForInFlowBlockLevelChild(parent_space, style); this should always be true for floats? ...
3 years, 9 months ago (2017-03-23 16:19:06 UTC) #6
ikilpatrick
On 2017/03/21 21:04:06, Gleb Lanbin wrote: > PTAL > > https://codereview.chromium.org/2755143003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc > File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right): ...
3 years, 9 months ago (2017-03-23 16:20:17 UTC) #7
Gleb Lanbin
On 2017/03/23 16:20:17, ikilpatrick wrote: > On 2017/03/21 21:04:06, Gleb Lanbin wrote: > > PTAL ...
3 years, 9 months ago (2017-03-23 19:35:47 UTC) #8
ikilpatrick
lgtm % nits. https://codereview.chromium.org/2755143003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_line_builder.h File third_party/WebKit/Source/core/layout/ng/ng_line_builder.h (right): https://codereview.chromium.org/2755143003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_line_builder.h#newcode116 third_party/WebKit/Source/core/layout/ng/ng_line_builder.h:116: // - tries to position the ...
3 years, 9 months ago (2017-03-23 19:39:59 UTC) #11
Gleb Lanbin
https://codereview.chromium.org/2755143003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_line_builder.h File third_party/WebKit/Source/core/layout/ng/ng_line_builder.h (right): https://codereview.chromium.org/2755143003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_line_builder.h#newcode116 third_party/WebKit/Source/core/layout/ng/ng_line_builder.h:116: // - tries to position the float right away ...
3 years, 9 months ago (2017-03-23 19:47:18 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2755143003/140001
3 years, 9 months ago (2017-03-24 01:32:35 UTC) #29
commit-bot: I haz the power
3 years, 9 months ago (2017-03-24 01:39:32 UTC) #32
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/fdd13e6a954714ba0cec7070080e...

Powered by Google App Engine
This is Rietveld 408576698