Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681783004:
Use Initial Containing Block size for vertical (Closed)
|
Created:
3 years, 10 months ago by atotic Modified:
3 years, 10 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, 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. |
DescriptionUse Initial Containing Block size for vertical
This patch fixes the vertical modes assert in ComputeInlineLength.
I used box.view()->viewportSizeForViewportUnits() for ICB size
per kojii's suggestion.
BUG=635619
Review-Url: https://codereview.chromium.org/2681783004
Cr-Commit-Position: refs/heads/master@{#449818}
Committed: https://chromium.googlesource.com/chromium/src/+/214a55c8dbf669d7d16e8693ffd93ae0d9d10182
Patch Set 1 #
Total comments: 1
Patch Set 2 : make availableSize always definite #Patch Set 3 : Update to master #Patch Set 4 : Better merge of ToConstraintSpace #
Total comments: 30
Patch Set 5 : CR fixes Make ICB size ICB width #Patch Set 6 : Builder: fix for htb, add tests #
Total comments: 16
Patch Set 7 : Final CR fixes #Messages
Total messages: 46 (14 generated)
atotic@chromium.org changed reviewers: + cbiesinger@chromium.org, kojii@chromium.org
Description was changed from ========== Use Initial Containing Block size for vertical BUG=635619 ========== to ========== Use Initial Containing Block size for vertical This patch fixes the vertical modes assert in ComputeInlineLength. I used box.view()->viewportSizeForViewportUnits() for ICB size per kojii's suggestion. BUG=635619 ==========
ikilpatrick@chromium.org changed reviewers: + ikilpatrick@chromium.org
https://codereview.chromium.org/2681783004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc (right): https://codereview.chromium.org/2681783004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:63: constraint_space.DefiniteAvailableSize().inline_size; This doesn't seem like the right solution here, can you like a test case which is currently failing? My intuition is that we'd want to adjust the Available/PercentageSize in the ng_constraint_space_builder if the writing modes are orthogonal?
On Feb 9, 2017 1:58 AM, <ikilpatrick@chromium.org> wrote: My intuition is that we'd want to adjust the Available/PercentageSize in the ng_constraint_space_builder if the writing modes are orthogonal? I suggested the same thing in a chat with Aleks :) -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Feb 9, 2017 1:58 AM, <ikilpatrick@chromium.org> wrote: My intuition is that we'd want to adjust the Available/PercentageSize in the ng_constraint_space_builder if the writing modes are orthogonal? I suggested the same thing in a chat with Aleks :) -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
You've convinced me. I've reworked the code, now AvailabelSize() is always definite. This is enforced in CSBuilder.
ptal
lgtm https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:175: return new NGConstraintSpace( It's unfortunate that we still need the two separate new NGConstraintSpace calls since they are increasingly similar :/
The CQ bit was checked by atotic@chromium.org
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
https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:64: NGPhysicalSize InitialContainingBlockSize() const { This should never be used by an algorithm right? Add comment? https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:143: // If inline size < 0, use height of initial containing block If the inline size is indefinite use the height of the initial containing block. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:146: if (available_size.inline_size < 0) { This should just be an indefinite heck instead? < 0 is a bit sloppy as it encourages code using this to do math which is invalid. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:150: if (percentage_resolution_size.inline_size < 0) { Same here. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:153: initial_containing_block_size_.height; This seems wrong, should this be the block size in orthogonal writing modes? It's only ever the ICB height for initial block?
glebl@chromium.org changed reviewers: + glebl@chromium.org
some minor comments. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:134: NGPhysicalSize initial_containing_block_size, .nit const NGPhysicalSize& https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:134: NGPhysicalSize initial_containing_block_size, .nit please document this variable. It's not clear what the initial containing block size mean. // Default constructor. // @param initial_containing_block_size ... https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:59: NGConstraintSpaceBuilder::SetInitialContainingBlockSize(NGPhysicalSize size) { const NGPhysicalSize& https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:60: initial_containing_block_size_ = size; icb_size_ = size https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:135: NGLogicalSize available_size = available_size_; why are you creating these temp variables here? https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:145: if (static_cast<NGWritingMode>(writing_mode_) != kHorizontalTopBottom) { maybe bool is_in_vertical_mode = static_cast<NGWritingMode>(writing_mode_) != kHorizontalTopBottom; if (is_in_vertical_mode) { ... } https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:71: NGPhysicalSize initial_containing_block_size_; icb_size_ ? but you need document ICB abbreviate. You are already using it in anyways.
Also can you add tests? https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:64: NGPhysicalSize InitialContainingBlockSize() const { Actually can you place this down into the private section after the constructor? Only the builder should ever use this. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:153: initial_containing_block_size_.height; I would check with Koji or the spec as to what is the expected behaviour here. The spec says it should resolve off the block size instead. FF is broken, edge I think have copied our behaviour. @koji should this be width if child is horizontal?
cbiesinger@google.com changed reviewers: + cbiesinger@google.com
https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:153: initial_containing_block_size_.height; On 2017/02/09 at 22:47:32, ikilpatrick wrote: > I would check with Koji or the spec as to what is the expected behaviour here. The spec says it should resolve off the block size instead. FF is broken, edge I think have copied our behaviour. > > @koji should this be width if child is horizontal? I don't really understand your comment, what block size should it use, in which circumstances?
On 2017/02/09 at 22:47:33, ikilpatrick wrote: > Also can you add tests? What kind of test? Only one I can think of is an end-to-end test in ng_block_layout_algorithm. Constraint spaces are not visible to outside code, you can only test indirectly through side effects.
Replies so far. Unresolved: Ian: - comment on whether horizontal blocks should get definite inline size from ICB.width. Waiting on koji - should NGIndefiniteSize be moved to ng_units.h from ng_length_utils.h. I recommend yes. - what kinds of tests should be written. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:64: NGPhysicalSize InitialContainingBlockSize() const { On 2017/02/09 at 22:14:27, ikilpatrick wrote: > This should never be used by an algorithm right? Add comment? I just added it for completeness. If you think it should never be used, I'll take it out. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:64: NGPhysicalSize InitialContainingBlockSize() const { On 2017/02/09 at 22:47:32, ikilpatrick wrote: > Actually can you place this down into the private section after the constructor? Only the builder should ever use this. done. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:134: NGPhysicalSize initial_containing_block_size, On 2017/02/09 at 22:40:29, Gleb Lanbin wrote: > .nit const NGPhysicalSize& done. We should agree on one standard or another. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:59: NGConstraintSpaceBuilder::SetInitialContainingBlockSize(NGPhysicalSize size) { On 2017/02/09 at 22:40:29, Gleb Lanbin wrote: > const NGPhysicalSize& done. Now inconsistent with SetAvailableSize() https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:60: initial_containing_block_size_ = size; On 2017/02/09 at 22:40:29, Gleb Lanbin wrote: > icb_size_ = size done. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:135: NGLogicalSize available_size = available_size_; On 2017/02/09 at 22:40:29, Gleb Lanbin wrote: > why are you creating these temp variables here? because I have to swap them. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:143: // If inline size < 0, use height of initial containing block On 2017/02/09 at 22:14:32, ikilpatrick wrote: > If the inline size is indefinite use the height of the initial containing block. done. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:145: if (static_cast<NGWritingMode>(writing_mode_) != kHorizontalTopBottom) { On 2017/02/09 at 22:40:29, Gleb Lanbin wrote: > maybe > bool is_in_vertical_mode = static_cast<NGWritingMode>(writing_mode_) != kHorizontalTopBottom; > if (is_in_vertical_mode) { > ... > } no. i do not find it any simpler. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:146: if (available_size.inline_size < 0) { On 2017/02/09 at 22:14:30, ikilpatrick wrote: > This should just be an indefinite heck instead? < 0 is a bit sloppy as it encourages code using this to do math which is invalid. NGSizeIndefinite is defined inside ng_length_utils.h, which I did not want to include for a single constant. If we are going to use NGSizeIndefinite outside of ng_length stuff, we should move NGSizeIndefinite to ng_units. Should I do that? https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:150: if (percentage_resolution_size.inline_size < 0) { On 2017/02/09 at 22:14:31, ikilpatrick wrote: > Same here. Same answer. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:153: initial_containing_block_size_.height; On 2017/02/09 at 22:14:29, ikilpatrick wrote: > This seems wrong, should this be the block size in orthogonal writing modes? It's only ever the ICB height for initial block? Huh? We are trying to fix inline_size in orthogonal mode, and that is defined as ICB height. I do not understand what do you see. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:175: return new NGConstraintSpace( On 2017/02/09 at 21:36:03, cbiesinger wrote: > It's unfortunate that we still need the two separate new NGConstraintSpace calls since they are increasingly similar :/ Yep. They look the same, but they are not. https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:71: NGPhysicalSize initial_containing_block_size_; On 2017/02/09 at 22:40:29, Gleb Lanbin wrote: > icb_size_ ? but you need document ICB abbreviate. You are already using it in anyways. no. rarely used.
Another fix, after hangout converasion with ikilpatrick/eae - Only compute ICB height, no current use for width.
On 2017/02/10 00:11:34, atotic wrote:
> Another fix, after hangout converasion with ikilpatrick/eae
>
> - Only compute ICB height, no current use for width.
- should NGIndefiniteSize be moved to ng_units.h from ng_length_utils.h. I
recommend yes.
Yup, sounds good.
- what kinds of tests should be written.
Yeah unit test in ng_block_layout_algorithm_test.cc
Just something which does setBodyInnerHTML to:
<style>
#vertical {
writing-mode: vertical-rl;
height: 50%;
}
#horizontal {
writing-mode: horizontal-tb;
width: 50%;
}
</style>
<div id="vertical">
<div id="horizontal"></div>
</div>
On 2017/02/10 02:02:17, ikilpatrick wrote:
> On 2017/02/10 00:11:34, atotic wrote:
> > Another fix, after hangout converasion with ikilpatrick/eae
> >
> > - Only compute ICB height, no current use for width.
>
> - should NGIndefiniteSize be moved to ng_units.h from ng_length_utils.h. I
> recommend yes.
>
> Yup, sounds good.
>
> - what kinds of tests should be written.
>
> Yeah unit test in ng_block_layout_algorithm_test.cc
>
> Just something which does setBodyInnerHTML to:
> <style>
> #vertical {
> writing-mode: vertical-rl;
> height: 50%;
> }
>
> #horizontal {
> writing-mode: horizontal-tb;
> width: 50%;
> }
> </style>
> <div id="vertical">
> <div id="horizontal"></div>
> </div>
.... /sigh pressed send message to soon...
and checks that vertical height is 50% of whatever you set the spaces ICB height
to.
and checks the horizontal width. :)
https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:153: initial_containing_block_size_.height; Sorry for late reply; IIUC what Ian meant was when the root element is vertical, and you have horizontal element. Ian is right, in this case, we need to fallback to ICB width instead because it's the block size. We have crbug.com/583207 for this case. (Note, the spec copied Trident behavior, not Edge copied ours ;-)
On 2017/02/10 at 02:03:36, ikilpatrick wrote:
> On 2017/02/10 02:02:17, ikilpatrick wrote:
> > On 2017/02/10 00:11:34, atotic wrote:
> > > Another fix, after hangout converasion with ikilpatrick/eae
> > >
> > > - Only compute ICB height, no current use for width.
> >
> > - should NGIndefiniteSize be moved to ng_units.h from ng_length_utils.h. I
> > recommend yes.
> >
> > Yup, sounds good.
> >
> > - what kinds of tests should be written.
> >
> > Yeah unit test in ng_block_layout_algorithm_test.cc
> >
> > Just something which does setBodyInnerHTML to:
> > <style>
> > #vertical {
> > writing-mode: vertical-rl;
> > height: 50%;
> > }
> >
> > #horizontal {
> > writing-mode: horizontal-tb;
> > width: 50%;
> > }
> > </style>
> > <div id="vertical">
> > <div id="horizontal"></div>
> > </div>
>
> .... /sigh pressed send message to soon...
>
> and checks that vertical height is 50% of whatever you set the spaces ICB
height to.
> and checks the horizontal width. :)
This is an end-to-end test. Many tests that exercise this already exist inside
LayoutTests. What is the value of adding a Unit test that is really an
end-to-end?
The reason that I am fixing this bug is that it is impossible to run many
orthogonal mode tests without it.
atotic@chromium.org changed reviewers: + eae@chromium.org
ptal eae, I'll need lgtm for BUILD.gn Changes: - Builder: horizontal blocks inside vertical now get inline_size from ICB - Builder: gets minimalist test suite - NGSizeIndefinite moved to ng_units
lgtm thanks for your patience. https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc (right): https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:13: class NGConstraintSpaceBuilderTest : public RenderingTest { can just remove this. https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:25: // This test exercises this loop by placing two fixed elements inside abs. nope fix comment :) https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:26: TEST_F(NGConstraintSpaceBuilderTest, AvailableSizeFromHorizontalICB) { can just be TEST(...) if remove test class above. https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:68: }; nl after this. https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:69: } needs "// namespace" comment https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:70: } needs "// namespace blink" comment
https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:148: available_size.inline_size = initial_containing_block_size_.width; I am still skeptical how this can happen? We should inherit this through the constraint spaces, right? Koji / Ian, do you have a testcase where this would not be definite already? https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_units.h:21: #define NGSizeIndefinite LayoutUnit(-1) Thanks! I've been meaning to move this.
https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc (right): https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:13: class NGConstraintSpaceBuilderTest : public RenderingTest { On 2017/02/10 at 22:54:14, ikilpatrick wrote: > can just remove this. done https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:25: // This test exercises this loop by placing two fixed elements inside abs. On 2017/02/10 at 22:54:13, ikilpatrick wrote: > nope fix comment :) done. Good catch, that was subtle. https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:26: TEST_F(NGConstraintSpaceBuilderTest, AvailableSizeFromHorizontalICB) { On 2017/02/10 at 22:54:13, ikilpatrick wrote: > can just be TEST(...) if remove test class above. done https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:68: }; On 2017/02/10 at 22:54:13, ikilpatrick wrote: > nl after this. done https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:69: } On 2017/02/10 at 22:54:13, ikilpatrick wrote: > needs "// namespace" comment done https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder_test.cc:70: } On 2017/02/10 at 22:54:13, ikilpatrick wrote: > needs "// namespace blink" comment done
The CQ bit was checked by atotic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2681783004/#ps120001 (title: "Final CR fixes")
The CQ bit was unchecked by atotic@chromium.org
@eae can I get the LGTM for BUILD.GN
On 2017/02/10 23:29:42, atotic wrote: > @eae can I get the LGTM for BUILD.GN Shouldn't my lgtm be good enough for that?
https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:148: available_size.inline_size = initial_containing_block_size_.width; On 2017/02/10 23:26:05, cbiesinger wrote: > I am still skeptical how this can happen? We should inherit this through the > constraint spaces, right? > > Koji / Ian, do you have a testcase where this would not be definite already? Yeah basically this: <!DOCTYPE html> <style> body { writing-mode: vertical-rl; } #vertical { writing-mode: vertical-rl; height: 50%; outline: red solid 1px; } #horizontal { writing-mode: horizontal-tb; width: 50%; outline: purple solid 3px; } #vert2 { writing-mode: vertical-rl; outline: green solid 5px; height: 50%; } </style> <div id="vertical"> <div id="horizontal"> <div id="vert2"></div> </div> </div> The horizontal element on this will have an indefinite available size. It resolves to 50% of ICB. I think we have a bug in our current engine for this.
> Shouldn't my lgtm be good enough for that? didn't know you have those too.
https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2681783004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:148: available_size.inline_size = initial_containing_block_size_.width; html { writing-mode: vertical-rl; } </style> <div style="writing-mode:horizontal-tb;border: 5px solid black"> <p>hi</p> </div> triggers it.
The CQ bit was checked by atotic@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/10 at 23:37:21, ikilpatrick wrote: > > I think we have a bug in our current engine for this. Right, http://debug.com/583207
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486769965236420,
"parent_rev": "ba288972a7a0083806ff09e16feff9496ca6808d", "commit_rev":
"214a55c8dbf669d7d16e8693ffd93ae0d9d10182"}
Message was sent while issue was closed.
Description was changed from ========== Use Initial Containing Block size for vertical This patch fixes the vertical modes assert in ComputeInlineLength. I used box.view()->viewportSizeForViewportUnits() for ICB size per kojii's suggestion. BUG=635619 ========== to ========== Use Initial Containing Block size for vertical This patch fixes the vertical modes assert in ComputeInlineLength. I used box.view()->viewportSizeForViewportUnits() for ICB size per kojii's suggestion. BUG=635619 Review-Url: https://codereview.chromium.org/2681783004 Cr-Commit-Position: refs/heads/master@{#449818} Committed: https://chromium.googlesource.com/chromium/src/+/214a55c8dbf669d7d16e8693ffd9... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/214a55c8dbf669d7d16e8693ffd9...
Message was sent while issue was closed.
\o/ |
