Aura on Android: UnhandledKeyboardEventHandler::HandleNativeKeyboardEvent implementaion.

Aura on Android: UnhandledKeyboardEventHandler::HandleNativeKeyboardEvent implementaion.

Using Linux implementation of UnhandledKeyboardEventHandler::HandleNativeKeyboardEvent on Android Aura.

BUG=507792
Committed: https://crrev.com/437e3af44c12c064569174f64b45cdf282d1abe0
Cr-Commit-Position: refs/heads/master@{#355448}

[mfomitchev, 2015-10-15 18:14:21]

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

[mfomitchev, 2015-10-15 18:14:21]

sky, can you PTAL? I am not sure if it is okay to just use _linux.cc impl on
Aura Android or I should rename it to something like _default.cc?

[mfomitchev, 2015-10-15 18:14:43]

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

[sky, 2015-10-15 21:07:36]

https://codereview.chromium.org/1403283002/diff/1/ui/views/controls/webview/B...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/1/ui/views/controls/webview/B...
ui/views/controls/webview/BUILD.gn:43: if (is_android && use_aura) {
Do you need the use_aura switch? When would android attempt to compile this if
use_aura was false?

[mfomitchev, 2015-10-15 21:35:14]

https://codereview.chromium.org/1403283002/diff/1/ui/views/controls/webview/B...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/1/ui/views/controls/webview/B...
ui/views/controls/webview/BUILD.gn:43: if (is_android && use_aura) {
On 2015/10/15 21:07:36, sky wrote:
> Do you need the use_aura switch? When would android attempt to compile this if
> use_aura was false?

Good point. Got rid of it. Are you okay with us using
unhandled_keyboard_event_handler_linux.cc for Android, or is it better to rename
it to something like unhandled_keyboard_event_handler_default.cc?

[no sievers, 2015-10-15 22:14:43]

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/BUILD.gn:44: sources += [
"unhandled_keyboard_event_handler_linux.cc" ]
Mind just duplicating the file since it's only one line?

Then it can also just go above with the others, not needing the if-check.

[mfomitchev, 2015-10-16 17:27:58]

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/BUILD.gn:44: sources += [
"unhandled_keyboard_event_handler_linux.cc" ]
On 2015/10/15 22:14:43, sievers wrote:
> Mind just duplicating the file since it's only one line?
> 
> Then it can also just go above with the others, not needing the if-check.

I'd rather not. IMHO a little bit of build config code > a little bit of code
duplication. If the code needs to change, we'll only need to change it in one
place, and we eliminate the possibility of forgetting to update it in one of the
places.

[no sievers, 2015-10-16 18:01:26]

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/BUILD.gn:44: sources += [
"unhandled_keyboard_event_handler_linux.cc" ]
On 2015/10/16 17:27:58, mfomitchev wrote:
> On 2015/10/15 22:14:43, sievers wrote:
> > Mind just duplicating the file since it's only one line?
> > 
> > Then it can also just go above with the others, not needing the if-check.
> 
> I'd rather not. IMHO a little bit of build config code > a little bit of code
> duplication. If the code needs to change, we'll only need to change it in one
> place, and we eliminate the possibility of forgetting to update it in one of
the
> places.

Sharing is fine, but then it shouldn't be called '_linux.cc/.h'.
Once we start doing that (same for _ozone.cc/.h) things will get really
confusing. Not sure what a good name would be though.

[mfomitchev, 2015-10-16 20:22:58]

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/BUILD.gn:44: sources += [
"unhandled_keyboard_event_handler_linux.cc" ]
Yeah, I brought that up as well. I'd be happy to change the name to something
like unhandled_keyboard_event_handler_default.cc (the _linux implementation does
make some sense as a default implementation IMO). @sky, WDYT?

[sky, 2015-10-16 20:36:44]

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/BUILD.gn:44: sources += [
"unhandled_keyboard_event_handler_linux.cc" ]
On 2015/10/16 20:22:57, mfomitchev wrote:
> Yeah, I brought that up as well. I'd be happy to change the name to something
> like unhandled_keyboard_event_handler_default.cc (the _linux implementation
does
> make some sense as a default implementation IMO). @sky, WDYT?

I'm ok with renaming to default.

[mfomitchev, 2015-10-20 20:52:03]

Please take another look.

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/BUILD.gn (right):

https://codereview.chromium.org/1403283002/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/BUILD.gn:44: sources += [
"unhandled_keyboard_event_handler_linux.cc" ]
On 2015/10/16 20:36:44, sky wrote:
> On 2015/10/16 20:22:57, mfomitchev wrote:
> > Yeah, I brought that up as well. I'd be happy to change the name to
something
> > like unhandled_keyboard_event_handler_default.cc (the _linux implementation
> does
> > make some sense as a default implementation IMO). @sky, WDYT?
> 
> I'm ok with renaming to default.

Done.

[sky, 2015-10-21 16:17:00]

LGTM

[mfomitchev, 2015-10-21 22:44:20]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2015-10-21 22:45:07]

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

[commit-bot: I haz the power, 2015-10-22 00:18:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-22 00:19:14]

Patchset 3 (id:??) landed as
https://crrev.com/437e3af44c12c064569174f64b45cdf282d1abe0
Cr-Commit-Position: refs/heads/master@{#355448}