More explicit LayoutUnit conversions

More explicit LayoutUnit conversions

Smokey the engineer says: Only You can prevent implicit conversion.

BUG=581254
TBR=dmazzoni

Committed: https://crrev.com/64c0da8cdaf8c7a857051640fa453ce6a7ec5a6f
Cr-Commit-Position: refs/heads/master@{#372565}

[leviw_travelin_and_unemployed, 2016-01-30 23:50:13]

Description was changed from

==========
More patches toward explicit LayoutUnit constructors

BUG=581254
==========

to

==========
More explicit LayoutUnit conversions

Smokey the engineer says: say no to implicit LayoutUnit
conversion.

BUG=581254
==========

[leviw_travelin_and_unemployed, 2016-01-30 23:50:13]

leviw@chromium.org changed reviewers:
+ dmazzoni@chromium.org, eae@chromium.org

[leviw_travelin_and_unemployed, 2016-01-30 23:50:58]

LayoutUnit(review);

[eae, 2016-01-30 23:58:24]

LGTM w/nits

Only you can prevent implicit conversions.

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:354: bool
logicalHeightWasIndefinite = computeContentLogicalHeight(MainOrPreferredSize,
style()->logicalHeight(), LayoutUnit(-1)) == -1;
Shouldn't this also be a LayoutUnit (to the right of the operator)?

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:413: return
valueForLength(trackGap, LayoutUnit(0)) * (span - 1);
LayoutUnit()

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:436:
sizingData.freeSpaceForDirection(ForRows) = LayoutUnit(0);
LayoutUnit()

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:289:
setLogicalWidth(LayoutUnit(std::min(availableContentLogicalWidth,
maxPreferredLogicalWidth()).toInt()));
Can we do floor() instead of toInt() or would that change the handling of
negative numbers?

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:733: m_rowPos[r +
1] = std::max(m_rowPos[r] + minimumValueForLength(m_grid[r].logicalHeight,
LayoutUnit(0)).round(), 0);
LayoutUnit()

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/SpatialNavigation.cpp (right):

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/SpatialNavigation.cpp:235: // TODO(leviw):
Why are these values truncated (toInt) instead of rounding?
These should probably be snapped based on the container.

[leviw_travelin_and_unemployed, 2016-01-31 00:33:13]

Thanks for the review, Ranger Emil!

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:413: return
valueForLength(trackGap, LayoutUnit(0)) * (span - 1);
On 2016/01/30 at 23:58:24, eae wrote:
> LayoutUnit()

D'oh :p

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:289:
setLogicalWidth(LayoutUnit(std::min(availableContentLogicalWidth,
maxPreferredLogicalWidth()).toInt()));
On 2016/01/30 at 23:58:24, eae wrote:
> Can we do floor() instead of toInt() or would that change the handling of
negative numbers?

It would, but here I don't think we care here :) Tryin' it!

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/SpatialNavigation.cpp (right):

https://codereview.chromium.org/1651703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/SpatialNavigation.cpp:235: // TODO(leviw):
Why are these values truncated (toInt) instead of rounding?
On 2016/01/30 at 23:58:24, eae wrote:
> These should probably be snapped based on the container.

Yup! I'm avoiding for this round to keep from conflating these changes with
actual behavioral changes.

[leviw_travelin_and_unemployed, 2016-01-31 00:33:52]

Description was changed from

==========
More explicit LayoutUnit conversions

Smokey the engineer says: say no to implicit LayoutUnit
conversion.

BUG=581254
==========

to

==========
More explicit LayoutUnit conversions

Smokey the engineer says: Only You can prevent implicit conversion.

BUG=581254
==========

[leviw_travelin_and_unemployed, 2016-01-31 00:34:00]

(I stole your description)

[eae, 2016-01-31 00:44:24]

r- for unclear patchset description, shouldn't it be PropertyBag instead of
Traits?

LGTM

[leviw_travelin_and_unemployed, 2016-01-31 01:13:32]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2016-01-31 01:13:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1651703002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1651703002/40001

[commit-bot: I haz the power, 2016-01-31 01:21:59]

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

[commit-bot: I haz the power, 2016-01-31 01:22:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[leviw_travelin_and_unemployed, 2016-01-31 01:35:20]

Description was changed from

==========
More explicit LayoutUnit conversions

Smokey the engineer says: Only You can prevent implicit conversion.

BUG=581254
==========

to

==========
More explicit LayoutUnit conversions

Smokey the engineer says: Only You can prevent implicit conversion.

BUG=581254
TBR=dmazzoni
==========

[leviw_travelin_and_unemployed, 2016-01-31 01:35:49]

TBR-ing the simple AX changes.

[leviw_travelin_and_unemployed, 2016-01-31 01:35:51]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2016-01-31 01:36:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1651703002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1651703002/40001

[commit-bot: I haz the power, 2016-01-31 01:41:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-31 01:42:09]

Description was changed from

==========
More explicit LayoutUnit conversions

Smokey the engineer says: Only You can prevent implicit conversion.

BUG=581254
TBR=dmazzoni
==========

to

==========
More explicit LayoutUnit conversions

Smokey the engineer says: Only You can prevent implicit conversion.

BUG=581254
TBR=dmazzoni

Committed: https://crrev.com/64c0da8cdaf8c7a857051640fa453ce6a7ec5a6f
Cr-Commit-Position: refs/heads/master@{#372565}
==========

[commit-bot: I haz the power, 2016-01-31 01:42:11]

Patchset 3 (id:??) landed as
https://crrev.com/64c0da8cdaf8c7a857051640fa453ce6a7ec5a6f
Cr-Commit-Position: refs/heads/master@{#372565}