|
|
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] Add fields required for new multi-col approach.
Adds:
1) UsedBlockSize() - used to communicate how much block size was used by
preceeding fragments.
2) ChildBreakTokens() - used for resuming children in fragmented flow.
3) IsFinished() - if true the node cannot produce any more fragments. This
shouldn't have any child break tokens.
BUG=635619
Review-Url: https://codereview.chromium.org/2697843004
Cr-Commit-Position: refs/heads/master@{#451233}
Committed: https://chromium.googlesource.com/chromium/src/+/ecb3fae9e265de59f536e27b66baeb641e90fae9
Patch Set 1 #
Total comments: 16
Patch Set 2 : rebase. #Patch Set 3 : address comments. #
Messages
Total messages: 27 (13 generated)
ikilpatrick@chromium.org changed reviewers: + glebl@chromium.org, mstensho@opera.com
Obviously have a look at high level approach here: crrev.com/2693193002 :)
lgtm with nits https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:13: class NGLayoutInputNode; can we just include NGLayoutInputNode? https://google.github.io/styleguide/cppguide.html#Forward_Declarationss https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:43: bool IsFinished() const { return is_finished_; } .nit you used "exhausted" word to explain this case before. IsExhausted? https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:47: NGLayoutInputNode* InputNode() const { return node_.get(); } just return node_;
cbiesinger@chromium.org changed reviewers: + cbiesinger@chromium.org
https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:23: : NGBreakToken(kBlockBreakToken, /* is_finished */ false, node), Consider an enum instead of a bool?
https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:41: // size is 150px the next fragment should have a size of 50px (assuming no Also, I'm confused by this comment. Typically blocks do not fill the available size in the block direction. Why should the next fragment be exactly 50px high in this example?
(just replying to cbiesinger atm). https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:41: // size is 150px the next fragment should have a size of 50px (assuming no On 2017/02/15 19:24:22, cbiesinger wrote: > Also, I'm confused by this comment. Typically blocks do not fill the available > size in the block direction. Why should the next fragment be exactly 50px high > in this example? So consider this: https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E... In the above example the first fragment is 75px high as it hit a fragmentation line at 75px. On the break_token for the first fragment it's used_block_size is 75px. Inside the second fragment it calculates it's size by: (block_size - used_block_size) = (150px - 75px) = 75x. Would a better comment be? Represents the amount of block size used in previous fragments. E.g. if the layout node has a definite height of 200px, and the used block size of preceding fragments is 150px the next fragment will have a block size of 50px (assuming no additional fragmentation).
I don't think I have any further comments here. Just a rant about compilation times. https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:13: class NGLayoutInputNode; On 2017/02/15 19:21:39, Gleb Lanbin wrote: > can we just include NGLayoutInputNode? > https://google.github.io/styleguide/cppguide.html#Forward_Declarationss Chromium style thankfully overrides this, though: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Compilation times just keep growing. :(
cbiesinger@google.com changed reviewers: + cbiesinger@google.com
https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:41: // size is 150px the next fragment should have a size of 50px (assuming no On 2017/02/15 at 19:45:35, ikilpatrick wrote: > On 2017/02/15 19:24:22, cbiesinger wrote: > > Also, I'm confused by this comment. Typically blocks do not fill the available > > size in the block direction. Why should the next fragment be exactly 50px high > > in this example? > > So consider this: > https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E... > > In the above example the first fragment is 75px high as it hit a fragmentation line at 75px. On the break_token for the first fragment it's used_block_size is 75px. > > Inside the second fragment it calculates it's size by: (block_size - used_block_size) = (150px - 75px) = 75x. > > Would a better comment be? > > Represents the amount of block size used in previous fragments. > E.g. if the layout node has a definite height of 200px, and the used block size of preceding fragments is 150px the next fragment will have a block size of 50px (assuming no additional fragmentation). Oh I see. I missed the fact that 200px is the size of this object, not its container. Hm.. maybe just say "used size of its preceding fragments"? Or "of the previous fragments of this block"?
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:23: : NGBreakToken(kBlockBreakToken, /* is_finished */ false, node), On 2017/02/15 19:23:19, cbiesinger wrote: > Consider an enum instead of a bool? Yeah I looked at doing that, but decided against as it's just internal to this class hierarchy, and is publicly exposed as different ctors, and the IsFinished() method. https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:41: // size is 150px the next fragment should have a size of 50px (assuming no On 2017/02/16 00:14:55, cbiesinger1 wrote: > On 2017/02/15 at 19:45:35, ikilpatrick wrote: > > On 2017/02/15 19:24:22, cbiesinger wrote: > > > Also, I'm confused by this comment. Typically blocks do not fill the > available > > > size in the block direction. Why should the next fragment be exactly 50px > high > > > in this example? > > > > So consider this: > > > https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E... > > > > In the above example the first fragment is 75px high as it hit a fragmentation > line at 75px. On the break_token for the first fragment it's used_block_size is > 75px. > > > > Inside the second fragment it calculates it's size by: (block_size - > used_block_size) = (150px - 75px) = 75x. > > > > Would a better comment be? > > > > Represents the amount of block size used in previous fragments. > > E.g. if the layout node has a definite height of 200px, and the used block > size of preceding fragments is 150px the next fragment will have a block size of > 50px (assuming no additional fragmentation). > > Oh I see. I missed the fact that 200px is the size of this object, not its > container. Hm.. maybe just say "used size of its preceding fragments"? Or "of > the previous fragments of this block"? Tried some rewording... https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:13: class NGLayoutInputNode; On 2017/02/15 21:25:38, mstensho wrote: > On 2017/02/15 19:21:39, Gleb Lanbin wrote: > > can we just include NGLayoutInputNode? > > https://google.github.io/styleguide/cppguide.html#Forward_Declarationss > > Chromium style thankfully overrides this, though: > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Compilation times just keep growing. :( We should try and push for keeping the forward declare rule in a pre-condition check... :/ https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:43: bool IsFinished() const { return is_finished_; } On 2017/02/15 19:21:38, Gleb Lanbin wrote: > .nit you used "exhausted" word to explain this case before. IsExhausted? I'll keep as IsFinished, but reworded above... https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_break_token.h:47: NGLayoutInputNode* InputNode() const { return node_.get(); } On 2017/02/15 19:21:39, Gleb Lanbin wrote: > just return node_; Done.
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/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:23: : NGBreakToken(kBlockBreakToken, /* is_finished */ false, node), On 2017/02/16 17:01:30, ikilpatrick wrote: > On 2017/02/15 19:23:19, cbiesinger wrote: > > Consider an enum instead of a bool? > > Yeah I looked at doing that, but decided against as it's just internal to this > class hierarchy, and is publicly exposed as different ctors, and the > IsFinished() method. I can also change if you feel strongly about this :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (right): https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:23: : NGBreakToken(kBlockBreakToken, /* is_finished */ false, node), On 2017/02/16 17:02:10, ikilpatrick wrote: > On 2017/02/16 17:01:30, ikilpatrick wrote: > > On 2017/02/15 19:23:19, cbiesinger wrote: > > > Consider an enum instead of a bool? > > > > Yeah I looked at doing that, but decided against as it's just internal to this > > class hierarchy, and is publicly exposed as different ctors, and the > > IsFinished() method. > > I can also change if you feel strongly about this :) Well, I think /* is_finished */ false looks hideous so I still prefer an enum but I don't care strongly enough that I'll insist on it. :) https://codereview.chromium.org/2697843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:41: // size is 150px the next fragment should have a size of 50px (assuming no On 2017/02/16 17:01:30, ikilpatrick wrote: > On 2017/02/16 00:14:55, cbiesinger1 wrote: > > On 2017/02/15 at 19:45:35, ikilpatrick wrote: > > > On 2017/02/15 19:24:22, cbiesinger wrote: > > > > Also, I'm confused by this comment. Typically blocks do not fill the > > available > > > > size in the block direction. Why should the next fragment be exactly 50px > > high > > > > in this example? > > > > > > So consider this: > > > > > > https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E... > > > > > > In the above example the first fragment is 75px high as it hit a > fragmentation > > line at 75px. On the break_token for the first fragment it's used_block_size > is > > 75px. > > > > > > Inside the second fragment it calculates it's size by: (block_size - > > used_block_size) = (150px - 75px) = 75x. > > > > > > Would a better comment be? > > > > > > Represents the amount of block size used in previous fragments. > > > E.g. if the layout node has a definite height of 200px, and the used block > > size of preceding fragments is 150px the next fragment will have a block size > of > > 50px (assuming no additional fragmentation). > > > > Oh I see. I missed the fact that 200px is the size of this object, not its > > container. Hm.. maybe just say "used size of its preceding fragments"? Or "of > > the previous fragments of this block"? > > Tried some rewording... Thanks!
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from glebl@chromium.org Link to the patchset: https://codereview.chromium.org/2697843004/#ps40001 (title: "address comments.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ikilpatrick@chromium.org
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": 40001, "attempt_start_ts": 1487309769069840, "parent_rev": "4ba96148ab4fea7ae4f535c7e7bfdb37d9daf027", "commit_rev": "ecb3fae9e265de59f536e27b66baeb641e90fae9"}
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Add fields required for new multi-col approach. Adds: 1) UsedBlockSize() - used to communicate how much block size was used by preceeding fragments. 2) ChildBreakTokens() - used for resuming children in fragmented flow. 3) IsFinished() - if true the node cannot produce any more fragments. This shouldn't have any child break tokens. BUG=635619 ========== to ========== [LayoutNG] Add fields required for new multi-col approach. Adds: 1) UsedBlockSize() - used to communicate how much block size was used by preceeding fragments. 2) ChildBreakTokens() - used for resuming children in fragmented flow. 3) IsFinished() - if true the node cannot produce any more fragments. This shouldn't have any child break tokens. BUG=635619 Review-Url: https://codereview.chromium.org/2697843004 Cr-Commit-Position: refs/heads/master@{#451233} Committed: https://chromium.googlesource.com/chromium/src/+/ecb3fae9e265de59f536e27b66ba... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ecb3fae9e265de59f536e27b66ba... |