Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706403008:
[LayoutNG] Implement ComputeMinAndMaxContentSizes for inline (Closed)
|
Created:
3 years, 10 months ago by kojii Modified:
3 years, 9 months ago Reviewers:
cbiesinger CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Implement ComputeMinAndMaxContentSizes for inline
This patch implements ComputeMinAndMaxContentSizes for NGInlineNode, and
changes NGBlockLayoutAlgorithm::ComputeMinAndMaxContentSizes to call it
when it found inline children.
BUG=636993
Review-Url: https://codereview.chromium.org/2706403008
Cr-Commit-Position: refs/heads/master@{#453513}
Committed: https://chromium.googlesource.com/chromium/src/+/1aebff2782292aed9da6a88bf3e98dc8a280c605
Patch Set 1 #Patch Set 2 : Cleanup and add a test #
Total comments: 4
Patch Set 3 : cbiesinger review, add comments, add and fix tests #Patch Set 4 : Change the way to load ahem to make asan bot happy #
Total comments: 6
Patch Set 5 : cbiesinger review #
Messages
Total messages: 32 (24 generated)
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 #2 (id:20001) has been deleted
Description was changed from ========== WIP: [LayoutNG] minmax BUG= ========== to ========== [LayoutNG] Implement ComputeMinAndMaxContentSizes for inline This patch implements ComputeMinAndMaxContentSizes for NGInlineNode, and changes NGBlockLayoutAlgorithm::ComputeMinAndMaxContentSizes to call it when it found inline children. BUG=636993 ==========
kojii@chromium.org changed reviewers: + cbiesinger@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2706403008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2706403008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:341: sizes.min_content = std::max(sizes.min_content, child_sizes.min_content); This assumes you can linebreak between two inlines, which may not be the case https://codereview.chromium.org/2706403008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:342: sizes.max_content = std::max(sizes.max_content, child_sizes.max_content); This is not correct for inline. For inline, you basically need to sum up the sizes of all inlines within a potential row. This is actually fairly complicated, see the old implementation: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... It seems like maybe some of your NGInlineNode code needs to move here in some way...?
https://codereview.chromium.org/2706403008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2706403008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:341: sizes.min_content = std::max(sizes.min_content, child_sizes.min_content); On 2017/02/23 at 19:07:01, cbiesinger wrote: > This assumes you can linebreak between two inlines, which may not be the case At this point, |child_sizes| contains values from all lines produced from the all inline nodes starting |node|. If |node| is inline, it represents a sequence of inline nodes up until the end or it sees a block node. So in this loop, |node| is like an anonymous block that contains all inlines starting from the |node|, and |NextSibling| returns the next of the anonymous. The floats/OOF doc explains this a bit. Sorry for the lack of explanation, I'll add comment. https://codereview.chromium.org/2706403008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:342: sizes.max_content = std::max(sizes.max_content, child_sizes.max_content); On 2017/02/23 at 19:07:01, cbiesinger wrote: > This is not correct for inline. For inline, you basically need to sum up the sizes of all inlines within a potential row. This is actually fairly complicated, see the old implementation: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > It seems like maybe some of your NGInlineNode code needs to move here in some way...? Yeah, the code in |NGInlineNode::ComputeMinAndMaxContentSizes| does basically the same thing as |LayoutBlockFlow::computeInlinePreferredLogicalWidths|. |LayoutBlockFlow| optimizes for min/max content by not using the |BreakingContext|, but still uses |LazyLineBreakIterator|. Having two line breakers have been a bit of head ache for me and Emil, so we may optimize when things are stabilized, but I'd like to use the line breaker (NGLineBuilder) for now. There's a plan to completely replace the current line breaker, so I don't want to create two separate paths at least until the replace is completed.
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 #3 (id:60001) has been deleted
PTAL, added comments for clarification, and added a test for multiple spans. Your comments reminded me how poorly I have explained how inline works in NG to the team. Emil and I were discussing how to improve that. I'll spend more time on documents, thank you about that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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: This issue passed the CQ dry run.
lgtm, but see comments below https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:333: // following inline siblings and descendants. Thanks for the explanation! That was not obvious to me before. https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_inline_node.h (right): https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_inline_node.h:43: MinAndMaxContentSizes ComputeMinAndMaxContentSizes(); Please add a comment for this function, as well. https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_inline_node.h:48: // calling the Layout method. LayoutInline calls this function itself. Should it be made private?
The CQ bit was checked by kojii@chromium.org
Thank you for your review, all done! https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:333: // following inline siblings and descendants. On 2017/02/27 at 23:16:16, cbiesinger wrote: > Thanks for the explanation! That was not obvious to me before. My bad actually, this isn't obvious to anyone, I should explain better. Update the class comment of NGInlineNode too, and I'll do so to documents. https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_inline_node.h (right): https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_inline_node.h:43: MinAndMaxContentSizes ComputeMinAndMaxContentSizes(); On 2017/02/27 at 23:16:16, cbiesinger wrote: > Please add a comment for this function, as well. Done. https://codereview.chromium.org/2706403008/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_inline_node.h:48: // calling the Layout method. On 2017/02/27 at 23:16:16, cbiesinger wrote: > LayoutInline calls this function itself. Should it be made private? Done.
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2706403008/#ps120001 (title: "cbiesinger review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488252636532570,
"parent_rev": "2451a9a11f87ce84cd6f280984b05251d7a2093d", "commit_rev":
"1aebff2782292aed9da6a88bf3e98dc8a280c605"}
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Implement ComputeMinAndMaxContentSizes for inline This patch implements ComputeMinAndMaxContentSizes for NGInlineNode, and changes NGBlockLayoutAlgorithm::ComputeMinAndMaxContentSizes to call it when it found inline children. BUG=636993 ========== to ========== [LayoutNG] Implement ComputeMinAndMaxContentSizes for inline This patch implements ComputeMinAndMaxContentSizes for NGInlineNode, and changes NGBlockLayoutAlgorithm::ComputeMinAndMaxContentSizes to call it when it found inline children. BUG=636993 Review-Url: https://codereview.chromium.org/2706403008 Cr-Commit-Position: refs/heads/master@{#453513} Committed: https://chromium.googlesource.com/chromium/src/+/1aebff2782292aed9da6a88bf3e9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/1aebff2782292aed9da6a88bf3e9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
