gfx: Fix ToEnclosing/ToEnclosed math to deal with big numbers.

gfx: Fix ToEnclosing/ToEnclosed math to deal with big numbers.

This match ensures that ToEnclosingRect and ToEnclosedRect deal
better with extreme values. In particular, width and height
calculations exhibited undefined behavior with signed integer
overflow. This was fixed by first promoting the operands to float
and then safely truncating the result to int.

R=danakj

Committed: https://crrev.com/03aa528c7796e123655a04c1d3bc3f9a75453ddd
Cr-Commit-Position: refs/heads/master@{#334487}

[vmpstr, 2015-06-15 18:46:13]

vmpstr@chromium.org changed reviewers:
+ danakj@chromium.org

[vmpstr, 2015-06-15 18:46:21]

Please take a look.

[danakj, 2015-06-15 18:48:21]

Thanks for the tests and fixes to the code. I'm not sure why
SafelyScaleToEnclosingRect() is better than ToEnclosingRect(ScaleRect()) ?

[vmpstr, 2015-06-15 18:54:43]

On 2015/06/15 18:48:21, danakj wrote:
> Thanks for the tests and fixes to the code. I'm not sure why
> SafelyScaleToEnclosingRect() is better than ToEnclosingRect(ScaleRect()) ?

Mostly for specifying explicit intent. Someone skimming the code that encounters
ToEnclosingRect(ScaleRect()) might be temped to replace that with
ScaleToEnclosingRect and run into issues with values near
std::numeric_limits<int>::min/max

[danakj, 2015-06-15 18:58:02]

On Mon, Jun 15, 2015 at 11:54 AM, <vmpstr@chromium.org> wrote:

> On 2015/06/15 18:48:21, danakj wrote:
>
>> Thanks for the tests and fixes to the code. I'm not sure why
>> SafelyScaleToEnclosingRect() is better than ToEnclosingRect(ScaleRect()) ?
>>
>
> Mostly for specifying explicit intent. Someone skimming the code that
> encounters
> ToEnclosingRect(ScaleRect()) might be temped to replace that with
> ScaleToEnclosingRect and run into issues with values near
> std::numeric_limits<int>::min/max
>

Can you put it in the same place as ScaleToEnclosingRect then? It seems
like safely should be the default and the other should explain why it's not
safe if we're going to have both.


>
> https://codereview.chromium.org/1186133003/
>

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

[vmpstr, 2015-06-15 19:09:26]

On 2015/06/15 18:58:02, danakj wrote:
> On Mon, Jun 15, 2015 at 11:54 AM, <mailto:vmpstr@chromium.org> wrote:
> 
> > On 2015/06/15 18:48:21, danakj wrote:
> >
> >> Thanks for the tests and fixes to the code. I'm not sure why
> >> SafelyScaleToEnclosingRect() is better than ToEnclosingRect(ScaleRect()) ?
> >>
> >
> > Mostly for specifying explicit intent. Someone skimming the code that
> > encounters
> > ToEnclosingRect(ScaleRect()) might be temped to replace that with
> > ScaleToEnclosingRect and run into issues with values near
> > std::numeric_limits<int>::min/max
> >
> 
> Can you put it in the same place as ScaleToEnclosingRect then? It seems
> like safely should be the default and the other should explain why it's not
> safe if we're going to have both.

Done. From my understanding ScaleToEnclosingRect is faster than
ToEnclosingRect(ScaleRect()), and I think most uses (in cc) are ok, since the
rects are generally clipped to reasonable sized rects (like layer's
content_bounds).

> 
> 
> >
> > https://codereview.chromium.org/1186133003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-06-15 19:13:48]

https://codereview.chromium.org/1186133003/diff/20001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/1186133003/diff/20001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:249: // scale. If the result is scaling would make any
dimension of the rect be too
result from

https://codereview.chromium.org/1186133003/diff/20001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:255: GFX_EXPORT Rect SafelyScaleToEnclosingRect(const
Rect& rect, float scale);
Should we be returning a bool equivalent of |clipped| in MathUtil stuff? Like
here's a rect but it's not actually enclosing. GLHF.

[vmpstr, 2015-06-15 19:33:33]

PTAL. I removed the SafelyScaleToEnclosedRect function. Instead, this patch just
fixes the signed overflow with extreme values.

[danakj, 2015-06-15 20:02:52]

LGTM

[vmpstr, 2015-06-15 20:03:49]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-06-15 20:04:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1186133003/60001

[commit-bot: I haz the power, 2015-06-15 22:11:57]

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

[commit-bot: I haz the power, 2015-06-15 22:11:59]

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

[vmpstr, 2015-06-15 23:03:23]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-06-15 23:05:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1186133003/60001

[commit-bot: I haz the power, 2015-06-15 23:12:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-15 23:12:48]

Patchset 4 (id:??) landed as
https://crrev.com/03aa528c7796e123655a04c1d3bc3f9a75453ddd
Cr-Commit-Position: refs/heads/master@{#334487}