Fix calculation of abspos style properties: left/padding/width/height...

Fixes calculation of abspos style properties:
left/padding/width/height...
Replaced NGSizeIndefinite with Optional<LayoutUnit>

Fixed a couple of minor bugs where AvailableSize was incorrect.

Passing ~30 more tests.

BUG=635619
Review-Url: https://codereview.chromium.org/2616613003
Cr-Commit-Position: refs/heads/master@{#441881}
Committed: https://chromium.googlesource.com/chromium/src/+/49b2b8a8727853391ce1f38de8044cfd4678ed1f

[atotic, 2017-01-05 01:18:37]

Description was changed from

==========
Fix calculation of abspos style properties: left/padding/width/height...

Passing 39 more tests

BUG=635619
==========

to

==========
Fixes calculation of abspos style properties:
left/padding/width/height...
Replaced NGSizeIndefinite with Optional<LayoutUnit>

Fixed a couple of minor bugs where AvailableSize was incorrect.

Passing 39 more tests.

BUG=635619
==========

[atotic, 2017-01-05 01:18:37]

atotic@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[atotic, 2017-01-05 01:20:38]

ptal. This is the (hopefully) last large abspos CR.

It bring computation of left/width/margins/padding... to spec,
and abolishes NGSizeIndefinite.

Fixes couple of minor AvailableSize bugs.

[atotic, 2017-01-05 05:36:58]

On 2017/01/05 at 01:20:38, atotic wrote:
> ptal. This is the (hopefully) last large abspos CR.
> 
> It bring computation of left/width/margins/padding... to spec,
> and abolishes NGSizeIndefinite.
> 
> Fixes couple of minor AvailableSize bugs.

I just took a look at remaining failing abspos LayoutTests.
I'd love to either make them pass, or understand why they fail.

Some stuff I've noticed: absbox positioned on top of border,
absbox with an incorrect offset.

Do you think it is better to review the patch now as is,
or would you like to wait until all tests are done?

I'd prefer to do a large checkin in now, then post minor 
fixes for the bugs I find. It makes for a simpler code review.
But I am happy with either decision.

[cbiesinger, 2017-01-05 16:48:23]

> Do you think it is better to review the patch now as is

Yes :)

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right):

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:42:
valueForLength(style.paddingLeft(), percentage_logical.block_size);
Why is this resolved against the block size?

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:194:
valueForLength(style.paddingTop(), percentage_logical.block_size);
This should also use inline_size, right?

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:367: child_minmax
= MinAndMaxContentSizes{*child_block_size, *child_block_size};
This should use the overflow size, I think?

[atotic, 2017-01-05 19:03:26]

2 bugs fixed, got one long question:

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right):

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:42:
valueForLength(style.paddingLeft(), percentage_logical.block_size);
On 2017/01/05 at 16:48:23, cbiesinger wrote:
> Why is this resolved against the block size?

done. also paddingRight

my brain still has a hard time with all the coordinate transformations.

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:194:
valueForLength(style.paddingTop(), percentage_logical.block_size);
On 2017/01/05 at 16:48:23, cbiesinger wrote:
> This should also use inline_size, right?

done.

https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:367: child_minmax
= MinAndMaxContentSizes{*child_block_size, *child_block_size};
On 2017/01/05 at 16:48:23, cbiesinger wrote:
> This should use the overflow size, I think?

I am unsure. child_minmax was originally used for 
ShrinkToFit inside abspos algorithm, now it is also 
used for ResolveInlineLength. Might need to separate them.

There is a question at the end of this long note.

Here is my understanding, want to run it by you before doing
anything:

1) Original MinAndMaxContentSizes usage: compute ShrinkToFit

ComputePartialAbsoluteWithChildInlineSize uses 
computed child_minmax for ShrinkToFit size.

After partial inline has been computed, ShrinkToFit block size 
is already known (it is the block size given computed inline size).

This known size is expressed by creating MinAndMaxContentSizes 
where min = max = known size.

ComputeFullAbsoluteWithChildBlockSize uses the 'known size' variant of
MinAndMaxContentSizes.

2) New MinAndMaxContentSizes usage: ResolveInlineLength

ResolveInlineLength uses MinAndMaxContentSizes to resolve 
min-width, max-width and family.

ComputePartialAbsoluteWithChildInlineSize needs computed minmax.
ComputeFullAbsoluteWithChildBlockSize also needs computed minmax.

### Conclusion:

Need to modify ComputeFullAbsoluteWithChildBlockSize to capture
new usage of MinAndMaxContentSizes.

### Question: What should MinAndMaxContentSizes be for ResolveInlineLength
inside ComputeFullAbsoluteWithChildBlockSize?

Spec:
min-content block size is equivalent to the max-content block size.
max-content block size the box’s “ideal” size in the block axis. Usually
the block size of the content after layout.

If I am understanding this correctly, this is the same known size trick
I used for ShrinkToFit, and my existing code is correct?

[cbiesinger, 2017-01-05 19:10:20]

On 2017/01/05 19:03:26, atotic wrote:
> 2 bugs fixed, got one long question:
> 
>
https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right):
> 
>
https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:42:
> valueForLength(style.paddingLeft(), percentage_logical.block_size);
> On 2017/01/05 at 16:48:23, cbiesinger wrote:
> > Why is this resolved against the block size?
> 
> done. also paddingRight
> 
> my brain still has a hard time with all the coordinate transformations.
> 
>
https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:194:
> valueForLength(style.paddingTop(), percentage_logical.block_size);
> On 2017/01/05 at 16:48:23, cbiesinger wrote:
> > This should also use inline_size, right?
> 
> done.
> 
>
https://codereview.chromium.org/2616613003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:367:
child_minmax
> = MinAndMaxContentSizes{*child_block_size, *child_block_size};
> On 2017/01/05 at 16:48:23, cbiesinger wrote:
> > This should use the overflow size, I think?
> 
> I am unsure. child_minmax was originally used for 
> ShrinkToFit inside abspos algorithm, now it is also 
> used for ResolveInlineLength. Might need to separate them.
> 
> There is a question at the end of this long note.
> 
> Here is my understanding, want to run it by you before doing
> anything:
> 
> 1) Original MinAndMaxContentSizes usage: compute ShrinkToFit
> 
> ComputePartialAbsoluteWithChildInlineSize uses 
> computed child_minmax for ShrinkToFit size.
> 
> After partial inline has been computed, ShrinkToFit block size 
> is already known (it is the block size given computed inline size).
> 
> This known size is expressed by creating MinAndMaxContentSizes 
> where min = max = known size.
> 
> ComputeFullAbsoluteWithChildBlockSize uses the 'known size' variant of
> MinAndMaxContentSizes.
> 
> 2) New MinAndMaxContentSizes usage: ResolveInlineLength
> 
> ResolveInlineLength uses MinAndMaxContentSizes to resolve 
> min-width, max-width and family.
> 
> ComputePartialAbsoluteWithChildInlineSize needs computed minmax.
> ComputeFullAbsoluteWithChildBlockSize also needs computed minmax.
> 
> ### Conclusion:
> 
> Need to modify ComputeFullAbsoluteWithChildBlockSize to capture
> new usage of MinAndMaxContentSizes.
> 
> ### Question: What should MinAndMaxContentSizes be for ResolveInlineLength
> inside ComputeFullAbsoluteWithChildBlockSize?
> 
> Spec:
> min-content block size is equivalent to the max-content block size.
> max-content block size the box’s “ideal” size in the block axis. Usually
> the block size of the content after layout.
> 
> If I am understanding this correctly, this is the same known size trick
> I used for ShrinkToFit, and my existing code is correct?

I think you're right that in practice this makes no difference here because if
height is given as a fixed value we don't even look at this argument, and in all
cases where we do look at it the known size is the size we want.

[atotic, 2017-01-05 19:25:59]

ptal

padding: block_size => inline has changed.

[cbiesinger, 2017-01-05 19:27:54]

lgtm

[atotic, 2017-01-05 19:30:45]

The CQ bit was checked by atotic@chromium.org

[commit-bot: I haz the power, 2017-01-05 19:31:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 21:25:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 21:25:47]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[atotic, 2017-01-05 23:56:53]

The CQ bit was checked by atotic@chromium.org

[atotic, 2017-01-05 23:56:53]

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/2616613003/#ps40001
(title: "Reduce TestExpectations")

[commit-bot: I haz the power, 2017-01-05 23:57:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 02:14:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:14:49]

Failed to apply patch for
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:9
  error: third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:
patch does not apply

Patch:      
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
Index: third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
diff --git
a/third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
b/third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
index
ec503ba1aac09d3ee709fe80fd0bf0eacffdcb5e..818f7a353d24fe403b7ca733b8b0fdd1f1efa74b
100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
@@ -9,6 +9,7 @@
 #include "core/layout/ng/ng_constraint_space_builder.h"
 #include "core/layout/ng/ng_fragment_base.h"
 #include "core/layout/ng/ng_length_utils.h"
+#include "core/layout/ng/ng_physical_fragment.h"
 #include "core/style/ComputedStyle.h"
 
 namespace blink {
@@ -34,6 +35,7 @@ NGOutOfFlowLayoutPart::NGOutOfFlowLayoutPart(
   NGConstraintSpaceBuilder space_builder(
       FromPlatformWritingMode(container_style->getWritingMode()));
   space_builder.SetAvailableSize(space_size);
+  space_builder.SetPercentageResolutionSize(space_size);
   space_builder.SetIsNewFormattingContext(true);
   space_builder.SetTextDirection(container_style->direction());
   parent_space_ = space_builder.ToConstraintSpace();
@@ -107,8 +109,7 @@ bool NGOutOfFlowLayoutPart::ComputeInlineSizeEstimate() {
   if (AbsoluteNeedsChildInlineSize(*node_->Style())) {
     MinAndMaxContentSizes size;
     if (node_->ComputeMinAndMaxContentSizes(&size)) {
-      inline_estimate_ =
-          size.ShrinkToFit(parent_space_->AvailableSize().inline_size);
+      inline_estimate_ = size;
       return true;
     }
     return false;
@@ -145,7 +146,7 @@ bool NGOutOfFlowLayoutPart::ComputeNodeFragment() {
     if (block_estimate_)
       available_size.block_size = *block_estimate_;
     builder.SetAvailableSize(available_size);
-    builder.SetPercentageResolutionSize(available_size);
+    builder.SetPercentageResolutionSize(parent_space_->AvailableSize());
     if (block_estimate_)
       builder.SetIsFixedSizeBlock(true);
     builder.SetIsFixedSizeInline(true);

[atotic, 2017-01-06 04:03:29]

The CQ bit was checked by atotic@chromium.org

[atotic, 2017-01-06 04:03:30]

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/2616613003/#ps60001
(title: "Rebase for merge conflicts")

[commit-bot: I haz the power, 2017-01-06 04:03:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[atotic, 2017-01-06 04:03:48]

Description was changed from

==========
Fixes calculation of abspos style properties:
left/padding/width/height...
Replaced NGSizeIndefinite with Optional<LayoutUnit>

Fixed a couple of minor bugs where AvailableSize was incorrect.

Passing 39 more tests.

BUG=635619
==========

to

==========
Fixes calculation of abspos style properties:
left/padding/width/height...
Replaced NGSizeIndefinite with Optional<LayoutUnit>

Fixed a couple of minor bugs where AvailableSize was incorrect.

Passing ~30 more tests.

BUG=635619
==========

[commit-bot: I haz the power, 2017-01-06 05:28:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483675409854600,
"parent_rev": "777a40893e4f879bf17464406f9f7d445404b28b", "commit_rev":
"49b2b8a8727853391ce1f38de8044cfd4678ed1f"}

[commit-bot: I haz the power, 2017-01-06 05:29:15]

Description was changed from

==========
Fixes calculation of abspos style properties:
left/padding/width/height...
Replaced NGSizeIndefinite with Optional<LayoutUnit>

Fixed a couple of minor bugs where AvailableSize was incorrect.

Passing ~30 more tests.

BUG=635619
==========

to

==========
Fixes calculation of abspos style properties:
left/padding/width/height...
Replaced NGSizeIndefinite with Optional<LayoutUnit>

Fixed a couple of minor bugs where AvailableSize was incorrect.

Passing ~30 more tests.

BUG=635619

Review-Url: https://codereview.chromium.org/2616613003
Cr-Commit-Position: refs/heads/master@{#441881}
Committed:
https://chromium.googlesource.com/chromium/src/+/49b2b8a8727853391ce1f38de804...
==========

[commit-bot: I haz the power, 2017-01-06 05:29:17]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/49b2b8a8727853391ce1f38de804...