Avoid negative content box sizes.

Avoid negative content box sizes.

Negative values had two possible causes:

1. Subtracting the scrollbar size from the border box size. Scrollbars do not
affect the border box size, but they occupy space between some border edge and
padding edge. This means that the presence of a scrollbar on an object reduces
the size of the content box and the containing block established by said
object. This means that if e.g. the specified width of an object is 10px and it
has a vertical scrollbar that takes up more than that, e.g. 15px, the content
box width should become 0, not -5px.

2. Subtracting two huge (or even saturated) LayoutUnit values from one
LayoutUnit value. When we during layout convert a specified content-box width
to border-box (via padding-box), which is what LayoutBox::m_frameRect uses, we
may end up with saturated LayoutUnit values, so that:
LayoutUnit specified_content_box_width = <whatever, something nice, maybe>;
LayoutUnit left_padding = LayoutUnit::max() /*(or something huge, at least)*/;
LayoutUnit right_padding = LayoutUnit::max() /*(or something huge, at least);*/
LayoutUnit padding_box_width = content_box_width + left_padding + right_padding;
LayoutUnit content_box_width = padding_box_width - left_padding - right_padding;
Here, content_box_width won't be the same as specified_content_box_width,
because padding_box_width got saturated. That's kind of inevitable, with
saturated arithmetic and all, but what's worse is that we used to end up with a
negative value in content_box_width, which is illegal. So just clamp negative
values to 0 to avoid that.

Negative box sizes have various kinds of ill effects, such as inline-axis
misalignment and unwanted negative block direction progression. It was possible
to get negative padding (which is illegal) resolved from percentage values.
This in turn caused unnecessary assertion failures in multicol.

Attempted to come up with sensible layout tests that don't make assumptions
about how the engine deals with extreme values internally.

BUG=683554, 683090
Review-Url: https://codereview.chromium.org/2716583002
Cr-Commit-Position: refs/heads/master@{#452578}
Committed: https://chromium.googlesource.com/chromium/src/+/47305a33360149240ab99c57c6c65ff5f05fa68e

[mstensho (USE GERRIT), 2017-02-23 11:56:30]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 11:56:45]

Dry run: 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-02-23 13:18:03]

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

[commit-bot: I haz the power, 2017-02-23 13:18:04]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2017-02-23 13:35:09]

mstensho@opera.com changed reviewers:
+ eae@chromium.org

[mstensho (USE GERRIT), 2017-02-23 13:35:10]

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html (right):

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html:4: <div
style="position:absolute; left:100%; border-width:1234567890px;
border-style:solid;">
This test is valid, but it would also pass prior to this CL, but not prior to
https://codereview.chromium.org/2640143005 (support for sub-pixel borders),
which landed while I was working on this CL. The test is still worth keeping,
I'd say.

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:513: // of the two border
sides may be larger than the border box size stored in
This was true when I started working on this patch, but it's no longer the case.
With https://codereview.chromium.org/2640143005 border sides are now limited to
2^20px , while LayoutUnit values are limited to +/- 2^25px, so subtracting
borders should no longer result in LayoutUnit saturation.

However, I still believe we should keep my change as it is, because:

1: We need to guard against the scrollbar being wider than the padding box
anyway.
2: Some time in the future, we might start to allow larger border values than
today

[eae, 2017-02-23 15:54:37]

LGTM w/minor suggestions

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html (right):

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html:4: <div
style="position:absolute; left:100%; border-width:1234567890px;
border-style:solid;">
On 2017/02/23 13:35:09, mstensho wrote:
> This test is valid, but it would also pass prior to this CL, but not prior to
> https://codereview.chromium.org/2640143005 (support for sub-pixel borders),
> which landed while I was working on this CL. The test is still worth keeping,
> I'd say.

Agreed!

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:513: // of the two border
sides may be larger than the border box size stored in
On 2017/02/23 13:35:09, mstensho wrote:
> This was true when I started working on this patch, but it's no longer the
case.
> With https://codereview.chromium.org/2640143005 border sides are now limited
to
> 2^20px , while LayoutUnit values are limited to +/- 2^25px, so subtracting
> borders should no longer result in LayoutUnit saturation.
> 
> However, I still believe we should keep my change as it is, because:
> 
> 1: We need to guard against the scrollbar being wider than the padding box
> anyway.
> 2: Some time in the future, we might start to allow larger border values than
> today

Fully agree that we should keep the check. Please update the comment to reflect
the current state of the world though.

Thanks for staying on top of things!

[mstensho (USE GERRIT), 2017-02-23 17:42:09]

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:513: // of the two border
sides may be larger than the border box size stored in
On 2017/02/23 15:54:37, eae wrote:
> On 2017/02/23 13:35:09, mstensho wrote:
> > This was true when I started working on this patch, but it's no longer the
> case.
> > With https://codereview.chromium.org/2640143005 border sides are now limited
> to
> > 2^20px , while LayoutUnit values are limited to +/- 2^25px, so subtracting
> > borders should no longer result in LayoutUnit saturation.
> > 
> > However, I still believe we should keep my change as it is, because:
> > 
> > 1: We need to guard against the scrollbar being wider than the padding box
> > anyway.
> > 2: Some time in the future, we might start to allow larger border values
than
> > today
> 
> Fully agree that we should keep the check. Please update the comment to
reflect
> the current state of the world though.

Good idea! Done.

[mstensho (USE GERRIT), 2017-02-23 17:42:13]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2017-02-23 17:42:14]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2716583002/#ps20001
(title: "code review")

[commit-bot: I haz the power, 2017-02-23 17:42:54]

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-02-23 19:19:19]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487871733886080,
"parent_rev": "5cb64f9cf9fbe97e44fdedaf319308768424f62e", "commit_rev":
"47305a33360149240ab99c57c6c65ff5f05fa68e"}

[commit-bot: I haz the power, 2017-02-23 19:19:53]

Description was changed from

==========
Avoid negative content box sizes.

Negative values had two possible causes:

1. Subtracting the scrollbar size from the border box size. Scrollbars do not
affect the border box size, but they occupy space between some border edge and
padding edge. This means that the presence of a scrollbar on an object reduces
the size of the content box and the containing block established by said
object. This means that if e.g. the specified width of an object is 10px and it
has a vertical scrollbar that takes up more than that, e.g. 15px, the content
box width should become 0, not -5px.

2. Subtracting two huge (or even saturated) LayoutUnit values from one
LayoutUnit value. When we during layout convert a specified content-box width
to border-box (via padding-box), which is what LayoutBox::m_frameRect uses, we
may end up with saturated LayoutUnit values, so that:
LayoutUnit specified_content_box_width = <whatever, something nice, maybe>;
LayoutUnit left_padding = LayoutUnit::max() /*(or something huge, at least)*/;
LayoutUnit right_padding = LayoutUnit::max() /*(or something huge, at least);*/
LayoutUnit padding_box_width = content_box_width + left_padding + right_padding;
LayoutUnit content_box_width = padding_box_width - left_padding - right_padding;
Here, content_box_width won't be the same as specified_content_box_width,
because padding_box_width got saturated. That's kind of inevitable, with
saturated arithmetic and all, but what's worse is that we used to end up with a
negative value in content_box_width, which is illegal. So just clamp negative
values to 0 to avoid that.

Negative box sizes have various kinds of ill effects, such as inline-axis
misalignment and unwanted negative block direction progression. It was possible
to get negative padding (which is illegal) resolved from percentage values.
This in turn caused unnecessary assertion failures in multicol.

Attempted to come up with sensible layout tests that don't make assumptions
about how the engine deals with extreme values internally.

BUG=683554,683090
==========

to

==========
Avoid negative content box sizes.

Negative values had two possible causes:

1. Subtracting the scrollbar size from the border box size. Scrollbars do not
affect the border box size, but they occupy space between some border edge and
padding edge. This means that the presence of a scrollbar on an object reduces
the size of the content box and the containing block established by said
object. This means that if e.g. the specified width of an object is 10px and it
has a vertical scrollbar that takes up more than that, e.g. 15px, the content
box width should become 0, not -5px.

2. Subtracting two huge (or even saturated) LayoutUnit values from one
LayoutUnit value. When we during layout convert a specified content-box width
to border-box (via padding-box), which is what LayoutBox::m_frameRect uses, we
may end up with saturated LayoutUnit values, so that:
LayoutUnit specified_content_box_width = <whatever, something nice, maybe>;
LayoutUnit left_padding = LayoutUnit::max() /*(or something huge, at least)*/;
LayoutUnit right_padding = LayoutUnit::max() /*(or something huge, at least);*/
LayoutUnit padding_box_width = content_box_width + left_padding + right_padding;
LayoutUnit content_box_width = padding_box_width - left_padding - right_padding;
Here, content_box_width won't be the same as specified_content_box_width,
because padding_box_width got saturated. That's kind of inevitable, with
saturated arithmetic and all, but what's worse is that we used to end up with a
negative value in content_box_width, which is illegal. So just clamp negative
values to 0 to avoid that.

Negative box sizes have various kinds of ill effects, such as inline-axis
misalignment and unwanted negative block direction progression. It was possible
to get negative padding (which is illegal) resolved from percentage values.
This in turn caused unnecessary assertion failures in multicol.

Attempted to come up with sensible layout tests that don't make assumptions
about how the engine deals with extreme values internally.

BUG=683554,683090

Review-Url: https://codereview.chromium.org/2716583002
Cr-Commit-Position: refs/heads/master@{#452578}
Committed:
https://chromium.googlesource.com/chromium/src/+/47305a33360149240ab99c57c6c6...
==========

[commit-bot: I haz the power, 2017-02-23 19:19:54]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/47305a33360149240ab99c57c6c6...