Transition to explicit constructors in LayoutUnit

Transition to explicit constructors in LayoutUnit

This is a work in progress, but because there are implicit conversions
of base types, and converting the world en masse would be a seriously
large patch, this converts a handful of files toward the goal of
switching all of LayoutUnit's non-default constructors to be explicit.

BUG=581254
Committed: https://crrev.com/db620902e31edb691f210eb7a50799ddfe6eb538
Cr-Commit-Position: refs/heads/master@{#372520}

[leviw_travelin_and_unemployed, 2016-01-28 07:52:23]

Description was changed from

==========
WIP Explicit constructors

BUG=
==========

to

==========
Transition to explicit constructors in LayoutUnit

This is a work in progress, but because there are implicit conversions
of base types, and converting the world en masse would be a seriously
large patch, this converts a handful of files toward the goal of
switching all of LayoutUnit's non-default constructors to be explicit.

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

[leviw_travelin_and_unemployed, 2016-01-29 03:24:01]

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

[leviw_travelin_and_unemployed, 2016-01-29 03:24:01]

PTAL

[eae, 2016-01-29 03:38:12]

Fun times

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1277: return
LayoutPoint(LayoutUnit(lineDirectionPoint - absoluteBlockPoint.x()),
root->blockDirectionPointInLine());
lineDirectionPoint - LayoutUnit(absoluteBlockPoint.x())

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1279: return
LayoutPoint(root->blockDirectionPointInLine(), LayoutUnit(lineDirectionPoint -
absoluteBlockPoint.y()));
...and here:

return LayoutPoint(root->blockDirectionPointInLine(), lineDirectionPoint -
LayoutUnit(absoluteBlockPoint.y()));

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.h (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.h:470: return
std::max<LayoutUnit>(block->marginBefore(), LayoutUnit());
No need for template specification if you specify the type for the second
argument.

std::max(block->marginBefore(), LayoutUnit());

Given how common this is we might be better off with a helper function here.

Something like ensurePositive or clampToZero. Could be a static function or a
method on LayoutUnit.

block->marginBefore().clampToZero();

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:916: x =
std::min(x, std::max<LayoutUnit>(maxX - caretWidth(), LayoutUnit()));
Remove template specification or use clampToZero if we add it.

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:221:
LayoutRectOutsets borderInsets() const { return
LayoutRectOutsets(LayoutUnit(-borderTop()), LayoutUnit(-borderRight()),
LayoutUnit(-borderBottom()), LayoutUnit(-borderLeft())); }
How about adding a LayoutRectOutset constructor that takes four ints?

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp
(right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp:661:
childX += child->marginLeft() + std::max<LayoutUnit>(LayoutUnit(),
(contentWidth() - (child->size().width() + child->marginWidth())) / 2);
Add LayoutUnit() around the non-LayoutUnit value instead of around all to reduce
number of conversions. Also remove template specialization.

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp
(right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp:490: width
= std::max<LayoutUnit>(0, (availableWidth - ((count - 1) * columnGap)) / count);
LayoutUnit()

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp:492: count
= std::max<LayoutUnit>(1, (availableWidth + columnGap) / (computedColumnWidth +
columnGap));
LayoutUnit(1)

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp:495: count
= std::max<LayoutUnit>(std::min<LayoutUnit>(LayoutUnit(computedColumnCount),
(availableWidth + columnGap) / (computedColumnWidth + columnGap)), 1);
LayoutUnit(computedColumnCount)

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.h (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.h:145: return
LayoutUnit();
return 0; (this method returns an int)

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.h:151: return
LayoutUnit();
return 0; (this method returns an int)

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.h:157: return
LayoutUnit();
return 0; (this method returns an int)

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.h:163: return
LayoutUnit();
return 0; (this method returns an int)

[leviw_travelin_and_unemployed, 2016-01-29 03:47:28]

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1277: return
LayoutPoint(LayoutUnit(lineDirectionPoint - absoluteBlockPoint.x()),
root->blockDirectionPointInLine());
On 2016/01/29 at 03:38:11, eae wrote:
> lineDirectionPoint - LayoutUnit(absoluteBlockPoint.x())

This is actually different behavior, and likely changes LayoutTests.

Currently, for float/double operators we convert LayoutUnit to float/double and
do the math on the FPU. I can try it on the bots if you like.

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1279: return
LayoutPoint(root->blockDirectionPointInLine(), LayoutUnit(lineDirectionPoint -
absoluteBlockPoint.y()));
On 2016/01/29 at 03:38:11, eae wrote:
> ...and here:
> 
> return LayoutPoint(root->blockDirectionPointInLine(), lineDirectionPoint -
LayoutUnit(absoluteBlockPoint.y()));

Ditto.

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.h (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.h:470: return
std::max<LayoutUnit>(block->marginBefore(), LayoutUnit());
On 2016/01/29 at 03:38:11, eae wrote:
> No need for template specification if you specify the type for the second
argument.
> 
> std::max(block->marginBefore(), LayoutUnit());
> 
> Given how common this is we might be better off with a helper function here.
> 
> Something like ensurePositive or clampToZero. Could be a static function or a
method on LayoutUnit.
> 
> block->marginBefore().clampToZero();

Makes sense.

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1648573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:221:
LayoutRectOutsets borderInsets() const { return
LayoutRectOutsets(LayoutUnit(-borderTop()), LayoutUnit(-borderRight()),
LayoutUnit(-borderBottom()), LayoutUnit(-borderLeft())); }
On 2016/01/29 at 03:38:12, eae wrote:
> How about adding a LayoutRectOutset constructor that takes four ints?

Sure.

[leviw_travelin_and_unemployed, 2016-01-29 04:26:32]

PTAL

[eae, 2016-01-29 05:48:02]

LGTM

[eae, 2016-01-29 05:48:10]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-01-29 05:48:19]

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

[commit-bot: I haz the power, 2016-01-29 06:33:34]

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

[commit-bot: I haz the power, 2016-01-29 06:33:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[leviw_travelin_and_unemployed, 2016-01-29 23:31:40]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2016-01-29 23:31:40]

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/1648573002/#ps120001
(title: "Fix negative sign binding in negativeMarginBefore/After")

[eae, 2016-01-29 23:33:28]

LGTM

[commit-bot: I haz the power, 2016-01-29 23:34:27]

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

[commit-bot: I haz the power, 2016-01-30 00:48:35]

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

[commit-bot: I haz the power, 2016-01-30 00:48:36]

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

[leviw_travelin_and_unemployed, 2016-01-30 01:35:43]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2016-01-30 01:35:44]

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/1648573002/#ps140001
(title: "Remove int constructors for size/point... they result in incorrect
conversions until LayoutUnit is …")

[commit-bot: I haz the power, 2016-01-30 01:36:12]

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

[commit-bot: I haz the power, 2016-01-30 02:39:09]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-30 02:40:08]

Description was changed from

==========
Transition to explicit constructors in LayoutUnit

This is a work in progress, but because there are implicit conversions
of base types, and converting the world en masse would be a seriously
large patch, this converts a handful of files toward the goal of
switching all of LayoutUnit's non-default constructors to be explicit.

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

to

==========
Transition to explicit constructors in LayoutUnit

This is a work in progress, but because there are implicit conversions
of base types, and converting the world en masse would be a seriously
large patch, this converts a handful of files toward the goal of
switching all of LayoutUnit's non-default constructors to be explicit.

BUG=581254

Committed: https://crrev.com/db620902e31edb691f210eb7a50799ddfe6eb538
Cr-Commit-Position: refs/heads/master@{#372520}
==========

[commit-bot: I haz the power, 2016-01-30 02:40:09]

Patchset 8 (id:??) landed as
https://crrev.com/db620902e31edb691f210eb7a50799ddfe6eb538
Cr-Commit-Position: refs/heads/master@{#372520}