Fixes rounding bug in transform. This is needed to avoid rounding

Fixes rounding bug in transform. This is needed to avoid rounding
something like -.9998 to 0 instead of -1. Without this a transformed
views bounds would end up with an extra scale-1 pixels on the left edge.

BUG=none
TEST=covered by unit test
R=ben@chromium.org,wjmaclean@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86430

[wjmaclean, 2011-05-24 12:11:40]

http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc#newcode77
ui/gfx/transform.cc:77: static_cast<int>(std::floor(skp.fY)));
Just curious ... for positive values, wouldn't this run the danger of converting
0.998 to 0 instead of 1? Do we want rounding or floor/ceil here?

The comment in the unit test mentions only negative numbers, so perhaps I'm
missing how this is used.

[sky, 2011-05-24 15:51:48]

http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc#newcode77
ui/gfx/transform.cc:77: static_cast<int>(std::floor(skp.fY)));
On 2011/05/24 12:11:40, wjmaclean wrote:
> Just curious ... for positive values, wouldn't this run the danger of
converting
> 0.998 to 0 instead of 1? Do we want rounding or floor/ceil here?

I'm sure we'll have rounding problems until we use doubles every where.

The current code uses static cast, which rounds .998 to 0 too, so that behavior
isn't any different. What is different is that negative numbers are now rounded
down, not up (as static_cast does). Here's an example to show the problems with
the current code. Lets say the root view has one child view at 50,50 with a
scale 5. Because the current code rounds down, anything in the range 46-49 of
the parent gets mapped to 0x0 in the child. This means that any mouse movements
from 46-49 get forwarded to the child even though it doesn't contain the point.
By rounding down instead of up for negative numbers we ensure 46-49 are treated
as -1 and don't get forwarded to the child. Does that makes sense?

> The comment in the unit test mentions only negative numbers, so perhaps I'm
> missing how this is used.

[wjmaclean, 2011-05-24 15:58:29]

Thanks, I didn't understand how it was being used.

LGTM!

On 2011/05/24 15:51:48, sky wrote:
> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc
> File ui/gfx/transform.cc (right):
> 
> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc#newcode77
> ui/gfx/transform.cc:77: static_cast<int>(std::floor(skp.fY)));
> On 2011/05/24 12:11:40, wjmaclean wrote:
> > Just curious ... for positive values, wouldn't this run the danger of
> converting
> > 0.998 to 0 instead of 1? Do we want rounding or floor/ceil here?
> 
> I'm sure we'll have rounding problems until we use doubles every where.
> 
> The current code uses static cast, which rounds .998 to 0 too, so that
behavior
> isn't any different. What is different is that negative numbers are now
rounded
> down, not up (as static_cast does). Here's an example to show the problems
with
> the current code. Lets say the root view has one child view at 50,50 with a
> scale 5. Because the current code rounds down, anything in the range 46-49 of
> the parent gets mapped to 0x0 in the child. This means that any mouse
movements
> from 46-49 get forwarded to the child even though it doesn't contain the
point.
> By rounding down instead of up for negative numbers we ensure 46-49 are
treated
> as -1 and don't get forwarded to the child. Does that makes sense?
> 
> > The comment in the unit test mentions only negative numbers, so perhaps I'm
> > missing how this is used.

[sadrul, 2011-05-24 15:59:59]

On 2011/05/24 15:51:48, sky wrote:
> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc
> File ui/gfx/transform.cc (right):
> 
> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc#newcode77
> ui/gfx/transform.cc:77: static_cast<int>(std::floor(skp.fY)));
> On 2011/05/24 12:11:40, wjmaclean wrote:
> > Just curious ... for positive values, wouldn't this run the danger of
> converting
> > 0.998 to 0 instead of 1? Do we want rounding or floor/ceil here?
> 
> I'm sure we'll have rounding problems until we use doubles every where.

I had a TODO to have gfx::Point etc. use float's instead of int's. Does it make
sense to make that change now?

[sky, 2011-05-24 16:07:46]

On Tue, May 24, 2011 at 9:00 AM,  <sadrul@chromium.org> wrote:
> On 2011/05/24 15:51:48, sky wrote:
>>
>> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc
>> File ui/gfx/transform.cc (right):
>
>>
>> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc#newcode77
>> ui/gfx/transform.cc:77: static_cast<int>(std::floor(skp.fY)));
>> On 2011/05/24 12:11:40, wjmaclean wrote:
>> > Just curious ... for positive values, wouldn't this run the danger of
>> converting
>> > 0.998 to 0 instead of 1? Do we want rounding or floor/ceil here?
>
>> I'm sure we'll have rounding problems until we use doubles every where.
>
> I had a TODO to have gfx::Point etc. use float's instead of int's. Does it
> make
> sense to make that change now?

We would have to convert to ints at some point anyway, so I think how
we round would will always be an issue.

  -Scott

[Ben Goodger (Google), 2011-05-24 19:23:11]

LGTM also

On Tue, May 24, 2011 at 9:07 AM, Scott Violet <sky@chromium.org> wrote:

> On Tue, May 24, 2011 at 9:00 AM,  <sadrul@chromium.org> wrote:
> > On 2011/05/24 15:51:48, sky wrote:
> >>
> >> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc
> >> File ui/gfx/transform.cc (right):
> >
> >>
> >>
> http://codereview.chromium.org/7066010/diff/1/ui/gfx/transform.cc#newcode77
> >> ui/gfx/transform.cc:77: static_cast<int>(std::floor(skp.fY)));
> >> On 2011/05/24 12:11:40, wjmaclean wrote:
> >> > Just curious ... for positive values, wouldn't this run the danger of
> >> converting
> >> > 0.998 to 0 instead of 1? Do we want rounding or floor/ceil here?
> >
> >> I'm sure we'll have rounding problems until we use doubles every where.
> >
> > I had a TODO to have gfx::Point etc. use float's instead of int's. Does
> it
> > make
> > sense to make that change now?
>
> We would have to convert to ints at some point anyway, so I think how
> we round would will always be an issue.
>
>  -Scott
>