Reland Unified Gesture Recognizer for Aura

Reland Unified Gesture Recognizer for Aura

Original patch here: https://codereview.chromium.org/251543003/.
Reverted here: https://codereview.chromium.org/278183002/,
due to memory management issues in tests.

Relanding with a better strategy for cleaning up deleted
GestureProviderAura objects.

This adds the unified GR for Aura behind the --use-unified-gesture-detector flag.

BUG=332418
TEST=GestureRecognizer/GestureRecognizerTest.*
MotionEventUITest.*
TBR=jochen

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

[tdresser, 2014-05-12 13:32:43]

sadrul@, the unified GR was reverted while my fix was stuck in the CQ.

Relanding - the only change is the way that I'm deleting GestureProviderAura
objects.

Can you take a look?
The changes are in gesture_recognizer_impl.cc::323-329.

This approach may delay deleting the AuraGestureProvider slightly more than the
other approach, but I think it's probably more efficient overall.

[tdresser, 2014-05-12 16:07:01]

On 2014/05/12 13:32:43, tdresser wrote:
> sadrul@, the unified GR was reverted while my fix was stuck in the CQ.
> 
> Relanding - the only change is the way that I'm deleting GestureProviderAura
> objects.
> 
> Can you take a look?
> The changes are in gesture_recognizer_impl.cc::323-329.
> 
> This approach may delay deleting the AuraGestureProvider slightly more than
the
> other approach, but I think it's probably more efficient overall.

Hmmm, the fact that patch set #2 is needed makes me think this approach may not
be correct.

I can add a test to ensure that the crash that happens with patch set #1 won't
happen again, but this feels like it may have some other subtleties. Do you have
any thoughts?

[sadrul, 2014-05-14 02:56:13]

On 2014/05/12 16:07:01, tdresser wrote:
> On 2014/05/12 13:32:43, tdresser wrote:
> > sadrul@, the unified GR was reverted while my fix was stuck in the CQ.
> > 
> > Relanding - the only change is the way that I'm deleting GestureProviderAura
> > objects.
> > 
> > Can you take a look?
> > The changes are in gesture_recognizer_impl.cc::323-329.
> > 
> > This approach may delay deleting the AuraGestureProvider slightly more than
> the
> > other approach, but I think it's probably more efficient overall.
> 
> Hmmm, the fact that patch set #2 is needed makes me think this approach may
not
> be correct.
> 
> I can add a test to ensure that the crash that happens with patch set #1 won't
> happen again, but this feels like it may have some other subtleties. Do you
have
> any thoughts?

What is the crash that happens with PS#1 (the tries all seem green for that
one)?

Do we have any insight on why DeleteSoon() didn't work?

Can we tell the GestureProviderAura to destroy itself when it's done?

[tdresser, 2014-05-14 14:13:30]

The issue with DeleteSoon was that I was calling RunAllPendingInMessageLoop too
early - it needed to be called after AuraTestBase::TearDown.

I haven't dug into the crash which occurred in PS#1 - I can take a closer look
if you think it's worth the time. It's a bit hard to reproduce.

[tdresser, 2014-05-15 14:55:12]

Adding jochen@ for **/render_widget_host_view_guest.*
Adding oshima@ for **/drag_drop_controller_unittest.cc

PTAL

[oshima, 2014-05-15 15:56:28]

ash lgtm

looks like nothing has changed in drag_drop test code so you may TBR.

[sadrul, 2014-05-15 16:02:26]

lgtm

[tdresser, 2014-05-15 17:07:06]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-15 17:08:50]

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

[commit-bot: I haz the power, 2014-05-15 18:47:20]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-15 18:57:34]

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

[commit-bot: I haz the power, 2014-05-15 18:57:34]

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

[tdresser, 2014-05-15 19:07:41]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-15 19:09:04]

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

[commit-bot: I haz the power, 2014-05-15 21:21:07]

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

[commit-bot: I haz the power, 2014-05-15 21:21:08]

Failed to apply patch for ui/events/events.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/events/events.gyp
  Hunk #4 succeeded at 133 (offset -12 lines).
  Hunk #5 succeeded at 185 (offset -34 lines).
  Hunk #6 succeeded at 195 (offset -34 lines).
  Hunk #7 succeeded at 210 (offset -34 lines).
  Hunk #8 FAILED at 303.
  Hunk #9 succeeded at 279 (offset -32 lines).
  Hunk #10 succeeded at 301 (offset -32 lines).
  1 out of 10 hunks FAILED -- saving rejects to file ui/events/events.gyp.rej

Patch:       ui/events/events.gyp
Index: ui/events/events.gyp
diff --git a/ui/events/events.gyp b/ui/events/events.gyp
index
a4ae41828ebb292e296c68ef48e56a634994c79b..f8a6e4f32641959179288b96c0b103d79830e152
100644
--- a/ui/events/events.gyp
+++ b/ui/events/events.gyp
@@ -40,6 +40,8 @@
         'events_base_export.h',
         'gesture_event_details.cc',
         'gesture_event_details.h',
+        'gestures/gesture_configuration.cc',
+        'gestures/gesture_configuration.h',
         'keycodes/keyboard_code_conversion.cc',
         'keycodes/keyboard_code_conversion.h',
         'keycodes/keyboard_code_conversion_android.cc',
@@ -79,6 +81,7 @@
         '../gfx/gfx.gyp:gfx',
         '../gfx/gfx.gyp:gfx_geometry',
         'events_base',
+        'gesture_detection',
       ],
       'defines': [
         'EVENTS_IMPLEMENTATION',
@@ -107,16 +110,18 @@
         'event_utils.h',
         'events_export.h',
         'events_stub.cc',
-        'gestures/gesture_configuration.cc',
-        'gestures/gesture_configuration.h',
         'gestures/gesture_point.cc',
         'gestures/gesture_point.h',
+        'gestures/gesture_provider_aura.cc',
+        'gestures/gesture_provider_aura.h',
         'gestures/gesture_recognizer.h',
         'gestures/gesture_recognizer_impl.cc',
         'gestures/gesture_recognizer_impl.h',
         'gestures/gesture_sequence.cc',
         'gestures/gesture_sequence.h',
         'gestures/gesture_types.h',
+        'gestures/motion_event_aura.cc',
+        'gestures/motion_event_aura.h',
         'gestures/velocity_calculator.cc',
         'gestures/velocity_calculator.h',
         'ozone/events_ozone.cc',
@@ -140,6 +145,24 @@
         'linux/text_edit_key_bindings_delegate_auralinux.h',
       ],
       'conditions': [
+        ['use_aura==0', {
+          'sources!': [
+            'gestures/gesture_point.cc',
+            'gestures/gesture_point.h',
+            'gestures/gesture_provider_aura.cc',
+            'gestures/gesture_provider_aura.h',
+            'gestures/gesture_recognizer.h',
+            'gestures/gesture_recognizer_impl.cc',
+            'gestures/gesture_recognizer_impl.h',
+            'gestures/gesture_sequence.cc',
+            'gestures/gesture_sequence.h',
+            'gestures/gesture_types.h',
+            'gestures/motion_event_aura.cc',
+            'gestures/motion_event_aura.h',
+            'gestures/velocity_calculator.cc',
+            'gestures/velocity_calculator.h',
+          ],
+        }],
         # We explicitly enumerate the platforms we _do_ provide native cracking
         # for here.
         ['OS=="win" or OS=="mac" or use_x11==1 or use_ozone==1', {
@@ -196,6 +219,9 @@
         'gesture_detection/bitset_32.h',
         'gesture_detection/filtered_gesture_provider.cc',
         'gesture_detection/filtered_gesture_provider.h',
+        'gesture_detection/gesture_config_helper.h',
+        'gesture_detection/gesture_config_helper_android.cc',
+        'gesture_detection/gesture_config_helper_aura.cc',
         'gesture_detection/gesture_detection_export.h',
         'gesture_detection/gesture_detector.cc',
         'gesture_detection/gesture_detector.h',
@@ -203,9 +229,6 @@
         'gesture_detection/gesture_event_data.h',
         'gesture_detection/gesture_event_data_packet.cc',
         'gesture_detection/gesture_event_data_packet.h',
-        'gesture_detection/gesture_config_helper.h',
-        'gesture_detection/gesture_config_helper_aura.cc',
-        'gesture_detection/gesture_config_helper_android.cc',
         'gesture_detection/gesture_provider.cc',
         'gesture_detection/gesture_provider.h',
         'gesture_detection/motion_event.h',
@@ -221,11 +244,6 @@
         'gesture_detection/velocity_tracker.h',
       ],
       'conditions': [
-        ['use_aura==1', {
-          'dependencies': [
-            'events'
-          ],
-        }],
         ['use_aura!=1 and OS!="android"', {
           'sources': [
             'gesture_detection/gesture_config_helper.cc',
@@ -285,7 +303,7 @@
         'events',
         'events_base',
         'events_test_support',
-        'gesture_detection'
+        'gesture_detection',
       ],
       'sources': [
         'cocoa/events_mac_unittest.mm',
@@ -293,6 +311,7 @@
         'event_processor_unittest.cc',
         'event_rewriter_unittest.cc',
         'event_unittest.cc',
+        'gestures/motion_event_aura_unittest.cc',
         'gestures/velocity_calculator_unittest.cc',
         'gesture_detection/bitset_32_unittest.cc',
         'gesture_detection/gesture_provider_unittest.cc',
@@ -314,6 +333,12 @@
           'dependencies': [
             'ozone/events_ozone.gyp:events_ozone',
             'ozone/events_ozone.gyp:events_ozone_evdev',
+          ]
+        }],
+        ['use_aura==0', {
+          'sources!': [
+            'gestures/motion_event_aura_unittest.cc',
+            'gestures/velocity_calculator_unittest.cc',
           ],
         }],
         ['OS=="linux" and use_allocator!="none"', {

[tdresser, 2014-05-15 21:24:17]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-15 21:24:45]

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

[commit-bot: I haz the power, 2014-05-16 00:34:58]

Change committed as 270874