Clean up GestureEventDetails constructors

Clean up GestureEventDetails' constructors.

In one of the GestureEventDetails' constructors, the arguments
delta_x, delta_y sometime are not used, so we should make a new
constructor which only takes one argument EventType.

BUG=350942
Committed: https://crrev.com/b5e408e7bffd3d1e6b60612f65c0538a90329c59
Cr-Commit-Position: refs/heads/master@{#294760}

[lanwei, 2014-09-11 18:42:59]

lanwei@chromium.org changed reviewers:
+ sadrul@chromium.org, tdresser@chromium.org

[lanwei, 2014-09-11 22:43:13]

Patchset #1 (id:1) has been deleted

[lanwei, 2014-09-11 22:43:21]

Patchset #1 (id:20001) has been deleted

[lanwei, 2014-09-11 22:54:29]

lanwei@chromium.org changed reviewers:
- sadrul@chromium.org

[lanwei, 2014-09-11 23:00:06]

Hello Tim, can you please take a look at the gesture_event_details.h and
gesture_event_details.cc files, which I made changes to the constructor, the
other files are modified because of the new constructor.

[lanwei, 2014-09-11 23:01:36]

lanwei@chromium.org changed reviewers:
+ jdduke@chromium.org, sadrul@chromium.org

[lanwei, 2014-09-11 23:01:37]

jdduke@chromium.org: Please review changes in 

sadrul@chromium.org: Please review changes in 

can you please take a look at the gesture_event_details.h and
gesture_event_details.cc files, which I made changes to the constructor, the
other files are modified because of the new constructor.

[jdduke (slow), 2014-09-11 23:37:56]

Yay! ui/events/gesture_detection and
content/browser/renderer_host/input/gesture_text_selector_unittest.cc lgtm.

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
ui/events/gesture_event_details.cc:22: : type_(type), touch_points_(1),
oldest_touch_id_(0) {
I wonder why oldest_touch_id_ is 0 here (and in the new constructor), but -1 in
the empty constructor? Seems like it might make it harder to validate that we're
always assigning the value (where it's used, I think only on Aura?). tdresser@,
thoughts?

[tdresser, 2014-09-12 12:35:27]

Nice!

gesture_event_details.* LGTM!

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
ui/events/gesture_event_details.cc:16: DCHECK_LE(type, ET_GESTURE_TYPE_END);
I think it would be worth DCHECKING that this isn't a gesture type which should
be using the other constructor.

DCHECK_NE(type, ET_GESTURE_SCROLL_BEGIN);
DCHECK_NE(type, ET_GESTURE_SCROLL_UPDATE);
etc.

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
ui/events/gesture_event_details.cc:22: : type_(type), touch_points_(1),
oldest_touch_id_(0) {
On 2014/09/11 23:37:56, jdduke wrote:
> I wonder why oldest_touch_id_ is 0 here (and in the new constructor), but -1
in
> the empty constructor? Seems like it might make it harder to validate that
we're
> always assigning the value (where it's used, I think only on Aura?).
tdresser@,
> thoughts?

Yeah, we should always initialize it to -1.

[lanwei, 2014-09-12 14:58:51]

lanwei@chromium.org changed reviewers:
+ sky@chromium.org

[lanwei, 2014-09-12 14:58:52]

sky@chromium.org: Please review changes in 
We are making changes to the GestureEventDetails's constructors, so we changes
the files that call this constructor. Can you please take a look at these files:

ash/drag_drop/
ash/shelf/
chrome/
content/
ui/wm/
Thank you.

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
ui/events/gesture_event_details.cc:22: : type_(type), touch_points_(1),
oldest_touch_id_(0) {
On 2014/09/12 12:35:27, tdresser wrote:
> On 2014/09/11 23:37:56, jdduke wrote:
> > I wonder why oldest_touch_id_ is 0 here (and in the new constructor), but -1
> in
> > the empty constructor? Seems like it might make it harder to validate that
> we're
> > always assigning the value (where it's used, I think only on Aura?).
> tdresser@,
> > thoughts?
> 
> Yeah, we should always initialize it to -1.

Done.

https://codereview.chromium.org/565583005/diff/40001/ui/events/gesture_event_...
ui/events/gesture_event_details.cc:22: : type_(type), touch_points_(1),
oldest_touch_id_(0) {
On 2014/09/11 23:37:56, jdduke wrote:
> I wonder why oldest_touch_id_ is 0 here (and in the new constructor), but -1
in
> the empty constructor? Seems like it might make it harder to validate that
we're
> always assigning the value (where it's used, I think only on Aura?).
tdresser@,
> thoughts?

Done.

[jdduke (slow), 2014-09-12 15:02:04]

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
ui/events/gesture_event_details.cc:54: NOTREACHED();
It may be worth logging the type here, i.e., 

NOTREACHED() << "Invalid event type for constructor: " << type;

If there's a string converter routine somewhere for EventType, even better, but
if not, the integer should be sufficient.

[sadrul, 2014-09-12 15:28:46]

LGTM

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
ui/events/gesture_event_details.h:19: GestureEventDetails(EventType type);
explicit

[sky, 2014-09-12 15:59:06]

If we're changing things around, how about removing GestureDetails from
GestureEvents constructor and adding:

GestureEventDetails* details() { }

so that people can mutate GestureEventDetails. Seems like this would simplify
useage every where.

[tdresser, 2014-09-12 16:51:17]

On 2014/09/12 15:59:06, sky wrote:
> If we're changing things around, how about removing GestureDetails from
> GestureEvents constructor and adding:
> 
> GestureEventDetails* details() { }
> 
> so that people can mutate GestureEventDetails. Seems like this would simplify
> useage every where.

We don't want people mutating events' GestureEventDetails, do we?

How would this simplify usage?

[sky, 2014-09-12 19:54:29]

On Fri, Sep 12, 2014 at 9:51 AM,  <tdresser@chromium.org> wrote:
> On 2014/09/12 15:59:06, sky wrote:
>>
>> If we're changing things around, how about removing GestureDetails from
>> GestureEvents constructor and adding:
>
>
>> GestureEventDetails* details() { }
>
>
>> so that people can mutate GestureEventDetails. Seems like this would
>> simplify
>> useage every where.
>
>
> We don't want people mutating events' GestureEventDetails, do we?

For the const GestureEvent case, that is correct, but for the
non-const case they should be able to, right?

> How would this simplify usage?

Well, for one you wouldn't have to touch all the files you're doing
now, right? In so far as creators of GestureEvent would only pass in
the type and not explicit details. But you're right, not the greatest
of savings. Ok, leave it like you have it.


  -Scott
>
> https://codereview.chromium.org/565583005/

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

[sky, 2014-09-12 19:54:38]

LGTM

[lanwei, 2014-09-14 02:36:41]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2014-09-14 02:37:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565583005/100001

[lanwei, 2014-09-14 02:46:27]

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
ui/events/gesture_event_details.cc:54: NOTREACHED();
On 2014/09/12 15:02:04, jdduke wrote:
> It may be worth logging the type here, i.e., 
> 
> NOTREACHED() << "Invalid event type for constructor: " << type;
> 
> If there's a string converter routine somewhere for EventType, even better,
but
> if not, the integer should be sufficient.

There is no public string converter function for EventType, but there is a one
method name() in the Event class.

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/565583005/diff/60001/ui/events/gesture_event_...
ui/events/gesture_event_details.h:19: GestureEventDetails(EventType type);
On 2014/09/12 15:28:45, sadrul wrote:
> explicit

Done.

[commit-bot: I haz the power, 2014-09-14 05:26:21]

Committed patchset #4 (id:100001) as 9b493949ce2a6e645c75cde0b572ed7181e8bf30

[commit-bot: I haz the power, 2014-09-14 05:29:30]

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

[nhiroki, 2014-09-15 01:48:43]

A revert of this CL (patchset #4 id:100001) has been created in
https://codereview.chromium.org/572593002/ by nhiroki@chromium.org.

The reason for reverting is: This seems to fail the following tests in
"athena_unittests" on "Linux ChromiumOS Tests (dbg)(3)"

- HomeCardGestureManagerTest.Basic
- HomeCardGestureManagerTest.StartCentered

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[lanwei, 2014-09-16 15:51:19]

Patchset #5 (id:120001) has been deleted