[ng_layout] Fix wrong percent_resolution_size

Fix wrong percent_resolution_size inside ConstraintSpace::CreateFromLayoutObject.
This caused RunOldLayout to use wrong PercentageResolutionSize

<main style="width:500px;height:150px;position:relative;border: solid black">
  <div style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div>
</main>

This used to display at 1/2 width in NGLayout.

BUG=635619
[ng_constraint_width]

Review-Url: https://codereview.chromium.org/2616093004
Cr-Commit-Position: refs/heads/master@{#442730}
Committed: https://chromium.googlesource.com/chromium/src/+/67e0a1510f8a824b0e07c61484bc55c28c2f6c86

[atotic, 2017-01-07 00:49:57]

Description was changed from

==========
Fix wrong percentage on RunOldLayout

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

to

==========
Fix wrong percent_resolution_size inside
ConstraintSpace::CreateFromLayoutObject.
This caused RunOldLayout to use wrong PercentageResolutionSize

<main style="width:500px;height:150px;position:relative;border: solid black">
  <div
style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div>
</main>

This used to display at 1/2 width in NGLayout.

BUG=635619

[ng_constraint_width]
==========

[atotic, 2017-01-07 00:49:57]

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

[atotic, 2017-01-07 00:50:11]

ptal

[cbiesinger, 2017-01-09 18:17:10]

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

https://codereview.chromium.org/2616093004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:75:
box.containingBlockLogicalHeightForContent(IncludeMarginBorderPadding)};
OK, a few questions:
- Here you explicitly choose containingBlockLogicalHeightForContent instead of
availableLogicalHeightForPercentageComputation for percentage resolution. Why?
The latter is supposed to be the correct one and if it isn't we should fix that
- Why is the available space supposed to be different from the percentage size
here?
- If this is in fact correct you should use ExcludeMarginBorderPadding right?

I assume this is for the !fixed_block case.

[atotic, 2017-01-09 23:22:13]

ptal

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

https://codereview.chromium.org/2616093004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:75:
box.containingBlockLogicalHeightForContent(IncludeMarginBorderPadding)};
On 2017/01/09 at 18:17:10, cbiesinger wrote:
> OK, a few questions:
> - Here you explicitly choose containingBlockLogicalHeightForContent instead of
availableLogicalHeightForPercentageComputation for percentage resolution. Why?
The latter is supposed to be the correct one and if it isn't we should fix that

You are right. The real problem was that available_sizes were overridden by 
hasOverrideLogicalContentWidth afterwards.

New fix just saves percentage_size before we override available_size with
OverrideLogicalContentSize.

[cbiesinger, 2017-01-10 19:45:03]

lgtm but see comments and nit below.

I am not completely sure why this fixes your bug though. The width calculation
for the block should just take the fixed inline size and never do actual
percentage calculation:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng...

Can you investigate a bit more why this is necessary?

(This is OK to submit anyway, because it will fix other bugs -- things like
percentage padding calculations would be wrong)

https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right):

https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:56:
available_logical_height};
While you're touching this, I'd also make this other change:

  NGLogicalSize available_size = percentage_size;
then in the ifs change available_size.{inline,block}_size. I think that would be
clear. It would also rename the ambiguously-named "size" variable, which is
another plus :)

[atotic, 2017-01-10 21:52:48]

On 2017/01/10 at 19:45:03, cbiesinger wrote:
> lgtm but see comments and nit below.
> 
> I am not completely sure why this fixes your bug though. The width calculation
for the block should just take the fixed inline size and never do actual
percentage calculation:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng...
> 
> Can you investigate a bit more why this is necessary?
> 
> (This is OK to submit anyway, because it will fix other bugs -- things like
percentage padding calculations would be wrong)

This is because NGBlockNode is RunOldLayout, and old code does take container
size into 
account. Still investingating how this happens, there are other bugs.

[atotic, 2017-01-10 21:53:20]

https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right):

https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:56:
available_logical_height};
On 2017/01/10 at 19:45:03, cbiesinger wrote:
> While you're touching this, I'd also make this other change:
> 
>   NGLogicalSize available_size = percentage_size;
> then in the ifs change available_size.{inline,block}_size. I think that would
be clear. It would also rename the ambiguously-named "size" variable, which is
another plus :)

done

[atotic, 2017-01-10 21:53:21]

The CQ bit was checked by atotic@chromium.org

[atotic, 2017-01-10 21:53:22]

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/2616093004/#ps40001
(title: "size->logical_size")

[commit-bot: I haz the power, 2017-01-10 21:54:09]

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

[cbiesinger, 2017-01-10 22:00:50]

https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right):

https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:74:
NGLogicalSize logical_size = {available_logical_width,
available_size is better. But I also meant if you could move this line above,
right under percentage_size.


Ah... I think I figured it out wrt the old codepath... two reasons:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
gets called before we check for overrideLogicalContentWidth. But also:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
We ignore the override width unless we're a flex item. Should probably change
that.

[atotic, 2017-01-10 23:24:15]

just replying to a comment

https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right):

https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:74:
NGLogicalSize logical_size = {available_logical_width,
On 2017/01/10 at 22:00:50, cbiesinger wrote:
> available_size is better. But I also meant if you could move this line above,
right under percentage_size.

logical_size can't go right under percentage_size, because 
available_logical_height gets recomputed in between those two statements.

> Ah... I think I figured it out wrt the old codepath... two reasons:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> gets called before we check for overrideLogicalContentWidth. But also:
> We ignore the override width unless we're a flex item. Should probably change
that.

I am shy when it comes to editing old code, unless I am explicitly fixing
an existing bug. My knowledge of existing code paths is limitied,
and I fear unexpected dependencies.

Do you want to do old code work, or should I just follow your advice?

[cbiesinger, 2017-01-10 23:26:37]

https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right):

https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:74:
NGLogicalSize logical_size = {available_logical_width,
On 2017/01/10 23:24:15, atotic wrote:
> On 2017/01/10 at 22:00:50, cbiesinger wrote:
> > available_size is better. But I also meant if you could move this line
above,
> right under percentage_size.
> 
> logical_size can't go right under percentage_size, because 
> available_logical_height gets recomputed in between those two statements.

I'll send a followup patch once this lands, I think that'll help in explaining
what I mean and then we can discuss whether it's a good idea :-)

> > Ah... I think I figured it out wrt the old codepath... two reasons:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> > gets called before we check for overrideLogicalContentWidth. But also:
> > We ignore the override width unless we're a flex item. Should probably
change
> that.
> 
> I am shy when it comes to editing old code, unless I am explicitly fixing
> an existing bug. My knowledge of existing code paths is limitied,
> and I fear unexpected dependencies.
> 
> Do you want to do old code work, or should I just follow your advice?

I can do that tomorrow.

[commit-bot: I haz the power, 2017-01-10 23:39:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484085201655740,
"parent_rev": "9422f34b57066a40cea5b40c1e022adeb4712859", "commit_rev":
"67e0a1510f8a824b0e07c61484bc55c28c2f6c86"}

[commit-bot: I haz the power, 2017-01-10 23:39:59]

Description was changed from

==========
Fix wrong percent_resolution_size inside
ConstraintSpace::CreateFromLayoutObject.
This caused RunOldLayout to use wrong PercentageResolutionSize

<main style="width:500px;height:150px;position:relative;border: solid black">
  <div
style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div>
</main>

This used to display at 1/2 width in NGLayout.

BUG=635619

[ng_constraint_width]
==========

to

==========
Fix wrong percent_resolution_size inside
ConstraintSpace::CreateFromLayoutObject.
This caused RunOldLayout to use wrong PercentageResolutionSize

<main style="width:500px;height:150px;position:relative;border: solid black">
  <div
style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div>
</main>

This used to display at 1/2 width in NGLayout.

BUG=635619

[ng_constraint_width]

Review-Url: https://codereview.chromium.org/2616093004
Cr-Commit-Position: refs/heads/master@{#442730}
Committed:
https://chromium.googlesource.com/chromium/src/+/67e0a1510f8a824b0e07c61484bc...
==========

[commit-bot: I haz the power, 2017-01-10 23:40:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/67e0a1510f8a824b0e07c61484bc...