Aura on Android: Making events work

Aura on Android: Making events work

- Using Ozone's event_utils implementation. If we ever set NativeEvent to be
MotionEvent with Aura on Android, we'll need to change this, but this seems to
work okay for now.
- Allow BEGIN/END gesture events to be dispatched. Views needs them to decide
when to retarget gesture events.

BUG=507792
Committed: https://crrev.com/0c918277fcacfa99e392674b55efcef3a707b474
Cr-Commit-Position: refs/heads/master@{#356862}

[mfomitchev, 2015-10-14 21:53:08]

mfomitchev@chromium.org changed reviewers:
+ sadrul@chromium.org, sievers@chromium.org

[sadrul, 2015-10-15 19:54:22]

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn#newcode171
ui/events/BUILD.gn:171: sources += [ "ozone/events_ozone.cc" ]
Hm. This is unfortunate.

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_configuration_android.cc:44: #endif
Or would it make sense to actually use GestureConfigurationAura on android-aura?

[mfomitchev, 2015-10-15 20:06:29]

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn#newcode171
ui/events/BUILD.gn:171: sources += [ "ozone/events_ozone.cc" ]
On 2015/10/15 19:54:21, sadrul wrote:
> Hm. This is unfortunate.

Are you just unhappy about name/location? We could call it something like
events_default.cc and move it out of ozone/..

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_configuration_android.cc:44: #endif
On 2015/10/15 19:54:22, sadrul wrote:
> Or would it make sense to actually use GestureConfigurationAura on
android-aura?

The Android implementation gets a bunch of settings from ViewConfiguration. It
ends up plugging directly into the native Android ViewConfiguration class. So if
we use the Aura implementation here, Chrome's feel will be different than that
of the native apps, which won't be good.

[jdduke (slow), 2015-10-15 20:50:50]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-10-15 20:50:51]

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_configuration_android.cc:44: #endif
On 2015/10/15 20:06:29, mfomitchev wrote:
> On 2015/10/15 19:54:22, sadrul wrote:
> > Or would it make sense to actually use GestureConfigurationAura on
> android-aura?
> 
> The Android implementation gets a bunch of settings from ViewConfiguration. It
> ends up plugging directly into the native Android ViewConfiguration class. So
if
> we use the Aura implementation here, Chrome's feel will be different than that
> of the native apps, which won't be good. No strong opinion either way though.

Sorry for the drive-by, but yeah, when we fashioned these classes we never
envisioned a world where Android/Aura intersected =/. Never say never I guess.
I'm fine with this addition, I suppose you could also just set it somewhere
early-on (during the late stages of startup?) in some Aura-specific code?

[mfomitchev, 2015-10-15 21:42:05]

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_configuration_android.cc:44: #endif
On 2015/10/15 20:50:51, jdduke wrote:
> On 2015/10/15 20:06:29, mfomitchev wrote:
> > On 2015/10/15 19:54:22, sadrul wrote:
> > > Or would it make sense to actually use GestureConfigurationAura on
> > android-aura?
> > 
> > The Android implementation gets a bunch of settings from ViewConfiguration.
It
> > ends up plugging directly into the native Android ViewConfiguration class.
So
> if
> > we use the Aura implementation here, Chrome's feel will be different than
that
> > of the native apps, which won't be good. No strong opinion either way
though.
> 
> Sorry for the drive-by, but yeah, when we fashioned these classes we never
> envisioned a world where Android/Aura intersected =/. Never say never I guess.
> I'm fine with this addition, I suppose you could also just set it somewhere
> early-on (during the late stages of startup?) in some Aura-specific code?
> 

We could, sure. Isn't it preferable though to have all the configuration in one
place so that we don't have to search through the code to understand the default
behavior? Is the concern simply that this pollutes the non-Aura Android logic?
If we get a lot of these ifdefs, we could create a
gesture_configuration_auraandroid.cc, but currently it doesn't seem too bad.

[no sievers, 2015-10-15 22:11:54]

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn#newcode171
ui/events/BUILD.gn:171: sources += [ "ozone/events_ozone.cc" ]
On 2015/10/15 20:06:29, mfomitchev wrote:
> On 2015/10/15 19:54:21, sadrul wrote:
> > Hm. This is unfortunate.
> 
> Are you just unhappy about name/location? We could call it something like
> events_default.cc and move it out of ozone/..

Yes that'd be nicer. Having it named '_ozone' I would expect it to not get
compiled if use_ozone == 0.

Also we don't need DispatchEventFromNativeUiEvent() and events_ozone.h, right?
Maybe that should also be split up.

[mfomitchev, 2015-10-22 15:01:17]

Please take another look

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1405773004/diff/1/ui/events/BUILD.gn#newcode171
ui/events/BUILD.gn:171: sources += [ "ozone/events_ozone.cc" ]
On 2015/10/15 22:11:54, sievers wrote:
> On 2015/10/15 20:06:29, mfomitchev wrote:
> > On 2015/10/15 19:54:21, sadrul wrote:
> > > Hm. This is unfortunate.
> > 
> > Are you just unhappy about name/location? We could call it something like
> > events_default.cc and move it out of ozone/..
> 
> Yes that'd be nicer. Having it named '_ozone' I would expect it to not get
> compiled if use_ozone == 0.
> 
> Also we don't need DispatchEventFromNativeUiEvent() and events_ozone.h, right?
> Maybe that should also be split up.

Done.

[no sievers, 2015-10-22 19:08:27]

nice, lgtm with nit comments

https://codereview.chromium.org/1405773004/diff/40001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1405773004/diff/40001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:179: if (is_win || is_mac || use_x11 || use_ozone ||
(is_android && use_aura)) {
nit: now that this has grown very long, should we add it conditionally instead?

https://codereview.chromium.org/1405773004/diff/40001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/1405773004/diff/40001/ui/events/events.gyp#ne...
ui/events/events.gyp:180: ['OS=="win" or OS=="mac" or use_x11==1 or use_ozone==1
or (OS=="android" and use_aura==1)', {
see comment in GN

https://codereview.chromium.org/1405773004/diff/40001/ui/events/events_defaul...
File ui/events/events_default.cc (right):

https://codereview.chromium.org/1405773004/diff/40001/ui/events/events_defaul...
ui/events/events_default.cc:165: int GetModifiersFromKeyState() {
nit: Looks like this is only used on windows (and not even implemented  in
events_stub.cc or events_x.cc).

You could consider ifdef'ing it in event_utils.h so we don't have the
declaration without definition.

There might be other functions that are not consistently defined, such as
UpdateDeviceList() I think.

[mfomitchev, 2015-10-22 20:34:36]

https://codereview.chromium.org/1405773004/diff/40001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1405773004/diff/40001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:179: if (is_win || is_mac || use_x11 || use_ozone ||
(is_android && use_aura)) {
On 2015/10/22 19:08:27, sievers wrote:
> nit: now that this has grown very long, should we add it conditionally
instead?

I don't have a particularly strong opinion, but I think there's some merit to
keeping it the way it is: say a new platform is added in the future. If we keep
this the way it is - events_stub.cc will be included, with no additional
changes, which is what we want by default. If we change this to add
events_stub.cc conditionally (based on a set of current platforms), we'd have to
modify this every time a new platform is added.

https://codereview.chromium.org/1405773004/diff/40001/ui/events/events_defaul...
File ui/events/events_default.cc (right):

https://codereview.chromium.org/1405773004/diff/40001/ui/events/events_defaul...
ui/events/events_default.cc:165: int GetModifiersFromKeyState() {
On 2015/10/22 19:08:27, sievers wrote:
> nit: Looks like this is only used on windows (and not even implemented  in
> events_stub.cc or events_x.cc).
> 
> You could consider ifdef'ing it in event_utils.h so we don't have the
> declaration without definition.
> 
> There might be other functions that are not consistently defined, such as
> UpdateDeviceList() I think.

Got rid of GetModifiersFromKeyState - it was already ifdef'd.
UpdateDeviceList seems to only be implemented on X, but that cleanup should
probably be done in a separate CL.

[sadrul, 2015-10-23 07:26:02]

lgtm

[mfomitchev, 2015-10-29 15:23:30]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2015-10-29 15:23:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1405773004/#ps60001
(title: "Getting rid of GetModifiersFromKeyState in events_default.cc")

[commit-bot: I haz the power, 2015-10-29 15:23:47]

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

[commit-bot: I haz the power, 2015-10-29 16:19:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-29 16:20:18]

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