Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in invocation of overloaded functions which have both the
all-LayoutUnit and all-int variations: the all-int variation is chosen
instead of the all-LayoutUnit one, which might not the intention of
the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf
Cr-Commit-Position: refs/heads/master@{#413578}

[Xianzhu, 2016-08-19 05:47:42]

Description was changed from

==========
Forbid implicit conversion from LayoutUnit to int/unsigned

BUG=638981
==========

to

==========
Forbid implicit conversion from LayoutUnit to int/unsigned

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-08-19 05:47:44]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 05:48:00]

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, 2016-08-19 05:55:44]

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

[commit-bot: I haz the power, 2016-08-19 05:55:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-08-21 00:28:55]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 00:28:58]

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, 2016-08-21 01:18:15]

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

[commit-bot: I haz the power, 2016-08-21 01:18:16]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Xianzhu, 2016-08-21 23:13:47]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 23:13:51]

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

[Xianzhu, 2016-08-22 00:21:40]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 00:21:54]

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, 2016-08-22 02:19:46]

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

[commit-bot: I haz the power, 2016-08-22 02:19:47]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-08-22 16:49:58]

Description was changed from

==========
Forbid implicit conversion from LayoutUnit to int/unsigned

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in overloaded functions which have both the all-LayoutUnit
and all-int variations: the all-int variation is chosen instead of the
all-LayoutUnit one, which is likely not the intention of the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-08-22 16:58:33]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 16:59:07]

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

[Xianzhu, 2016-08-22 17:00:51]

wangxianzhu@chromium.org changed reviewers:
+ eae@chromium.org, schenney@chromium.org

[Xianzhu, 2016-08-22 17:00:52]

Ptal:
eae@: Source/core/layout
schenney@: Other directories.

[Stephen Chennney, 2016-08-22 17:46:10]

LGTM for all of the non core/layout files. Where I could answer your "is this
intentional" questions I have. Before landing please remove the FIXMEs that I
had a definitive answer for.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:234: //
TODO(crbug.com/638981): Is the conversion to int intentional?
Yes, definitely intentional.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:243: //
TODO(crbug.com/638981): Is the conversion to int intentional?
Yes, definitely intentional.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:281: //
TODO(crbug.com/638981): Are the conversions to int (in LayoutRect(0, 0, ...))
intentional?
Yes, in this case I believe they are.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:56: //
TODO(crbug.com/638981): Is the first conversion to int intentional?
I suspect so. Tables are still laid out at int resolution, which leaves us with
all sorts of annoying bugs. Next gen layout will fix it, but for now I suspect
this is right.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/LayoutUnit.h (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/LayoutUnit.h:112: operator unsigned() const =
delete;
Am I right that these need to be deleted because otherwise there would be a
default cast on the m_value member?

[eae, 2016-08-22 17:58:54]

LGTM for core/layout

[mstensho (USE GERRIT), 2016-08-22 18:05:16]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-08-22 18:05:18]

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp
(right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp:592: count
= std::max(count, 1u);
What's this for? The line above already makes sure that it's at least
LayoutUnit(1), doesn't it?

[Xianzhu, 2016-08-22 18:14:38]

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:234: //
TODO(crbug.com/638981): Is the conversion to int intentional?
On 2016/08/22 17:46:10, Stephen Chennney wrote:
> Yes, definitely intentional.

Done.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:243: //
TODO(crbug.com/638981): Is the conversion to int intentional?
On 2016/08/22 17:46:10, Stephen Chennney wrote:
> Yes, definitely intentional.

Done.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:281: //
TODO(crbug.com/638981): Are the conversions to int (in LayoutRect(0, 0, ...))
intentional?
On 2016/08/22 17:46:10, Stephen Chennney wrote:
> Yes, in this case I believe they are.

Done.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:56: //
TODO(crbug.com/638981): Is the first conversion to int intentional?
On 2016/08/22 17:46:10, Stephen Chennney wrote:
> I suspect so. Tables are still laid out at int resolution, which leaves us
with
> all sorts of annoying bugs. Next gen layout will fix it, but for now I suspect
> this is right.

Done.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:61: //
TODO(crbug.com/638981): Is the conversion to int (in LayoutPoint(0, ...))
intentional?
Removed.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:67: //
TODO(crbug.com/638981): Is the conversion to int (in move(0, ...)) intentional?
Removed.

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/LayoutUnit.h (right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/LayoutUnit.h:112: operator unsigned() const =
delete;
On 2016/08/22 17:46:10, Stephen Chennney wrote:
> Am I right that these need to be deleted because otherwise there would be a
> default cast on the m_value member?

Even worse. To avoid implicit lossy conversion, we could add 'explicit' to these
operators, but the compiler would try to find other implicit conversion paths
and would either report ambiguous conversion error (because of multiple paths:
operator float() then int and operator bool() then int) or generate wrong code
if it chose the operator bool() then int path.

Added comment about the reason of 'delete'.

[Xianzhu, 2016-08-22 18:15:37]

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp
(right):

https://codereview.chromium.org/2261663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp:592: count
= std::max(count, 1u);
On 2016/08/22 18:05:17, mstensho wrote:
> What's this for? The line above already makes sure that it's at least
> LayoutUnit(1), doesn't it?

Thanks for the catch. This is garbage left from previous versions of the CL.
Removed.

[Xianzhu, 2016-08-22 18:33:43]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-22 18:33:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2261663002/#ps120001
(title: "-")

[commit-bot: I haz the power, 2016-08-22 18:34:44]

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

[Xianzhu, 2016-08-22 18:34:59]

Description was changed from

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in overloaded functions which have both the all-LayoutUnit
and all-int variations: the all-int variation is chosen instead of the
all-LayoutUnit one, which is likely not the intention of the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in overloaded functions which have both the all-LayoutUnit
and all-int variations: the all-int variation is chosen instead of the
all-LayoutUnit one, which might not the intention of the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-08-22 18:36:01]

Description was changed from

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in overloaded functions which have both the all-LayoutUnit
and all-int variations: the all-int variation is chosen instead of the
all-LayoutUnit one, which might not the intention of the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in invocation of overloaded functions which have both the
all-LayoutUnit and all-int variations: the all-int variation is chosen
instead of the all-LayoutUnit one, which might not the intention of
the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-08-22 21:55:22]

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

[commit-bot: I haz the power, 2016-08-22 21:55:24]

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

[Xianzhu, 2016-08-22 22:02:32]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-08-22 22:04:03]

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, 2016-08-22 23:28:38]

Description was changed from

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in invocation of overloaded functions which have both the
all-LayoutUnit and all-int variations: the all-int variation is chosen
instead of the all-LayoutUnit one, which might not the intention of
the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in invocation of overloaded functions which have both the
all-LayoutUnit and all-int variations: the all-int variation is chosen
instead of the all-LayoutUnit one, which might not the intention of
the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-08-22 23:28:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-22 23:30:40]

Description was changed from

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in invocation of overloaded functions which have both the
all-LayoutUnit and all-int variations: the all-int variation is chosen
instead of the all-LayoutUnit one, which might not the intention of
the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in invocation of overloaded functions which have both the
all-LayoutUnit and all-int variations: the all-int variation is chosen
instead of the all-LayoutUnit one, which might not the intention of
the author.

BUG=638981
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf
Cr-Commit-Position: refs/heads/master@{#413578}
==========

[commit-bot: I haz the power, 2016-08-22 23:30:42]

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