Constrain width/height to minmax values

Constrain width/height to minmax values

Width and height were not being constrained to min/max values.

Adds a ConstrainByMinMax to ng_lenght_utils

This makes another ~10 tests pass.

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

[atotic, 2017-01-19 19:25:14]

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

[atotic, 2017-01-19 19:25:42]

ptal

[cbiesinger, 2017-01-19 22:49:13]

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

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:34:
LengthResolveType::kContentSize);
So, you really need to pass in the LengthResolveType so that you can handle
kContentSize vs kMinSize vs kMaxSize correctly.

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:195: LayoutUnit
ResolveHeight(const Length& height,
I would move this function to the top, under ResolveWidth

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_length_utils.h (right):

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_length_utils.h:124: CORE_EXPORT
LayoutUnit ConstrainByMinMax(LayoutUnit length,
Now that you added this helper, can you make use of it in the other functions in
ng_length_utils.cc as well?

[atotic, 2017-01-19 23:47:44]

ptal, all comments addressed.

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

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:34:
LengthResolveType::kContentSize);
On 2017/01/19 at 22:49:13, cbiesinger wrote:
> So, you really need to pass in the LengthResolveType so that you can handle
kContentSize vs kMinSize vs kMaxSize correctly.

done. The usage of Resolve.. methods keeps tripping me.

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:195: LayoutUnit
ResolveHeight(const Length& height,
On 2017/01/19 at 22:49:13, cbiesinger wrote:
> I would move this function to the top, under ResolveWidth

done.

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_length_utils.h (right):

https://codereview.chromium.org/2643043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_length_utils.h:124: CORE_EXPORT
LayoutUnit ConstrainByMinMax(LayoutUnit length,
On 2017/01/19 at 22:49:13, cbiesinger wrote:
> Now that you added this helper, can you make use of it in the other functions
in ng_length_utils.cc as well?

done. Found a couple. 

I still dislike lots of template code around ConstrainByMinMax, feels fragile.
Would love to encapsulate it more.

What do you think about creating a LengthResolver object? It'd wrap space and
style, and have bunch of resolution methods: margin, padding, width, height.

[cbiesinger, 2017-01-20 21:15:14]

I wonder if it would simplify things if we did not use optional and instead
resolved MaxSizeNone to LayoutUnit::max()... (min-size doesn't really need
Optional because auto/0 is just 0)

Anyway that may be a longer discussion, this patch lgtm. Thanks!

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

https://codereview.chromium.org/2643043003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:32: const
LengthResolveType& resolve_type) {
That's an enum, it really does not need to be passed by reference :)

[atotic, 2017-01-20 21:22:38]

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

https://codereview.chromium.org/2643043003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:32: const
LengthResolveType& resolve_type) {
On 2017/01/20 at 21:15:14, cbiesinger wrote:
> That's an enum, it really does not need to be passed by reference :)

done. I was just being consistent, but if it is slower...

[atotic, 2017-01-20 21:22:39]

The CQ bit was checked by atotic@chromium.org

[atotic, 2017-01-20 21:22:40]

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/2643043003/#ps60001
(title: "Clamp width/height to minmax values")

[commit-bot: I haz the power, 2017-01-20 21:23:13]

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-20 23:11:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484947359735700,
"parent_rev": "6229de84ea74dc067cce0a324211d0d2d4874716", "commit_rev":
"b5a30a4313c916119d0b137d3c2e0c53a2021a3b"}

[commit-bot: I haz the power, 2017-01-20 23:12:19]

Description was changed from

==========
Constrain width/height to minmax values

Width and height were not being constrained to min/max values.

Adds a ConstrainByMinMax to ng_lenght_utils

This makes another ~10 tests pass.

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

to

==========
Constrain width/height to minmax values

Width and height were not being constrained to min/max values.

Adds a ConstrainByMinMax to ng_lenght_utils

This makes another ~10 tests pass.

BUG=635619

Review-Url: https://codereview.chromium.org/2643043003
Cr-Commit-Position: refs/heads/master@{#445201}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5a30a4313c916119d0b137d3c2e...
==========

[commit-bot: I haz the power, 2017-01-20 23:12:20]

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