Refcount touch tracking ids to prevent freeing them too early.

Currently, destroying a ui::TouchEvent removes its tracking id from the slot mapping. Because MenuEventDispatcher::DispatchEvent was creating
(and destroying) a touch event, the tracking id was being
removed too early.

This patch refcounts the tracking ids, so the tracking id won't be removed until it should be.

BUG=394437
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285580

[tdresser, 2014-07-18 13:58:21]

I'm hoping you have some ideas on how to make this a bit more robust.

The way we currently manage tracking ids assumes that only one ui::TouchEvent
will be created per native touch event.

This is one violation of that assumption - do you know of any others?

Are there any free bits on the XEvent that we could set to indicate that a
ui::TouchEvent has been constructed from this XEvent? We could then DCHECK in
the ui::TouchEvent constructor that this XEvent hasn't already been converted
into a ui::TouchEvent.

Another idea that crossed my mind was to keep a counter per tracking id
indicating how many calls to ReleaseSlotForTrackingID should be required before
we actually release the slot.

I'm also not sure if there's a reasonable way to write a test for this change.

Any thoughts?

[tdresser, 2014-07-18 17:46:24]

I'm not happy with much of the naming in this patch - I could rename
ClearTouchIdIfReleased and ReleaseSlotForTrackingId to more clearly indicate
their behavior, if you think that would be an improvement.

I'm also not sure how explicit I should be in calling this "refcounting."

Should I reword the comments as "Increase refcount on entry in tracking id to
slot mapping" or something like that?

[tdresser, 2014-07-22 16:01:01]

On 2014/07/18 17:46:24, tdresser wrote:
> I'm not happy with much of the naming in this patch - I could rename
> ClearTouchIdIfReleased and ReleaseSlotForTrackingId to more clearly indicate
> their behavior, if you think that would be an improvement.
> 
> I'm also not sure how explicit I should be in calling this "refcounting."
> 
> Should I reword the comments as "Increase refcount on entry in tracking id to
> slot mapping" or something like that?

Ping.

[sadrul, 2014-07-22 16:49:16]

https://codereview.chromium.org/400213002/diff/40001/ui/events/x/events_x.cc
File ui/events/x/events_x.cc (right):

https://codereview.chromium.org/400213002/diff/40001/ui/events/x/events_x.cc#...
ui/events/x/events_x.cc:656: factory->AcquireSlotForTrackingID(tracking_id);
Should we do this automagically from GetTouchId() when event-type is
TOUCH_PRESSED? It feels bad to add yet-another X11-only event_utils method that
no other platforms need to implement, but it also feels bad for GetTouchId() to
have a side-effect like this. WDYT?

[tdresser, 2014-07-22 20:17:09]

https://codereview.chromium.org/400213002/diff/40001/ui/events/x/events_x.cc
File ui/events/x/events_x.cc (right):

https://codereview.chromium.org/400213002/diff/40001/ui/events/x/events_x.cc#...
ui/events/x/events_x.cc:656: factory->AcquireSlotForTrackingID(tracking_id);
On 2014/07/22 16:49:16, sadrul wrote:
> Should we do this automagically from GetTouchId() when event-type is
> TOUCH_PRESSED? It feels bad to add yet-another X11-only event_utils method
that
> no other platforms need to implement, but it also feels bad for GetTouchId()
to
> have a side-effect like this. WDYT?

As we just discussed in person, I think there's a lot of value in having the
calls to |IncrementTouchIdRefCount| and |ClearTouchIdIfReleased| in the
constructor and destructor respectively, as it makes it obvious that they match
up.

I'm inclined to go with the current solution.

[sadrul, 2014-07-22 20:20:05]

lgtm

[tdresser, 2014-07-22 20:20:32]

The CQ bit was checked by tdresser@chromium.org

[sadrul, 2014-07-22 20:21:06]

On 2014/07/22 20:20:05, sadrul wrote:
> lgtm

Can you look into adding a test in events_x_unittest.cc for this?

[commit-bot: I haz the power, 2014-07-22 20:22:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/400213002/40001

[tdresser, 2014-07-22 20:29:52]

The CQ bit was unchecked by tdresser@chromium.org

[tdresser, 2014-07-22 21:09:32]

Added test.

[sadrul, 2014-07-22 21:23:21]

https://codereview.chromium.org/400213002/diff/60001/ui/events/x/events_x_uni...
File ui/events/x/events_x_unittest.cc (right):

https://codereview.chromium.org/400213002/diff/60001/ui/events/x/events_x_uni...
ui/events/x/events_x_unittest.cc:319: IncrementTouchIdRefCount(scoped_xevent0);
Instead of calling GetTouchId()/IncrementTouchIdRefCount(), can you create a
ui::TouchEvent here (and below in lines 327:329, 337)? So something like:

   XI2Event xpress0;
   scoped_ptr<ui::TouchEvent> upress0(xpress0);
   XI2Event xpress1;
   for (i = 0 : 4) {
     scoped_ptr<ui::TouchEvent> upress1(xpress1);
     ...
    }

and so on

[tdresser, 2014-07-23 15:15:50]

https://codereview.chromium.org/400213002/diff/60001/ui/events/x/events_x_uni...
File ui/events/x/events_x_unittest.cc (right):

https://codereview.chromium.org/400213002/diff/60001/ui/events/x/events_x_uni...
ui/events/x/events_x_unittest.cc:319: IncrementTouchIdRefCount(scoped_xevent0);
On 2014/07/22 21:23:20, sadrul wrote:
> Instead of calling GetTouchId()/IncrementTouchIdRefCount(), can you create a
> ui::TouchEvent here (and below in lines 327:329, 337)? So something like:
> 
>    XI2Event xpress0;
>    scoped_ptr<ui::TouchEvent> upress0(xpress0);
>    XI2Event xpress1;
>    for (i = 0 : 4) {
>      scoped_ptr<ui::TouchEvent> upress1(xpress1);
>      ...
>     }
> 
> and so on
>    

Done.

[sadrul, 2014-07-24 18:28:23]

lgtm++ Thanks!

[tdresser, 2014-07-24 18:42:37]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-07-24 18:43:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/400213002/100001

[commit-bot: I haz the power, 2014-07-24 19:59:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[commit-bot: I haz the power, 2014-07-24 20:35:53]

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

[commit-bot: I haz the power, 2014-07-24 20:35:54]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[tdresser, 2014-07-24 20:50:00]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-07-24 20:53:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/400213002/100001

[commit-bot: I haz the power, 2014-07-24 21:28:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[commit-bot: I haz the power, 2014-07-24 22:03:52]

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

[commit-bot: I haz the power, 2014-07-24 22:03:52]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[tdresser, 2014-07-25 12:23:40]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-07-25 12:24:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/400213002/100001

[commit-bot: I haz the power, 2014-07-25 12:58:03]

Change committed as 285580