Making LayoutUnit conversions to Float type explicit

Making LayoutUnit conversions to Float type explicit
LayoutUnit misuse, often from improper conversion to other types,is a 
constant source of bugs in Blink. This is the second patch with the goal 
of making such error-prone conversions explicit in the hopes of forcing 
developers to think through these conversions while writing their patches.
These conversion sites were discovered by toggling the float conversions
to explicit and finding the compiler errors.

BUG=350474
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169545

[gnana, 2014-03-14 08:19:00]

Please have a look.

[gnana, 2014-03-14 13:22:36]

On 2014/03/14 08:19:00, gnana wrote:
> Please have a look.

Patch is not ready yet.

[gnana, 2014-03-14 14:10:05]

On 2014/03/14 13:22:36, gnana wrote:
> On 2014/03/14 08:19:00, gnana wrote:
> > Please have a look.
> 
> Patch is not ready yet.

Layouttest and unit test are passed.
Now patch is ready.

[leviw_travelin_and_unemployed, 2014-03-14 18:25:25]

https://codereview.chromium.org/200023002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5110:
quads[i].move(-FloatSize(visibleContentRect.x().toFloat(),
visibleContentRect.y().toFloat()));
It'd be nice to use visibleContentRect.location() instead of having these as
components.

https://codereview.chromium.org/200023002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5121:
rect.move(-FloatSize(visibleContentRect.x().toFloat(),
visibleContentRect.y().toFloat()));
Ditto.

https://codereview.chromium.org/200023002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2868: int expectedPageWidth =
std::min<float>(documentRect.width().toFloat(), pageSize.width() *
maximumShrinkFactor);
It seems wrong not to call .toInt() on the LayoutUnit here since we're
ultimately assigning it to an int.

It'd also be better to have an IntSize for the expected page size instead of
separated components.

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
File Source/core/rendering/shapes/BoxShape.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/BoxShape.cpp:80: float y2 = logicalTop.toFloat() +
logicalHeight.toFloat();
Add first, then toFloat() the result. This uses integer math for the addition,
and only converts to a float once.

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/BoxShape.cpp:122: float y2 = logicalTop.toFloat() +
logicalHeight.toFloat();
ditto

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
File Source/core/rendering/shapes/PolygonShape.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/PolygonShape.cpp:372: float y2 =
logicalTop.toFloat() + logicalHeight.toFloat();
Again, convert the result of the addition.

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/PolygonShape.cpp:400: float y2 =
logicalTop.toFloat() + logicalHeight.toFloat();
Ditto.

[gnana, 2014-03-18 14:20:17]

Please have a look.

https://codereview.chromium.org/200023002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5110:
quads[i].move(-FloatSize(visibleContentRect.x().toFloat(),
visibleContentRect.y().toFloat()));
On 2014/03/14 18:25:26, Levi wrote:
> It'd be nice to use visibleContentRect.location() instead of having these as
> components.

We have below canditates for move
FloatQuad::Move(FloatSize)
FloatQuad::Move(float,float)

if we use visibleContentRect.location() returns layoutPoint. 
again it may require to make componenets.

https://codereview.chromium.org/200023002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5121:
rect.move(-FloatSize(visibleContentRect.x().toFloat(),
visibleContentRect.y().toFloat()));
On 2014/03/14 18:25:26, Levi wrote:
> Ditto.
We have below canditates for move
FloatQuad::Move(FloatSize)
FloatQuad::Move(float,float)

if we use visibleContentRect.location() returns layoutPoint. 
again it may require to make componenets.

https://codereview.chromium.org/200023002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2868: int expectedPageWidth =
std::min<float>(documentRect.width().toFloat(), pageSize.width() *
maximumShrinkFactor);
On 2014/03/14 18:25:26, Levi wrote:
> It seems wrong not to call .toInt() on the LayoutUnit here since we're
> ultimately assigning it to an int.
> 
> It'd also be better to have an IntSize for the expected page size instead of
> separated components.

Made it float, because resizePageRectsKeepingRatio takes floatsize
Done.

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
File Source/core/rendering/shapes/BoxShape.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/BoxShape.cpp:80: float y2 = logicalTop.toFloat() +
logicalHeight.toFloat();
On 2014/03/14 18:25:26, Levi wrote:
> Add first, then toFloat() the result. This uses integer math for the addition,
> and only converts to a float once.

Done.

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/BoxShape.cpp:122: float y2 = logicalTop.toFloat() +
logicalHeight.toFloat();
On 2014/03/14 18:25:26, Levi wrote:
> ditto

Done.

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
File Source/core/rendering/shapes/PolygonShape.cpp (right):

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/PolygonShape.cpp:372: float y2 =
logicalTop.toFloat() + logicalHeight.toFloat();
On 2014/03/14 18:25:26, Levi wrote:
> Again, convert the result of the addition.

Done.

https://codereview.chromium.org/200023002/diff/40001/Source/core/rendering/sh...
Source/core/rendering/shapes/PolygonShape.cpp:400: float y2 =
logicalTop.toFloat() + logicalHeight.toFloat();
On 2014/03/14 18:25:26, Levi wrote:
> Ditto.

Done.

[leviw_travelin_and_unemployed, 2014-03-18 19:26:29]

lgtm with a nit about the description.

Please copy the subject into the description otherwise it won't be in the commit
message, and wrap your change description to 80 characters.

https://codereview.chromium.org/200023002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/200023002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2869: FloatSize maxPageSize =
m_frame->resizePageRectsKeepingRatio(FloatSize(originalPageSize.width(),
originalPageSize.height()), expectedPageSize);
This is actually a change in behavior (we'd floor to int then upcast it to float
again here), but if the tests pass, it seems better than the previous behavior.

https://codereview.chromium.org/200023002/diff/60001/Source/platform/LayoutUn...
File Source/platform/LayoutUnit.h (left):

https://codereview.chromium.org/200023002/diff/60001/Source/platform/LayoutUn...
Source/platform/LayoutUnit.h:110: operator float() const { return toFloat(); }
I would have also accepted making this simply "explicit" but this is fine.

[gnana, 2014-03-19 10:01:04]

The CQ bit was checked by gnanasekar.s@samsung.com

[commit-bot: I haz the power, 2014-03-19 10:01:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/200023002/80001

[commit-bot: I haz the power, 2014-03-19 10:49:44]

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

[commit-bot: I haz the power, 2014-03-19 10:49:44]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[gnana, 2014-03-19 11:02:26]

The CQ bit was checked by gnanasekar.s@samsung.com

[commit-bot: I haz the power, 2014-03-19 11:02:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/200023002/80001

[commit-bot: I haz the power, 2014-03-19 11:46:59]

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

[commit-bot: I haz the power, 2014-03-19 11:47:00]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[gnana, 2014-03-19 11:58:24]

The CQ bit was checked by gnanasekar.s@samsung.com

[commit-bot: I haz the power, 2014-03-19 11:58:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/200023002/80001

[commit-bot: I haz the power, 2014-03-19 12:35:15]

Change committed as 169545