aura: Stop converting Rect to RectF implicitly.

aura: Stop converting Rect to RectF implicitly.

This makes the conversions explicit, in the hopes of removing the
ability to do implicit int->float gfx::Rect conversions. This
patch hits the ui/ views/ aura/ ash/ directories.

R=sky@chromium.org
BUG=342848
Committed: https://crrev.com/335602bb0f8576a3439ed80df70fdd0b48ac3bf6
Cr-Commit-Position: refs/heads/master@{#346518}

[danakj, 2015-08-29 00:12:20]

(Also, do you think this is a good idea?)

[sky, 2015-08-29 16:29:54]

int->float, for the most part, isn't lossy like float->int, so I'm not sure this
is worth it. Was there some case you encountered that you made you think the
conversion should be explicit?

[danakj, 2015-08-31 20:06:57]

On Sat, Aug 29, 2015 at 9:29 AM, <sky@chromium.org> wrote:

> int->float, for the most part, isn't lossy like float->int, so I'm not
> sure this
> is worth it. Was there some case you encountered that you made you think
> the
> conversion should be explicit?
>

It's lossy for big numbers, which we've seen before a number of times in
bugs. But I'm more concerned about us doing implicit conversions, rather
than making it clear what the code is doing. In hot code converting
ints/floats is slow and if it's implicit it's harder to know where it is
happening.

Here's a CL in cc/ which has better examples:
https://codereview.chromium.org/1314943008/

The cc/layers/delegated* file changes I found the whole system is using
floats for damage, but we changed damage to be ints a long time ago.
However it was happy to implicitly turn them into floats in this code.

There's some places where we were taking ints, turning them into floats and
back for no reason at all. This used to just turn floats to ints, but now
it turns ints to ints via float :)
https://codereview.chromium.org/1314943008/diff/1/cc/output/software_renderer.cc

These kind of errors accrue over time with implicit conversions.

Do you think these motivations make sense? Or have any other ideas of
things we could do?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-08-31 22:12:22]

On Mon, Aug 31, 2015 at 1:01 PM, Dana Jansens <danakj@chromium.org> wrote:
> On Sat, Aug 29, 2015 at 9:29 AM, <sky@chromium.org> wrote:
>>
>> int->float, for the most part, isn't lossy like float->int, so I'm not
>> sure this
>> is worth it. Was there some case you encountered that you made you think
>> the
>> conversion should be explicit?
>
>
> It's lossy for big numbers, which we've seen before a number of times in
> bugs. But I'm more concerned about us doing implicit conversions, rather
> than making it clear what the code is doing. In hot code converting
> ints/floats is slow and if it's implicit it's harder to know where it is
> happening.
>
> Here's a CL in cc/ which has better examples:
> https://codereview.chromium.org/1314943008/
>
> The cc/layers/delegated* file changes I found the whole system is using
> floats for damage, but we changed damage to be ints a long time ago. However
> it was happy to implicitly turn them into floats in this code.
>
> There's some places where we were taking ints, turning them into floats and
> back for no reason at all. This used to just turn floats to ints, but now it
> turns ints to ints via float :)
>
https://codereview.chromium.org/1314943008/diff/1/cc/output/software_renderer.cc
>
> These kind of errors accrue over time with implicit conversions.
>
> Do you think these motivations make sense? Or have any other ideas of things
> we could do?

I agree. Folks should knowingly want the conversion, which implicit
conversion hides. Let me look at your patch again.

  -Scott

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-08-31 22:15:01]

LGTM

[danakj, 2015-08-31 22:15:38]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-08-31 22:16:56]

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

[commit-bot: I haz the power, 2015-08-31 23:35:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-31 23:36:10]

Patchset 2 (id:??) landed as
https://crrev.com/335602bb0f8576a3439ed80df70fdd0b48ac3bf6
Cr-Commit-Position: refs/heads/master@{#346518}