Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706353004:
[LayoutNG] Introduce block child iterator. (Closed)
|
Created:
3 years, 10 months ago by ikilpatrick Modified:
3 years, 10 months ago 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] Introduce block child iterator.
This will be used for iterating of children while resuming layout from a
given break token.
See experimental patch for the multi-col approach: crrev/2693193002
BUG=635619
Review-Url: https://codereview.chromium.org/2706353004
Cr-Commit-Position: refs/heads/master@{#452333}
Committed: https://chromium.googlesource.com/chromium/src/+/0e277884ee599a8a09b01c93c97934f3c8c285ac
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 29
Patch Set 4 : address comments. #
Total comments: 4
Patch Set 5 : address comments. #
Total comments: 6
Patch Set 6 : address comments. #Messages
Total messages: 22 (9 generated)
ikilpatrick@chromium.org changed reviewers: + glebl@chromium.org, mstensho@opera.com
atotic@chromium.org changed reviewers: + atotic@chromium.org
https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h:18: // break token will iterator through unfinished children. s/iterator/iterate ? As a user of iterator API, I often need to know how does iterator behaves wrt modifications (and most of Chrome docs do not specify this). Might want to add a note about it.
https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:23: HeapVector<Member<NGBreakToken>>& child_break_tokens); Since the implementation has now been moved to a separate file, better document here that child_break_tokens are taken over and emptied. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:29: const auto& child_break_tokens = break_token_->ChildBreakTokens(); Can be moved out of the do-while loop. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:32: if (child_token_idx_ < child_break_tokens.size()) { Looks like you could get out early here if you do: if (child_token_idx_ >= child_break_tokens.size()) break; https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:37: child_break_token_candidate->InputNode() == child_) { And how about breaking early here if child_break_token_candidate->InputNode() != child_? https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:40: // We have only found a node if it's break token is unfinished. it's -> its https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:43: break; Let me see if I've got this right: Each child that we have previously started to lay out (and possibly also finished) has an entry in child_break_tokens, while children that we haven't visited at all yet don't? So, whether a child fragmented or not doesn't really matter; they'll all get break tokens once we're finished with them (or when they actually break)? Yes? https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h:32: size_t child_token_idx_; Please document what this is. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:27: setBodyInnerHTML(R"HTML( I see that setBodyInnerHTML() is used in existing ng tests already, but do you have any thoughts on the fact that setBodyInnerHTML() will actually lay out with the current layout engine first? https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:33: toLayoutBlockFlow(document().getElementById("child1")->layoutObject())); You can use getLayoutObjectByElementId("id") instead of document().getElementById("id")->layoutObject(). https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:70: HeapVector<Member<NGBreakToken>> child_break_tokens2; Unused. Did you mean to use it below? Otherwise, just remove it. Re-using child_break_tokens, like you currently do, should work fine, since NGBlockBreakToken() eats the children.
https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:23: HeapVector<Member<NGBreakToken>>& child_break_tokens); On 2017/02/22 13:11:30, mstensho wrote: > Since the implementation has now been moved to a separate file, better document > here that child_break_tokens are taken over and emptied. Done. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:29: const auto& child_break_tokens = break_token_->ChildBreakTokens(); On 2017/02/22 13:11:30, mstensho wrote: > Can be moved out of the do-while loop. Done. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:32: if (child_token_idx_ < child_break_tokens.size()) { On 2017/02/22 13:11:31, mstensho wrote: > Looks like you could get out early here if you do: > > if (child_token_idx_ >= child_break_tokens.size()) > break; Done. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:37: child_break_token_candidate->InputNode() == child_) { On 2017/02/22 13:11:31, mstensho wrote: > And how about breaking early here if child_break_token_candidate->InputNode() != > child_? Done. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:40: // We have only found a node if it's break token is unfinished. On 2017/02/22 13:11:30, mstensho wrote: > it's -> its Done. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:43: break; On 2017/02/22 13:11:30, mstensho wrote: > Let me see if I've got this right: > > Each child that we have previously started to lay out (and possibly also > finished) has an entry in child_break_tokens, while children that we haven't > visited at all yet don't? So, whether a child fragmented or not doesn't really > matter; they'll all get break tokens once we're finished with them (or when they > actually break)? Yes? Yes thats exactly correct. I can try an encapsulate that in a comment if you like. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h:18: // break token will iterator through unfinished children. On 2017/02/22 05:45:07, atotic wrote: > s/iterator/iterate ? > > As a user of iterator API, I often need to know how does iterator > behaves wrt modifications (and most of Chrome docs do not specify > this). Might want to add a note about it. Done? https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h:32: size_t child_token_idx_; On 2017/02/22 13:11:31, mstensho wrote: > Please document what this is. Done. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:27: setBodyInnerHTML(R"HTML( On 2017/02/22 13:11:31, mstensho wrote: > I see that setBodyInnerHTML() is used in existing ng tests already, but do you > have any thoughts on the fact that setBodyInnerHTML() will actually lay out with > the current layout engine first? We may want our own test runner which just updates the document to updateStyleAndLayoutTree... i've been mainly using this just so that we can get LayoutObjects instead of building up style manually. Thoughts? https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:33: toLayoutBlockFlow(document().getElementById("child1")->layoutObject())); On 2017/02/22 13:11:31, mstensho wrote: > You can use getLayoutObjectByElementId("id") instead of > document().getElementById("id")->layoutObject(). Done. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:70: HeapVector<Member<NGBreakToken>> child_break_tokens2; On 2017/02/22 13:11:31, mstensho wrote: > Unused. Did you mean to use it below? Otherwise, just remove it. Re-using > child_break_tokens, like you currently do, should work fine, since > NGBlockBreakToken() eats the children. Removed, and from other test.
https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.cc:15: : NGBreakToken(kBlockBreakToken, /* is_finished */ false, node), may be use enum instead of boolean NGBlockBreakToken::Type { kNGBlockBreakTokenTypeUndefined = 0; kFinished = 1; } ? https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc (right): https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:17: NGLayoutInputNode* child = nullptr; can you move this initialization to the line #52? https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h (right): https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h:29: std::pair<NGLayoutInputNode*, NGBreakToken*> NextChild(); may be use a struct instead of std::pair ? NGBlockChildIterator::Entry ?
lgtm % glebl's comments (I tend to agree with them all, FWIW) https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:43: break; On 2017/02/22 18:47:19, ikilpatrick wrote: > On 2017/02/22 13:11:30, mstensho wrote: > > Let me see if I've got this right: > > > > Each child that we have previously started to lay out (and possibly also > > finished) has an entry in child_break_tokens, while children that we haven't > > visited at all yet don't? So, whether a child fragmented or not doesn't really > > matter; they'll all get break tokens once we're finished with them (or when > they > > actually break)? Yes? > > Yes thats exactly correct. I can try an encapsulate that in a comment if you > like. If it's a requirement that all finished nodes have break tokens, I'd like to have that documented somewhere, if it isn't already. Doing it in some follow-up CL would also be okay. If this isn't the most appropriate place, maybe right before the definition of NGBlockBreakToken::child_break_tokens_? Or before NGBlockBreakToken? https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:27: setBodyInnerHTML(R"HTML( On 2017/02/22 18:47:19, ikilpatrick wrote: > On 2017/02/22 13:11:31, mstensho wrote: > > I see that setBodyInnerHTML() is used in existing ng tests already, but do you > > have any thoughts on the fact that setBodyInnerHTML() will actually lay out > with > > the current layout engine first? > > We may want our own test runner which just updates the document to > updateStyleAndLayoutTree... i've been mainly using this just so that we can get > LayoutObjects instead of building up style manually. > > Thoughts? Yeah, something like that. Sorry for bringing it up in a review, though; that probably wasn't the right place. :) Just having the LayoutObject tree and computed style (without laying it out) would be a good first step. Building up style and input nodes manually is something few people would enjoy writing (or reading) all day long, so setBodyInnerHTML() is way superior to that. :)
Alternate names: NGBlockFlowChildrenIterator ? Entry -> ChildInfo? https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.cc:15: : NGBreakToken(kBlockBreakToken, /* is_finished */ false, node), On 2017/02/22 19:24:56, Gleb Lanbin wrote: > may be use enum instead of boolean NGBlockBreakToken::Type { > kNGBlockBreakTokenTypeUndefined = 0; > kFinished = 1; > } > ? Done. https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc (right): https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:17: NGLayoutInputNode* child = nullptr; On 2017/02/22 19:24:56, Gleb Lanbin wrote: > can you move this initialization to the line #52? Done. https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h (right): https://codereview.chromium.org/2706353004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.h:29: std::pair<NGLayoutInputNode*, NGBreakToken*> NextChild(); On 2017/02/22 19:24:56, Gleb Lanbin wrote: > may be use a struct instead of std::pair ? > NGBlockChildIterator::Entry ? Done.
> Alternate names: > NGBlockFlowChildrenIterator ? > Entry -> ChildInfo? I think "NGBlockChildIterator" is a good name (i.e. what you already have). "Entry" also WFM. "ChildInfo" isn't bad either, but it's longer. :) https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_break_token.h (right): https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:42: enum NGBreakTokenStatus { kUnFinished, kFinished }; Nit: Since it's spelled "unfinished", and not "un-finished", I think kUnfinished is the right name.
lgtm https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc (right): https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:17: TEST_F(NGBlockChildIteratorTest, testNullFirstChild) { 1) testNullFirstChild is a function name, so it should start with an uppercase character. 2) prefix "test" is redundant here and below. https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:36: EXPECT_EQ(NGBlockChildIterator::Entry(node1, nullptr), iterator.NextChild()); .nit s/EXPECT_EQ/ASSERT_EQ/g "Assert" will halt the execution of test while "expect" will let it continue. I think in your case each iterator's return depends on the previous one. It's better to use ASSERT_EQ here to avoid crashes in tests.
https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:43: break; On 2017/02/22 20:09:23, mstensho wrote: > On 2017/02/22 18:47:19, ikilpatrick wrote: > > On 2017/02/22 13:11:30, mstensho wrote: > > > Let me see if I've got this right: > > > > > > Each child that we have previously started to lay out (and possibly also > > > finished) has an entry in child_break_tokens, while children that we haven't > > > visited at all yet don't? So, whether a child fragmented or not doesn't > really > > > matter; they'll all get break tokens once we're finished with them (or when > > they > > > actually break)? Yes? > > > > Yes thats exactly correct. I can try an encapsulate that in a comment if you > > like. > > If it's a requirement that all finished nodes have break tokens, I'd like to > have that documented somewhere, if it isn't already. > Doing it in some follow-up CL would also be okay. > > If this isn't the most appropriate place, maybe right before the definition of > NGBlockBreakToken::child_break_tokens_? Or before NGBlockBreakToken? Clarified comment on NGBlockBreakToken. https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:27: setBodyInnerHTML(R"HTML( On 2017/02/22 20:09:23, mstensho wrote: > On 2017/02/22 18:47:19, ikilpatrick wrote: > > On 2017/02/22 13:11:31, mstensho wrote: > > > I see that setBodyInnerHTML() is used in existing ng tests already, but do > you > > > have any thoughts on the fact that setBodyInnerHTML() will actually lay out > > with > > > the current layout engine first? > > > > We may want our own test runner which just updates the document to > > updateStyleAndLayoutTree... i've been mainly using this just so that we can > get > > LayoutObjects instead of building up style manually. > > > > Thoughts? > > Yeah, something like that. Sorry for bringing it up in a review, though; that > probably wasn't the right place. :) > > Just having the LayoutObject tree and computed style (without laying it out) > would be a good first step. Building up style and input nodes manually is > something few people would enjoy writing (or reading) all day long, so > setBodyInnerHTML() is way superior to that. :) Np :), I might go through and normalize all our tests to look the same, at the moment its a little bit all over the place :/ https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc (right): https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:17: TEST_F(NGBlockChildIteratorTest, testNullFirstChild) { On 2017/02/22 20:52:10, Gleb Lanbin wrote: > 1) testNullFirstChild is a function name, so it should start with an uppercase > character. > 2) prefix "test" is redundant here and below. Done. https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc:36: EXPECT_EQ(NGBlockChildIterator::Entry(node1, nullptr), iterator.NextChild()); On 2017/02/22 20:52:10, Gleb Lanbin wrote: > .nit > > s/EXPECT_EQ/ASSERT_EQ/g "Assert" will halt the execution of test while "expect" > will let it continue. I think in your case each iterator's return depends on the > previous one. It's better to use ASSERT_EQ here to avoid crashes in tests. Done. https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_break_token.h (right): https://codereview.chromium.org/2706353004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:42: enum NGBreakTokenStatus { kUnFinished, kFinished }; On 2017/02/22 20:39:19, mstensho wrote: > Nit: Since it's spelled "unfinished", and not "un-finished", I think kUnfinished > is the right name. Done.
The CQ bit was checked by ikilpatrick@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 ikilpatrick@chromium.org
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com, glebl@chromium.org Link to the patchset: https://codereview.chromium.org/2706353004/#ps100001 (title: "address comments.")
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": 100001, "attempt_start_ts": 1487806018765410,
"parent_rev": "2ca532da23962857f4fda21095baaf227249f86c", "commit_rev":
"0e277884ee599a8a09b01c93c97934f3c8c285ac"}
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Introduce block child iterator. This will be used for iterating of children while resuming layout from a given break token. See experimental patch for the multi-col approach: crrev/2693193002 BUG=635619 ========== to ========== [LayoutNG] Introduce block child iterator. This will be used for iterating of children while resuming layout from a given break token. See experimental patch for the multi-col approach: crrev/2693193002 BUG=635619 Review-Url: https://codereview.chromium.org/2706353004 Cr-Commit-Position: refs/heads/master@{#452333} Committed: https://chromium.googlesource.com/chromium/src/+/0e277884ee599a8a09b01c93c979... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/0e277884ee599a8a09b01c93c979...
Message was sent while issue was closed.
https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc (right): https://codereview.chromium.org/2706353004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc:43: break; On 2017/02/22 22:25:21, ikilpatrick wrote: > On 2017/02/22 20:09:23, mstensho wrote: > > On 2017/02/22 18:47:19, ikilpatrick wrote: > > > On 2017/02/22 13:11:30, mstensho wrote: > > > > Let me see if I've got this right: > > > > > > > > Each child that we have previously started to lay out (and possibly also > > > > finished) has an entry in child_break_tokens, while children that we > haven't > > > > visited at all yet don't? So, whether a child fragmented or not doesn't > > really > > > > matter; they'll all get break tokens once we're finished with them (or > when > > > they > > > > actually break)? Yes? > > > > > > Yes thats exactly correct. I can try an encapsulate that in a comment if you > > > like. > > > > If it's a requirement that all finished nodes have break tokens, I'd like to > > have that documented somewhere, if it isn't already. > > Doing it in some follow-up CL would also be okay. > > > > If this isn't the most appropriate place, maybe right before the definition of > > NGBlockBreakToken::child_break_tokens_? Or before NGBlockBreakToken? > > Clarified comment on NGBlockBreakToken. Excellent! Thanks. |
