gfx: Make conversions from gfx::Point to PointF explicit.

gfx: Make conversions from gfx::Point to PointF explicit.

R=sky@chromium.org, vmpstr
BUG=342848
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/089746fb7612de40d86dcb5f440a45b4d8164e95
Cr-Commit-Position: refs/heads/master@{#357460}

[danakj, 2015-09-29 00:31:09]

Good change I missed a build target still but please have a look.

sky: everything but cc
vmpstr: cc

There's a /lot/ of places that make events, so this is a big one :( I can split
it if u want but probably won't help much.

[danakj, 2015-09-29 00:31:35]

On 2015/09/29 00:31:09, danakj wrote:
> Good change I missed a build target still but please have a look.
> 
> sky: everything but cc

oh, ignore content as you please, too. :)

[danakj, 2015-09-29 00:40:55]

On 2015/09/29 00:31:09, danakj wrote:
> Good change I missed a build target still but please have a look.

lol it's late.

I mean, it could be that I missed a build target still, but please have a look.
>__>

[vmpstr, 2015-09-29 05:04:27]

cc lgtm with optional nit

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:1157: gfx::Size
clip_size(23, 21);
It's a bit weird to have a origin and size be in different units

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:3964:
layer1->SetPosition(gfx::PointF());
nit: I kind of weakly prefer to have explicit 0, 0 here. It's really saying
position is at 0, 0. What you have here says "position is whatever the default
point is", which just happens to be the same as (0, 0)

I feel like it's different for gfx::Rect, since when you say gfx::Rect() you
mean an empty rect, which is a reasonable default. The fact that's implemented
as 0, 0 0x0 is also an implementation detail. 

It's up to you though. This is only happening in tests, afaict, so I'm fine with
it either way.

[sky, 2015-09-29 16:27:36]

sky@chromium.org changed reviewers:
+ sadrul@chormium.org

[sky, 2015-09-29 16:27:37]

I looked at a handful of the changes, and most were because
LocatedEvent::set_location takes a PointF. As we predominantly use LocatedEvent
with integer points (on windows we always create LocationEvent with integer
points) I'm inclined to think LocatedEvent should take a set_location(Point) and
implicitly do the conversion. WDYT?
+sadrul

[danakj, 2015-10-12 17:29:00]

On 2015/09/29 16:27:37, sky wrote:
> I looked at a handful of the changes, and most were because
> LocatedEvent::set_location takes a PointF. As we predominantly use
LocatedEvent
> with integer points (on windows we always create LocationEvent with integer
> points) I'm inclined to think LocatedEvent should take a set_location(Point)
and
> implicitly do the conversion. WDYT?
> +sadrul

I started the patch that way, but there's a few places that do pass a PointF or
get one back. The Points are mostly in test code, (tons of unit tests) whereas
in prod it does use a PointF sometimes.

We could have both setters if you prefer, I just generally don't like adding
prod functions for tests.

[danakj, 2015-10-12 17:32:01]

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:1157: gfx::Size
clip_size(23, 21);
On 2015/09/29 05:04:26, vmpstr wrote:
> It's a bit weird to have a origin and size be in different units

Ya, it's because Layer::Position and Layer::Size are different though. Positions
can be sub-pixel, sizes can't from blink.

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:3964:
layer1->SetPosition(gfx::PointF());
On 2015/09/29 05:04:26, vmpstr wrote:
> nit: I kind of weakly prefer to have explicit 0, 0 here. It's really saying
> position is at 0, 0. What you have here says "position is whatever the default
> point is", which just happens to be the same as (0, 0)
> 
> I feel like it's different for gfx::Rect, since when you say gfx::Rect() you
> mean an empty rect, which is a reasonable default. The fact that's implemented
> as 0, 0 0x0 is also an implementation detail. 
> 
> It's up to you though. This is only happening in tests, afaict, so I'm fine
with
> it either way.

PointF() is always 0,0 and I think that is pretty well known. There's so much
code that depends on that it's not like we could change it easily. I get what
you mean about an implementation detail but I think this is actually its API.
Point/Vector/Size/Rect are all zeros from default constructors.

[sky, 2015-10-12 19:12:09]

On 2015/10/12 17:29:00, danakj wrote:
> On 2015/09/29 16:27:37, sky wrote:
> > I looked at a handful of the changes, and most were because
> > LocatedEvent::set_location takes a PointF. As we predominantly use
> LocatedEvent
> > with integer points (on windows we always create LocationEvent with integer
> > points) I'm inclined to think LocatedEvent should take a set_location(Point)
> and
> > implicitly do the conversion. WDYT?
> > +sadrul
> 
> I started the patch that way, but there's a few places that do pass a PointF
or
> get one back. The Points are mostly in test code, (tons of unit tests) whereas
> in prod it does use a PointF sometimes.
> 
> We could have both setters if you prefer, I just generally don't like adding
> prod functions for tests.

I like having both.

[danakj, 2015-10-12 20:47:02]

On Mon, Oct 12, 2015 at 12:12 PM, <sky@chromium.org> wrote:

> On 2015/10/12 17:29:00, danakj wrote:
>
>> On 2015/09/29 16:27:37, sky wrote:
>> > I looked at a handful of the changes, and most were because
>> > LocatedEvent::set_location takes a PointF. As we predominantly use
>> LocatedEvent
>> > with integer points (on windows we always create LocationEvent with
>> integer
>> > points) I'm inclined to think LocatedEvent should take a
>> set_location(Point)
>> and
>> > implicitly do the conversion. WDYT?
>> > +sadrul
>>
>
> I started the patch that way, but there's a few places that do pass a
>> PointF
>>
> or
>
>> get one back. The Points are mostly in test code, (tons of unit tests)
>> whereas
>> in prod it does use a PointF sometimes.
>>
>
> We could have both setters if you prefer, I just generally don't like
>> adding
>> prod functions for tests.
>>
>
> I like having both.
>

kk I'll give that a go. The sad part is we'll be using function
(constructor) overloading with different types that make it not obvious
which get called. This really breaks the style guide.

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

[danakj, 2015-10-12 20:56:07]

danakj@chromium.org changed reviewers:
+ sadrul@chromium.org
- sadrul@chormium.org

[danakj, 2015-10-12 20:56:32]

+the real sadrul

[sky, 2015-10-12 23:37:10]

On Mon, Oct 12, 2015 at 1:46 PM, Dana Jansens <danakj@chromium.org> wrote:
> On Mon, Oct 12, 2015 at 12:12 PM, <sky@chromium.org> wrote:
>>
>> On 2015/10/12 17:29:00, danakj wrote:
>>>
>>> On 2015/09/29 16:27:37, sky wrote:
>>> > I looked at a handful of the changes, and most were because
>>> > LocatedEvent::set_location takes a PointF. As we predominantly use
>>> LocatedEvent
>>> > with integer points (on windows we always create LocationEvent with
>>> > integer
>>> > points) I'm inclined to think LocatedEvent should take a
>>> > set_location(Point)
>>> and
>>> > implicitly do the conversion. WDYT?
>>> > +sadrul
>>
>>
>>> I started the patch that way, but there's a few places that do pass a
>>> PointF
>>
>> or
>>>
>>> get one back. The Points are mostly in test code, (tons of unit tests)
>>> whereas
>>> in prod it does use a PointF sometimes.
>>
>>
>>> We could have both setters if you prefer, I just generally don't like
>>> adding
>>> prod functions for tests.
>>
>>
>> I like having both.
>
>
> kk I'll give that a go. The sad part is we'll be using function
> (constructor) overloading with different types that make it not obvious
> which get called. This really breaks the style guide.

Ick!
These classes support copying. Would a static factory function solve that?

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

[sadrul, 2015-10-13 21:25:01]

Would it make sense to allow implicit Point -> PointF conversion in tests? Would
that make things easier?

[danakj, 2015-10-13 21:59:01]

On Tue, Oct 13, 2015 at 2:25 PM, <sadrul@chromium.org> wrote:

> Would it make sense to allow implicit Point -> PointF conversion in tests?
> Would
> that make things easier?
>

That's an interesting thought, but the style guide is not different for
tests. And it would probably be pretty confusing. :)

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

[danakj, 2015-10-20 20:45:35]

On 2015/10/12 23:37:10, sky wrote:
> > kk I'll give that a go. The sad part is we'll be using function
> > (constructor) overloading with different types that make it not obvious
> > which get called. This really breaks the style guide.
> 
> Ick!
> These classes support copying. Would a static factory function solve that?

You're thinking like:

Foo::Foo(.... PointF ...)
and
Foo::CreateFromPoint(... Point ... )?

That would touch exactly the same number of callsites and I'm not sure what the
benefit is over just doing Foo(PointF(p)) directly.

[danakj, 2015-10-20 20:53:48]

On 2015/10/20 20:45:35, danakj wrote:
> On 2015/10/12 23:37:10, sky wrote:
> > > kk I'll give that a go. The sad part is we'll be using function
> > > (constructor) overloading with different types that make it not obvious
> > > which get called. This really breaks the style guide.
> > 
> > Ick!
> > These classes support copying. Would a static factory function solve that?
> 
> You're thinking like:
> 
> Foo::Foo(.... PointF ...)
> and
> Foo::CreateFromPoint(... Point ... )?
> 
> That would touch exactly the same number of callsites and I'm not sure what
the
> benefit is over just doing Foo(PointF(p)) directly.

I'll try split this up, it is clearly too big to review as one piece.

[danakj, 2015-10-20 23:10:01]

Patchset #1 (id:1) has been deleted

[danakj, 2015-10-20 23:10:15]

Patchset #1 (id:20001) has been deleted

[danakj, 2015-10-20 23:10:28]

Patchset #1 (id:40001) has been deleted

[sky, 2015-10-23 15:31:20]

On 2015/10/20 20:53:48, danakj wrote:
> On 2015/10/20 20:45:35, danakj wrote:
> > On 2015/10/12 23:37:10, sky wrote:
> > > > kk I'll give that a go. The sad part is we'll be using function
> > > > (constructor) overloading with different types that make it not obvious
> > > > which get called. This really breaks the style guide.
> > > 
> > > Ick!
> > > These classes support copying. Would a static factory function solve that?
> > 
> > You're thinking like:
> > 
> > Foo::Foo(.... PointF ...)
> > and
> > Foo::CreateFromPoint(... Point ... )?
> > 
> > That would touch exactly the same number of callsites and I'm not sure what
> the
> > benefit is over just doing Foo(PointF(p)) directly.
> 
> I'll try split this up, it is clearly too big to review as one piece.

I'm replying on this thread as this is where most of the conversation has
happened.

What happened to the idea of a constructor that takes the integer types? The
reason I suggest that is that views is predominantly integer based, so that
integer based mouse, gesture and located events are typical.

[sky, 2015-10-23 16:03:31]

In fact I'll go so far as to say the floating point ones are the
obscure ones, and I would rather go through extra hoops to create
those than make everyone have to create floats.

  -Scott

On Fri, Oct 23, 2015 at 8:31 AM,  <sky@chromium.org> wrote:
> On 2015/10/20 20:53:48, danakj wrote:
>>
>> On 2015/10/20 20:45:35, danakj wrote:
>> > On 2015/10/12 23:37:10, sky wrote:
>> > > > kk I'll give that a go. The sad part is we'll be using function
>> > > > (constructor) overloading with different types that make it not
>> > > > obvious
>> > > > which get called. This really breaks the style guide.
>> > >
>> > > Ick!
>> > > These classes support copying. Would a static factory function solve
>> > > that?
>> >
>> > You're thinking like:
>> >
>> > Foo::Foo(.... PointF ...)
>> > and
>> > Foo::CreateFromPoint(... Point ... )?
>> >
>> > That would touch exactly the same number of callsites and I'm not sure
>> > what
>> the
>> > benefit is over just doing Foo(PointF(p)) directly.
>
>
>> I'll try split this up, it is clearly too big to review as one piece.
>
>
> I'm replying on this thread as this is where most of the conversation has
> happened.
>
> What happened to the idea of a constructor that takes the integer types? The
> reason I suggest that is that views is predominantly integer based, so that
> integer based mouse, gesture and located events are typical.
>
> https://codereview.chromium.org/1372253002/

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

[danakj, 2015-10-23 18:33:52]

On Fri, Oct 23, 2015 at 9:03 AM, Scott Violet <sky@chromium.org> wrote:

> In fact I'll go so far as to say the floating point ones are the
> obscure ones, and I would rather go through extra hoops to create
> those than make everyone have to create floats.
>

The problem with constructor overloading was two things:
1. It's not obvious reading a callsite which is getting called which the
style guide dislikes.
2. Then you lose the advantage of no-implicit-conversions. That basically
does make an implicit conversion. Then the places in
https://codereview.chromium.org/1423703002/ would continue to compile
passing location() instead of location_f() so you can't tell that you're
dropping data and implicitly converting. If you take a PointF and can't
implicitly convert, then dropping the floats would be a compiler error
which is pretty obvious.

If you include the callsites in https://codereview.chromium.org/1423703002/
and https://codereview.chromium.org/1407133008/ and
https://codereview.chromium.org/1421713002/#msg9 I count 25 total, of which
at least half were doing problematic things.

In https://codereview.chromium.org/1421713002/ I added casts or changed
types in ~54 places. So you're right there's def more.

I can do that if you want. But you're going to lose all the typesafety that
explicit conversions is about. Let me know.


>
>   -Scott
>
> On Fri, Oct 23, 2015 at 8:31 AM,  <sky@chromium.org> wrote:
> > On 2015/10/20 20:53:48, danakj wrote:
> >>
> >> On 2015/10/20 20:45:35, danakj wrote:
> >> > On 2015/10/12 23:37:10, sky wrote:
> >> > > > kk I'll give that a go. The sad part is we'll be using function
> >> > > > (constructor) overloading with different types that make it not
> >> > > > obvious
> >> > > > which get called. This really breaks the style guide.
> >> > >
> >> > > Ick!
> >> > > These classes support copying. Would a static factory function solve
> >> > > that?
> >> >
> >> > You're thinking like:
> >> >
> >> > Foo::Foo(.... PointF ...)
> >> > and
> >> > Foo::CreateFromPoint(... Point ... )?
> >> >
> >> > That would touch exactly the same number of callsites and I'm not sure
> >> > what
> >> the
> >> > benefit is over just doing Foo(PointF(p)) directly.
> >
> >
> >> I'll try split this up, it is clearly too big to review as one piece.
> >
> >
> > I'm replying on this thread as this is where most of the conversation has
> > happened.
> >
> > What happened to the idea of a constructor that takes the integer types?
> The
> > reason I suggest that is that views is predominantly integer based, so
> that
> > integer based mouse, gesture and located events are typical.
> >
> > https://codereview.chromium.org/1372253002/
>

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

[danakj, 2015-10-23 19:50:43]

Patchset #15 (id:330001) has been deleted

[sky, 2015-10-23 20:43:29]

*SIGH* you are right.

My main gripe is we're adding all this complexity for the uncommon case. I
realize PointF is not complex, but I hate having to thinking about it,
which is what folks are going to have to do more of now...

  -Scott

On Fri, Oct 23, 2015 at 11:26 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Oct 23, 2015 at 9:03 AM, Scott Violet <sky@chromium.org> wrote:
>
>> In fact I'll go so far as to say the floating point ones are the
>> obscure ones, and I would rather go through extra hoops to create
>> those than make everyone have to create floats.
>>
>
> The problem with constructor overloading was two things:
> 1. It's not obvious reading a callsite which is getting called which the
> style guide dislikes.
> 2. Then you lose the advantage of no-implicit-conversions. That basically
> does make an implicit conversion. Then the places in
> https://codereview.chromium.org/1423703002/ would continue to compile
> passing location() instead of location_f() so you can't tell that you're
> dropping data and implicitly converting. If you take a PointF and can't
> implicitly convert, then dropping the floats would be a compiler error
> which is pretty obvious.
>
> If you include the callsites in
> https://codereview.chromium.org/1423703002/ and
> https://codereview.chromium.org/1407133008/ and
> https://codereview.chromium.org/1421713002/#msg9 I count 25 total, of
> which at least half were doing problematic things.
>
> In https://codereview.chromium.org/1421713002/ I added casts or changed
> types in ~54 places. So you're right there's def more.
>
> I can do that if you want. But you're going to lose all the typesafety
> that explicit conversions is about. Let me know.
>
>
>>
>>   -Scott
>>
>> On Fri, Oct 23, 2015 at 8:31 AM,  <sky@chromium.org> wrote:
>> > On 2015/10/20 20:53:48, danakj wrote:
>> >>
>> >> On 2015/10/20 20:45:35, danakj wrote:
>> >> > On 2015/10/12 23:37:10, sky wrote:
>> >> > > > kk I'll give that a go. The sad part is we'll be using function
>> >> > > > (constructor) overloading with different types that make it not
>> >> > > > obvious
>> >> > > > which get called. This really breaks the style guide.
>> >> > >
>> >> > > Ick!
>> >> > > These classes support copying. Would a static factory function
>> solve
>> >> > > that?
>> >> >
>> >> > You're thinking like:
>> >> >
>> >> > Foo::Foo(.... PointF ...)
>> >> > and
>> >> > Foo::CreateFromPoint(... Point ... )?
>> >> >
>> >> > That would touch exactly the same number of callsites and I'm not
>> sure
>> >> > what
>> >> the
>> >> > benefit is over just doing Foo(PointF(p)) directly.
>> >
>> >
>> >> I'll try split this up, it is clearly too big to review as one piece.
>> >
>> >
>> > I'm replying on this thread as this is where most of the conversation
>> has
>> > happened.
>> >
>> > What happened to the idea of a constructor that takes the integer
>> types? The
>> > reason I suggest that is that views is predominantly integer based, so
>> that
>> > integer based mouse, gesture and located events are typical.
>> >
>> > https://codereview.chromium.org/1372253002/
>>
>
>

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

[danakj, 2015-10-23 20:57:22]

On Fri, Oct 23, 2015 at 1:43 PM, Scott Violet <sky@chromium.org> wrote:

> *SIGH* you are right.
>
> My main gripe is we're adding all this complexity for the uncommon case. I
> realize PointF is not complex, but I hate having to thinking about it,
> which is what folks are going to have to do more of now...
>

Ya, I think that complexity was added when events were made to have PointFs
(e53cc7742f11156650dd63362383eb6cec27e0cd) but mostly hidden at that time
accidentally, which also hid buggy code. There was a TODO at that time to
make location() return PointF instead of Point which would have resolved
this, but that was eventually removed while leaving location() and
location_f() it seems.

Had we made this conversion explicit at that time (sorry!) it would have
been more clear at the code review stage and the merits of the complexity
could have been debated at a better time.

Perhaps if UI code is moving toward being more DSF aware and less DIP-y,
events can stop having floats again in the future. I'm not really sure what
the future plans hold around this stuff. But for now, not thinking about it
seems worse than hiding it.



>
>   -Scott
>
> On Fri, Oct 23, 2015 at 11:26 AM, Dana Jansens <danakj@chromium.org>
> wrote:
>
>> On Fri, Oct 23, 2015 at 9:03 AM, Scott Violet <sky@chromium.org> wrote:
>>
>>> In fact I'll go so far as to say the floating point ones are the
>>> obscure ones, and I would rather go through extra hoops to create
>>> those than make everyone have to create floats.
>>>
>>
>> The problem with constructor overloading was two things:
>> 1. It's not obvious reading a callsite which is getting called which the
>> style guide dislikes.
>> 2. Then you lose the advantage of no-implicit-conversions. That basically
>> does make an implicit conversion. Then the places in
>> https://codereview.chromium.org/1423703002/ would continue to compile
>> passing location() instead of location_f() so you can't tell that you're
>> dropping data and implicitly converting. If you take a PointF and can't
>> implicitly convert, then dropping the floats would be a compiler error
>> which is pretty obvious.
>>
>> If you include the callsites in
>> https://codereview.chromium.org/1423703002/ and
>> https://codereview.chromium.org/1407133008/ and
>> https://codereview.chromium.org/1421713002/#msg9 I count 25 total, of
>> which at least half were doing problematic things.
>>
>> In https://codereview.chromium.org/1421713002/ I added casts or changed
>> types in ~54 places. So you're right there's def more.
>>
>> I can do that if you want. But you're going to lose all the typesafety
>> that explicit conversions is about. Let me know.
>>
>>
>>>
>>>   -Scott
>>>
>>> On Fri, Oct 23, 2015 at 8:31 AM,  <sky@chromium.org> wrote:
>>> > On 2015/10/20 20:53:48, danakj wrote:
>>> >>
>>> >> On 2015/10/20 20:45:35, danakj wrote:
>>> >> > On 2015/10/12 23:37:10, sky wrote:
>>> >> > > > kk I'll give that a go. The sad part is we'll be using function
>>> >> > > > (constructor) overloading with different types that make it not
>>> >> > > > obvious
>>> >> > > > which get called. This really breaks the style guide.
>>> >> > >
>>> >> > > Ick!
>>> >> > > These classes support copying. Would a static factory function
>>> solve
>>> >> > > that?
>>> >> >
>>> >> > You're thinking like:
>>> >> >
>>> >> > Foo::Foo(.... PointF ...)
>>> >> > and
>>> >> > Foo::CreateFromPoint(... Point ... )?
>>> >> >
>>> >> > That would touch exactly the same number of callsites and I'm not
>>> sure
>>> >> > what
>>> >> the
>>> >> > benefit is over just doing Foo(PointF(p)) directly.
>>> >
>>> >
>>> >> I'll try split this up, it is clearly too big to review as one piece.
>>> >
>>> >
>>> > I'm replying on this thread as this is where most of the conversation
>>> has
>>> > happened.
>>> >
>>> > What happened to the idea of a constructor that takes the integer
>>> types? The
>>> > reason I suggest that is that views is predominantly integer based, so
>>> that
>>> > integer based mouse, gesture and located events are typical.
>>> >
>>> > https://codereview.chromium.org/1372253002/
>>>
>>
>>
>

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

[danakj, 2015-10-30 20:31:24]

PTAL

[danakj, 2015-10-30 21:22:44]

Added some more changes for browser tests

[sky, 2015-10-30 22:20:44]

This is mildly confusing, but I think it's the best option given the state of
the world. Thanks for the diligence!
LGTM

https://codereview.chromium.org/1372253002/diff/430001/ui/aura/gestures/gestu...
File ui/aura/gestures/gesture_recognizer_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/430001/ui/aura/gestures/gestu...
ui/aura/gestures/gesture_recognizer_unittest.cc:505: float x_start,
AFAICT x_start and y_start is always supplied integer values.

https://codereview.chromium.org/1372253002/diff/430001/ui/events/event_unitte...
File ui/events/event_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/430001/ui/events/event_unitte...
ui/events/event_unittest.cc:645: ui::MouseEvent mouse_event(ET_MOUSE_PRESSED,
gfx::Point(0, 0),
0,0 is synonymous with gfx::Point()

[danakj, 2015-10-30 22:39:46]

https://codereview.chromium.org/1372253002/diff/430001/ui/aura/gestures/gestu...
File ui/aura/gestures/gesture_recognizer_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/430001/ui/aura/gestures/gestu...
ui/aura/gestures/gesture_recognizer_unittest.cc:505: float x_start,
On 2015/10/30 22:20:44, sky wrote:
> AFAICT x_start and y_start is always supplied integer values.

Done.

https://codereview.chromium.org/1372253002/diff/430001/ui/events/event_unitte...
File ui/events/event_unittest.cc (right):

https://codereview.chromium.org/1372253002/diff/430001/ui/events/event_unitte...
ui/events/event_unittest.cc:645: ui::MouseEvent mouse_event(ET_MOUSE_PRESSED,
gfx::Point(0, 0),
On 2015/10/30 22:20:44, sky wrote:
> 0,0 is synonymous with gfx::Point()

Ya, it is. vmpstr recently convinced me that writing Point(0, 0) is actually
nicer to the reader than Point() when you care about the value of the thing, so
I stopped removing 0,0s where they exist.

[danakj, 2015-10-30 22:47:13]

danakj@chromium.org changed reviewers:
+ grt@chromium.org, sievers@chromium.org

[danakj, 2015-10-30 22:47:14]

+grt for win8/
+sievers for content/

[no sievers, 2015-10-30 22:54:21]

content lgtm

[danakj, 2015-10-30 23:25:55]

https://codereview.chromium.org/1372253002/diff/470001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/host/drm_cursor.cc (right):

https://codereview.chromium.org/1372253002/diff/470001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/host/drm_cursor.cc:61:
SetCursorLocationLocked(gfx::PointF(cursor_confined_bounds.CenterPoint()));
+dnicoara@chromium.org

It's not obvious to me that cursor locations should be floats, but FYI for these
changes.

[danakj, 2015-10-30 23:26:30]

Description was changed from

==========
gfx: Make conversions from gfx::Point to PointF explicit.

R=sky@chromium.org, vmpstr
BUG=342848
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
gfx: Make conversions from gfx::Point to PointF explicit.

R=sky@chromium.org, vmpstr
TBR=grt
BUG=342848
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2015-10-31 00:26:35]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-10-31 00:26:36]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
sky@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1372253002/#ps510001
(title: "pointfconvert-event: couplemore")

[commit-bot: I haz the power, 2015-10-31 00:27:02]

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

[commit-bot: I haz the power, 2015-10-31 00:47:47]

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

[commit-bot: I haz the power, 2015-10-31 00:47:49]

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

[grt (UTC plus 2), 2015-10-31 13:02:31]

win8 lgtm

[danakj, 2015-11-02 18:57:43]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-11-02 18:57:43]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
sky@chromium.org, sievers@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1372253002/#ps530001
(title: "pointfconvert-event: somemorestuff")

[danakj, 2015-11-02 18:58:29]

Description was changed from

==========
gfx: Make conversions from gfx::Point to PointF explicit.

R=sky@chromium.org, vmpstr
TBR=grt
BUG=342848
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
gfx: Make conversions from gfx::Point to PointF explicit.

R=sky@chromium.org, vmpstr
BUG=342848
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2015-11-02 18:58:36]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2015-11-02 18:58:46]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-11-02 18:59:17]

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

[commit-bot: I haz the power, 2015-11-02 19:16:45]

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

[commit-bot: I haz the power, 2015-11-02 19:16:47]

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

[danakj, 2015-11-02 19:35:15]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-11-02 19:35:15]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
sky@chromium.org, sievers@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1372253002/#ps550001
(title: "pointfconvert-event: pointsarepoints")

[commit-bot: I haz the power, 2015-11-02 19:36:36]

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

[commit-bot: I haz the power, 2015-11-02 20:23:02]

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

[commit-bot: I haz the power, 2015-11-02 20:23:05]

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

[danakj, 2015-11-02 20:59:47]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-11-02 20:59:48]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
sky@chromium.org, sievers@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1372253002/#ps570001
(title: "pointfconvert-event: .")

[commit-bot: I haz the power, 2015-11-02 21:00:30]

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

[commit-bot: I haz the power, 2015-11-02 21:25:55]

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

[commit-bot: I haz the power, 2015-11-02 21:25:57]

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

[danakj, 2015-11-02 21:32:26]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-11-02 21:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
sky@chromium.org, sievers@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1372253002/#ps590001
(title: "pointfconvert-event: mandol_line")

[commit-bot: I haz the power, 2015-11-02 21:34:31]

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

[danakj, 2015-11-02 21:36:57]

On Mon, Nov 2, 2015 at 1:25 PM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

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

mandoline T_T wtb component build targets

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

[commit-bot: I haz the power, 2015-11-02 22:52:44]

Committed patchset #27 (id:590001)

[commit-bot: I haz the power, 2015-11-02 22:53:53]

Patchset 27 (id:??) landed as
https://crrev.com/089746fb7612de40d86dcb5f440a45b4d8164e95
Cr-Commit-Position: refs/heads/master@{#357460}