API proposal for chrome.app.window to intercept all keys.

API proposal for chrome.app.window to intercept all keys.

This API allows application like virtual desktop to obtain
all keys (including system keys like alt-tab). The
application will need "app.window.allKeys" for the API to
take effect. This API might not be supported in all platforms
(and it is not clear if it can be supported when the
application is not maximized in all platform either).

API proposal: http://goo.gl/O2Zzgh

BUG=166928
Committed: https://crrev.com/c811bb98b489b67a5f85ed79d7acd288533a080d
Cr-Commit-Position: refs/heads/master@{#306219}

[Jamie, 2014-05-23 22:13:10]

https://codereview.chromium.org/297123002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/app_window.idl:351: // Set whether the window
should get all keyboard events including system keys that are usually
Chromium style is 80-character limit for most languages.

https://codereview.chromium.org/297123002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/app_window.idl:357: static boolean
getRequestAllKeys();
I don't think the getter is necessary (setAlwaysOnTop doesn't have one, for
example). If there's a valid use-case for knowing whether or not you're going to
receive all keyboard events, then an event is probably a better solution, since
the state can change asynchronously: It becomes false if another window gains
focus and it becomes true when the user gives the app permission to activate the
feature for the first time.

On the whole, though, I don't think being able to query this is very useful.

[Sriram, 2014-05-23 22:20:06]

https://codereview.chromium.org/297123002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/app_window.idl:351: // Set whether the window
should get all keyboard events including system keys that are usually
On 2014/05/23 22:13:10, Jamie wrote:
> Chromium style is 80-character limit for most languages.

Done.

https://codereview.chromium.org/297123002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/app_window.idl:357: static boolean
getRequestAllKeys();
On 2014/05/23 22:13:10, Jamie wrote:
> I don't think the getter is necessary (setAlwaysOnTop doesn't have one, for
> example). If there's a valid use-case for knowing whether or not you're going
to
> receive all keyboard events, then an event is probably a better solution,
since
> the state can change asynchronously: It becomes false if another window gains
> focus and it becomes true when the user gives the app permission to activate
the
> feature for the first time.
> 
> On the whole, though, I don't think being able to query this is very useful.

Done.

[benwells, 2014-05-27 22:29:48]

https://codereview.chromium.org/297123002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:353: // windows is in fullscreen.
Requires the <code>"requestAllKeys"</code>
The window permissions have changed to be app.window.alwaysOnTop (or something
like that), so you should use app.window.allKeys or something similar.

(the doco for alwaysOnTop and the esc overriding is showing the old format, now
that m35 is stable I'll update it soon).

[Sriram, 2014-05-27 22:48:05]

https://codereview.chromium.org/297123002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:353: // windows is in fullscreen.
Requires the <code>"requestAllKeys"</code>
yes, the permission added was "app.window.requestAllKeys" but I have changed it
to "app.window.allKeys" and also fixed the comments.

On 2014/05/27 22:29:48, benwells wrote:
> The window permissions have changed to be app.window.alwaysOnTop (or something
> like that), so you should use app.window.allKeys or something similar.
> 
> (the doco for alwaysOnTop and the esc overriding is showing the old format,
now
> that m35 is stable I'll update it soon).

[Jamie, 2014-06-25 16:46:58]

https://codereview.chromium.org/297123002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:449: [nocompile] static void
onWantAllKeysSettingLost();
As discussed off-line, I think one event and a getter would be cleaner than two
events. I also don't really like these names. How about something like
onAllKeysInterceptChanged? You might also want to change setWantAllKeys to
setInterceptAllKeps for symmetry (though I have no problem with the current name
for that).

[Sriram, 2014-06-25 16:57:24]

On 2014/06/25 16:46:58, Jamie wrote:
>
https://codereview.chromium.org/297123002/diff/100001/chrome/common/extension...
> File chrome/common/extensions/api/app_window.idl (right):
> 
>
https://codereview.chromium.org/297123002/diff/100001/chrome/common/extension...
> chrome/common/extensions/api/app_window.idl:449: [nocompile] static void
> onWantAllKeysSettingLost();
> As discussed off-line, I think one event and a getter would be cleaner than
two
> events. I also don't really like these names. How about something like
> onAllKeysInterceptChanged? You might also want to change setWantAllKeys to
> setInterceptAllKeps for symmetry (though I have no problem with the current
name
> for that).

Done.

[Jamie, 2014-06-27 17:32:20]

LGTM with nits.

https://codereview.chromium.org/297123002/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:356: // windows is in fullscreen.
Requires the <code>"app.window.allKeys"</code>
Is the full-screen comment correct? Also, please add that this is best-effort,
and may be subject to platform-specific constraints.

https://codereview.chromium.org/297123002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:451: [nocompile] static void
onWantAllKeysInterceptChanged();
Nit: The other methods don't mention "Want" any more. I think this would be
better (and shorter!) as onAllKeysInterceptChanged.

[Sriram, 2014-06-27 17:39:40]

https://codereview.chromium.org/297123002/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:356: // windows is in fullscreen.
Requires the <code>"app.window.allKeys"</code>
On 2014/06/27 17:32:20, Jamie wrote:
> Is the full-screen comment correct? Also, please add that this is best-effort,
> and may be subject to platform-specific constraints.

Done.

https://codereview.chromium.org/297123002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:451: [nocompile] static void
onWantAllKeysInterceptChanged();
On 2014/06/27 17:32:20, Jamie wrote:
> Nit: The other methods don't mention "Want" any more. I think this would be
> better (and shorter!) as onAllKeysInterceptChanged.

Done.

[Sriram, 2014-06-27 17:41:59]

The CQ bit was checked by sriramsr@chromium.org

[commit-bot: I haz the power, 2014-06-27 17:43:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sriramsr@chromium.org/297123002/140001

[Sriram, 2014-06-27 18:44:16]

The CQ bit was unchecked by sriramsr@chromium.org

[Sriram, 2014-06-27 18:44:36]

The CQ bit was checked by sriramsr@chromium.org

[Sriram, 2014-06-27 18:44:37]

The CQ bit was unchecked by sriramsr@chromium.org

[Sriram, 2014-06-27 18:46:25]

Ben - I need owners approval from you as well. Can you please take a look? 

Also OWNERS for extenstions/api/ does not include OWNERS for extenstions. Do you
know who of the following would be a good person to look at this?

awatson@chromium.org
mkearney@chromium.org
mkwst@chromium.org

[benwells, 2014-06-30 01:00:46]

On 2014/06/27 18:46:25, sriramsr wrote:
> Ben - I need owners approval from you as well. Can you please take a look? 
> 
> Also OWNERS for extenstions/api/ does not include OWNERS for extenstions. Do
you
> know who of the following would be a good person to look at this?
> 
> mailto:awatson@chromium.org
> mailto:mkearney@chromium.org
> mailto:mkwst@chromium.org

Apologies, I had a comment that I forgot to email out (the same one as Jamie
about fullscreen).

You can't commit just the idl like this. You need to implement it as well.

[benwells, 2014-06-30 01:03:07]

On 2014/06/30 01:00:46, benwells wrote:
> On 2014/06/27 18:46:25, sriramsr wrote:
> > Ben - I need owners approval from you as well. Can you please take a look? 
> > 
> > Also OWNERS for extenstions/api/ does not include OWNERS for extenstions. Do
> you
> > know who of the following would be a good person to look at this?
> > 
> > mailto:awatson@chromium.org
> > mailto:mkearney@chromium.org
> > mailto:mkwst@chromium.org
> 
> Apologies, I had a comment that I forgot to email out (the same one as Jamie
> about fullscreen).
> 
> You can't commit just the idl like this. You need to implement it as well.

BTW there isn't a noparent in chrome/common/extensions/api/OWNERS, so the owners
in the folder up can review. The specific api/OWNERS file is just adding extra
owners for some specific things.

[Sriram, 2014-07-02 23:41:27]

On 2014/06/30 01:03:07, benwells wrote:
> On 2014/06/30 01:00:46, benwells wrote:
> > On 2014/06/27 18:46:25, sriramsr wrote:
> > > Ben - I need owners approval from you as well. Can you please take a look?

> > > 
> > > Also OWNERS for extenstions/api/ does not include OWNERS for extenstions.
Do
> > you
> > > know who of the following would be a good person to look at this?
> > > 
> > > mailto:awatson@chromium.org
> > > mailto:mkearney@chromium.org
> > > mailto:mkwst@chromium.org
> > 
> > Apologies, I had a comment that I forgot to email out (the same one as Jamie
> > about fullscreen).
> > 
> > You can't commit just the idl like this. You need to implement it as well.
> 
> BTW there isn't a noparent in chrome/common/extensions/api/OWNERS, so the
owners
> in the folder up can review. The specific api/OWNERS file is just adding extra
> owners for some specific things.

Any idea why "ninja -C out/Debug chrome" would succeed without the API being
implemented? Do you have any pointer on how the idl generated files get pulled
into product? Thanks!

[benwells, 2014-07-03 06:36:14]

On 2014/07/02 23:41:27, sriramsr wrote:
> On 2014/06/30 01:03:07, benwells wrote:
> > On 2014/06/30 01:00:46, benwells wrote:
> > > On 2014/06/27 18:46:25, sriramsr wrote:
> > > > Ben - I need owners approval from you as well. Can you please take a
look?
> 
> > > > 
> > > > Also OWNERS for extenstions/api/ does not include OWNERS for
extenstions.
> Do
> > > you
> > > > know who of the following would be a good person to look at this?
> > > > 
> > > > mailto:awatson@chromium.org
> > > > mailto:mkearney@chromium.org
> > > > mailto:mkwst@chromium.org
> > > 
> > > Apologies, I had a comment that I forgot to email out (the same one as
Jamie
> > > about fullscreen).
> > > 
> > > You can't commit just the idl like this. You need to implement it as well.
> > 
> > BTW there isn't a noparent in chrome/common/extensions/api/OWNERS, so the
> owners
> > in the folder up can review. The specific api/OWNERS file is just adding
extra
> > owners for some specific things.
> 
> Any idea why "ninja -C out/Debug chrome" would succeed without the API being
> implemented? Do you have any pointer on how the idl generated files get pulled
> into product? Thanks!

Regardless of compilation or tests we don't commit idl without implementation
that I am aware of.

Having unimplemented IDL should compile, and mostly tests will pass, but in this
case I think it will break apps using the window api (so tests will fail in this
case).

IDL is translated into generated c structs when you run the build/gyp_chromium
step. The idl is also linked into the product and used from the extensions
system javascript.

[Sriram, 2014-07-08 22:03:47]

On 2014/07/03 06:36:14, benwells wrote:
> On 2014/07/02 23:41:27, sriramsr wrote:
> > On 2014/06/30 01:03:07, benwells wrote:
> > > On 2014/06/30 01:00:46, benwells wrote:
> > > > On 2014/06/27 18:46:25, sriramsr wrote:
> > > > > Ben - I need owners approval from you as well. Can you please take a
> look?
> > 
> > > > > 
> > > > > Also OWNERS for extenstions/api/ does not include OWNERS for
> extenstions.
> > Do
> > > > you
> > > > > know who of the following would be a good person to look at this?
> > > > > 
> > > > > mailto:awatson@chromium.org
> > > > > mailto:mkearney@chromium.org
> > > > > mailto:mkwst@chromium.org
> > > > 
> > > > Apologies, I had a comment that I forgot to email out (the same one as
> Jamie
> > > > about fullscreen).
> > > > 
> > > > You can't commit just the idl like this. You need to implement it as
well.
> > > 
> > > BTW there isn't a noparent in chrome/common/extensions/api/OWNERS, so the
> > owners
> > > in the folder up can review. The specific api/OWNERS file is just adding
> extra
> > > owners for some specific things.
> > 
> > Any idea why "ninja -C out/Debug chrome" would succeed without the API being
> > implemented? Do you have any pointer on how the idl generated files get
pulled
> > into product? Thanks!
> 
> Regardless of compilation or tests we don't commit idl without implementation
> that I am aware of.
> 
> Having unimplemented IDL should compile, and mostly tests will pass, but in
this
> case I think it will break apps using the window api (so tests will fail in
this
> case).
> 
> IDL is translated into generated c structs when you run the build/gyp_chromium
> step. The idl is also linked into the product and used from the extensions
> system javascript.

Thanks! Added sub-methods in the interfaces and plumbed things through. I have
also submitted to trybot after running subset of the tests locally. PTAL.

[benwells, 2014-07-16 09:39:14]

Sorry about the latency.

This is going in the right direction. What it needs now is:
- some tests (at least that the functionality can't be used in beta and stable,
but ideally a functional test as well)
- to implement the feature on at least one platform.

I didn't look at the aura / widget stuff.

https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc#newc...
apps/app_window.cc:919:
native_app_window_->SetInterceptAllKeys(should_intercept_all_keys);
This is going to be called a lot - e.g. when the window is resized. Should it
just be called when it needs to be set?

https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
chrome/app/generated_resources.grd:4469: +          Intercept all keyboard
events including task switcher key like ALT+TAB
Could you file a bug to get reviewed strings from security?

https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
Since the permission is only available in dev channel, is this check needed?

[Alexei Svitkine (slow), 2014-07-17 13:35:53]

histograms lgtm

[Sriram, 2014-07-18 21:33:24]

Thanks again for the review. I wanted to keep the implementation in following CL
as this one is already little big plumbing the API through. I will send out
another one for windows implementation.

Can you clarify as what you mean by test to validate that this functionality is
not in beta/stable? I the windows implementation includes functional tests as
well. 

On 2014/07/16 09:39:14, benwells wrote:
> Sorry about the latency.
> 
> This is going in the right direction. What it needs now is:
> - some tests (at least that the functionality can't be used in beta and
stable,
> but ideally a functional test as well)
> - to implement the feature on at least one platform.
> 
> I didn't look at the aura / widget stuff.
> 
> https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc
> File apps/app_window.cc (right):
> 
>
https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc#newc...
> apps/app_window.cc:919:
> native_app_window_->SetInterceptAllKeys(should_intercept_all_keys);
> This is going to be called a lot - e.g. when the window is resized. Should it
> just be called when it needs to be set?
> 
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:4469: +          Intercept all keyboard
> events including task switcher key like ALT+TAB
> Could you file a bug to get reviewed strings from security?
> 
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> File
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
> (right):
> 
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
> if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
> Since the permission is only available in dev channel, is this check needed?

[benwells, 2014-07-22 01:54:31]

On 2014/07/18 21:33:24, Sriram wrote:
> Thanks again for the review. I wanted to keep the implementation in following
CL
> as this one is already little big plumbing the API through. I will send out
> another one for windows implementation.

OK. Just to be clear, this is now implemented for chromeOS. Is that correct?

> 
> Can you clarify as what you mean by test to validate that this functionality
is
> not in beta/stable? I the windows implementation includes functional tests as
> well. 

You can use a ScopedCurrentChannel to simulate not being in dev / canary /
trunk. So you have a test which uses this to pretend to be in beta, and then
check that loading an app with this permission fails.

For functional test, I can see two levels:
- test that your plumbing is being set up as much as possible. E.g.:
  - create a window. check it doesn't have intercept
  - request intercept. now check that it does
  - give up interception and make sure that it now doesn't have it.
- test that the interception actually works. This would be better done as a unit
test of the actual widget changes your making, as opposed to an API test. BTW I
am not a good reviewer for that stuff, you should find an OWNER that can review
it and add a test. If you want to do that in a separate CL (which you seemed to
indicate above) that is fine.

> 
> On 2014/07/16 09:39:14, benwells wrote:
> > Sorry about the latency.
> > 
> > This is going in the right direction. What it needs now is:
> > - some tests (at least that the functionality can't be used in beta and
> stable,
> > but ideally a functional test as well)
> > - to implement the feature on at least one platform.
> > 
> > I didn't look at the aura / widget stuff.
> > 
> > https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc
> > File apps/app_window.cc (right):
> > 
> >
>
https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc#newc...
> > apps/app_window.cc:919:
> > native_app_window_->SetInterceptAllKeys(should_intercept_all_keys);
> > This is going to be called a lot - e.g. when the window is resized. Should
it
> > just be called when it needs to be set?
> > 
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > File chrome/app/generated_resources.grd (right):
> > 
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > chrome/app/generated_resources.grd:4469: +          Intercept all keyboard
> > events including task switcher key like ALT+TAB
> > Could you file a bug to get reviewed strings from security?
> > 
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> > File
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
> > if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
> > Since the permission is only available in dev channel, is this check needed?

[benwells, 2014-07-22 01:55:41]

On 2014/07/22 01:54:31, benwells wrote:
> On 2014/07/18 21:33:24, Sriram wrote:
> > Thanks again for the review. I wanted to keep the implementation in
following
> CL
> > as this one is already little big plumbing the API through. I will send out
> > another one for windows implementation.
> 
> OK. Just to be clear, this is now implemented for chromeOS. Is that correct?
> 
> > 
> > Can you clarify as what you mean by test to validate that this functionality
> is
> > not in beta/stable? I the windows implementation includes functional tests
as
> > well. 
> 
> You can use a ScopedCurrentChannel to simulate not being in dev / canary /
> trunk. So you have a test which uses this to pretend to be in beta, and then
> check that loading an app with this permission fails.
> 
> For functional test, I can see two levels:
> - test that your plumbing is being set up as much as possible. E.g.:
>   - create a window. check it doesn't have intercept
>   - request intercept. now check that it does
>   - give up interception and make sure that it now doesn't have it.
> - test that the interception actually works. This would be better done as a
unit
> test of the actual widget changes your making, as opposed to an API test. BTW
I
> am not a good reviewer for that stuff, you should find an OWNER that can
review
> it and add a test. If you want to do that in a separate CL (which you seemed
to
> indicate above) that is fine.
> 
> > 
> > On 2014/07/16 09:39:14, benwells wrote:
> > > Sorry about the latency.
> > > 
> > > This is going in the right direction. What it needs now is:
> > > - some tests (at least that the functionality can't be used in beta and
> > stable,
> > > but ideally a functional test as well)
> > > - to implement the feature on at least one platform.
> > > 
> > > I didn't look at the aura / widget stuff.
> > > 
> > > https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc
> > > File apps/app_window.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc#newc...
> > > apps/app_window.cc:919:
> > > native_app_window_->SetInterceptAllKeys(should_intercept_all_keys);
> > > This is going to be called a lot - e.g. when the window is resized. Should
> it
> > > just be called when it needs to be set?
> > > 
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > > File chrome/app/generated_resources.grd (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > > chrome/app/generated_resources.grd:4469: +          Intercept all keyboard
> > > events including task switcher key like ALT+TAB
> > > Could you file a bug to get reviewed strings from security?
> > > 
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> > > File
> > >
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> > >
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
> > > if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
> > > Since the permission is only available in dev channel, is this check
needed?

BTW when responding to reviews, it is really helpful if you respond to the
comments the reviewer makes with the commenting tool. E.g. if you do something
suggested, click the Done button. If you don't, explain why in the comment.

[Sriram, 2014-07-24 22:38:44]

On 2014/07/22 01:55:41, benwells wrote:
> On 2014/07/22 01:54:31, benwells wrote:
> > On 2014/07/18 21:33:24, Sriram wrote:
> > > Thanks again for the review. I wanted to keep the implementation in
> following
> > CL
> > > as this one is already little big plumbing the API through. I will send
out
> > > another one for windows implementation.
> > 
> > OK. Just to be clear, this is now implemented for chromeOS. Is that correct?
> > 
> > > 
> > > Can you clarify as what you mean by test to validate that this
functionality
> > is
> > > not in beta/stable? I the windows implementation includes functional tests
> as
> > > well. 
> > 
> > You can use a ScopedCurrentChannel to simulate not being in dev / canary /
> > trunk. So you have a test which uses this to pretend to be in beta, and then
> > check that loading an app with this permission fails.
> > 
> > For functional test, I can see two levels:
> > - test that your plumbing is being set up as much as possible. E.g.:
> >   - create a window. check it doesn't have intercept
> >   - request intercept. now check that it does
> >   - give up interception and make sure that it now doesn't have it.
> > - test that the interception actually works. This would be better done as a
> unit
> > test of the actual widget changes your making, as opposed to an API test.
BTW
> I
> > am not a good reviewer for that stuff, you should find an OWNER that can
> review
> > it and add a test. If you want to do that in a separate CL (which you seemed
> to
> > indicate above) that is fine.
> > 
> > > 
> > > On 2014/07/16 09:39:14, benwells wrote:
> > > > Sorry about the latency.
> > > > 
> > > > This is going in the right direction. What it needs now is:
> > > > - some tests (at least that the functionality can't be used in beta and
> > > stable,
> > > > but ideally a functional test as well)
> > > > - to implement the feature on at least one platform.
> > > > 
> > > > I didn't look at the aura / widget stuff.
> > > > 
> > > > https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc
> > > > File apps/app_window.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc#newc...
> > > > apps/app_window.cc:919:
> > > > native_app_window_->SetInterceptAllKeys(should_intercept_all_keys);
> > > > This is going to be called a lot - e.g. when the window is resized.
Should
> > it
> > > > just be called when it needs to be set?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > > > File chrome/app/generated_resources.grd (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > > > chrome/app/generated_resources.grd:4469: +          Intercept all
keyboard
> > > > events including task switcher key like ALT+TAB
> > > > Could you file a bug to get reviewed strings from security?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> > > > File
> > > >
> > >
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> > > >
> > >
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
> > > > if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
> > > > Since the permission is only available in dev channel, is this check
> needed?
> 
> BTW when responding to reviews, it is really helpful if you respond to the
> comments the reviewer makes with the commenting tool. E.g. if you do something
> suggested, click the Done button. If you don't, explain why in the comment.

Will do. Did not realize that send message in here does not publish saved
drafts/responses. Done.

[Sriram, 2014-07-24 22:38:54]

https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc#newc...
apps/app_window.cc:919:
native_app_window_->SetInterceptAllKeys(should_intercept_all_keys);
On 2014/07/16 09:39:14, benwells wrote:
> This is going to be called a lot - e.g. when the window is resized. Should it
> just be called when it needs to be set?

Done.

https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
chrome/app/generated_resources.grd:4469: +          Intercept all keyboard
events including task switcher key like ALT+TAB
Done. Created http://crbug.com/395255. Can you please verify that I have all the
bits set to get the string reviewed? Also is this enough to get the string
reviewed or should I also mail chrome-ui-review?

On 2014/07/16 09:39:14, benwells wrote:
> Could you file a bug to get reviewed strings from security?

https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
Followed pattern used with other API cls and also wanted to be prevent against
accidental merge.

On 2014/07/16 09:39:14, benwells wrote:
> Since the permission is only available in dev channel, is this check needed?

[Sriram, 2014-07-24 22:49:06]

On 2014/07/22 01:55:41, benwells wrote:
> On 2014/07/22 01:54:31, benwells wrote:
> > On 2014/07/18 21:33:24, Sriram wrote:
> > > Thanks again for the review. I wanted to keep the implementation in
> following
> > CL
> > > as this one is already little big plumbing the API through. I will send
out
> > > another one for windows implementation.
> > 
> > OK. Just to be clear, this is now implemented for chromeOS. Is that correct?

No, still just windows. I would have to expose API in ChromeOS to do this which
I will look into after finishing up with the UI alert. 


> > 
> > > 
> > > Can you clarify as what you mean by test to validate that this
functionality
> > is
> > > not in beta/stable? I the windows implementation includes functional tests
> as
> > > well. 
> > 
> > You can use a ScopedCurrentChannel to simulate not being in dev / canary /
> > trunk. So you have a test which uses this to pretend to be in beta, and then
> > check that loading an app with this permission fails.
> > 
> > For functional test, I can see two levels:
> > - test that your plumbing is being set up as much as possible. E.g.:
> >   - create a window. check it doesn't have intercept
> >   - request intercept. now check that it does
> >   - give up interception and make sure that it now doesn't have it.
> > - test that the interception actually works. This would be better done as a
> unit
> > test of the actual widget changes your making, as opposed to an API test.
BTW
> I
> > am not a good reviewer for that stuff, you should find an OWNER that can
> review
> > it and add a test. If you want to do that in a separate CL (which you seemed
> to
> > indicate above) that is fine.

On hindsight, I will pull in the windows implementation with this change and
bring in all the tests that validates all of this. Will send update (along with
owner for missing files).


> > 
> > > 
> > > On 2014/07/16 09:39:14, benwells wrote:
> > > > Sorry about the latency.
> > > > 
> > > > This is going in the right direction. What it needs now is:
> > > > - some tests (at least that the functionality can't be used in beta and
> > > stable,
> > > > but ideally a functional test as well)
> > > > - to implement the feature on at least one platform.
> > > > 
> > > > I didn't look at the aura / widget stuff.
> > > > 
> > > > https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc
> > > > File apps/app_window.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/apps/app_window.cc#newc...
> > > > apps/app_window.cc:919:
> > > > native_app_window_->SetInterceptAllKeys(should_intercept_all_keys);
> > > > This is going to be called a lot - e.g. when the window is resized.
Should
> > it
> > > > just be called when it needs to be set?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > > > File chrome/app/generated_resources.grd (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/app/generated_re...
> > > > chrome/app/generated_resources.grd:4469: +          Intercept all
keyboard
> > > > events including task switcher key like ALT+TAB
> > > > Could you file a bug to get reviewed strings from security?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> > > > File
> > > >
> > >
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/297123002/diff/320001/chrome/browser/extensio...
> > > >
> > >
> >
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
> > > > if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
> > > > Since the permission is only available in dev channel, is this check
> needed?
> 
> BTW when responding to reviews, it is really helpful if you respond to the
> comments the reviewer makes with the commenting tool. E.g. if you do something
> suggested, click the Done button. If you don't, explain why in the comment.

[Sriram, 2014-07-29 21:34:22]

Ping

[sadrul, 2014-07-29 22:22:41]

+sky@ since there are some Windows system calls that I am not familiar with in
DesktopKeyboardCaptureWin.

What happens if there's a context menu for the app, and user presses a key? Does
the menu get the key-event, or the app?

Instead of plumbing this through views, can you have DesktopKeyboardCaptureWin
live closer to chrome_native_app_window_views (where it seems to be used)? It
can get the HWND from views::Widget -> aura::Window -> aura::WindowTreeHost ->
gfx::AcceleratedWidget, and it can also know when a Widget becomes active from a
WidgetObserver. So it should be able to live out of views.

Also, would you mind updating the CL description to have shorter line-lengths,
because the UI unfortunately breaks (the rietveld web-interface, git log, etc.)
with very long lines? Thanks!

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:19:
DesktopKeyboardCaptureWin::registrations_;
I don't think you can have non-POD globals like this.

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2014.5

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
You probably want this to be a base::Singleton object to avoid having the
non-pod global/static objects.

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:37: KBDLLHOOKSTRUCT
hook_struct);
Please add comments on what these do

[benwells, 2014-07-29 22:34:15]

Are you going to add some tests?

[Sriram, 2014-07-29 22:39:05]

On 2014/07/29 22:34:15, benwells wrote:
> Are you going to add some tests?

I added test in app_window_intercept_all_keys_uitest.cc (which covers all the
cases). Did I miss some case that you would like covered?

[sky, 2014-07-29 23:07:26]

+cpu so that he can cringe when seeing us install a hook.

I would like to see some tests in the views side.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin::KeyboardInterceptRegistration {
You don't need VIEWS_EXPORT in a .cc file.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:26: if
(hook_handle_ != NULL) {
no {}

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:35: DLOG(ERROR) <<
"Error calling SetWindowsHookEx() - GLE = "
DVLOG

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:67:
task_runner->PostTask(
Why do you PostTask here?

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:76: base::AutoLock
lock(registration_lock_);
Why are you using a lock?

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:95: if
(registrations_.contains(handle)) {
no {}

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:111: bool
DesktopKeyboardCaptureWin::IsKeyUp(WPARAM w_param) {
Move functions like this that don't need any state from
DesktopKeyboardCaptureWin into an anonymous namespace.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:191: HWND
current_active_window = GetActiveWindow();
I think we should uninstall the hook when we don't have an active window any
longer. Less errors then and less calling into chrome from other apps.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2014.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
Please add class level descriptions and in general more comments.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
I think this code would be less error prone if it was effectively a one shot. In
the constructor/Init() it installs the hook and in its destructor it releases
it.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:23: static void
Capture(HWND window_handle, base::TaskRunner* task_runner);
Why does this take a task_runner?

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:45: static
base::Lock registration_lock_;
statics should only be POD (see style guide).

[benwells, 2014-07-30 02:50:25]

This is just the test cc, I have to look at the API implementation files again.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:27: LOG(INFO) <<
"Sending: " << key << " control = " << control << " shift = "
Please remove logging here and elsewhere, LOG(INFO)'s should not be committed as
they cause log spam.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:37: LOG(INFO) <<
"Failed to send key to app";
You can use LOG(WARNING) here, but LOG(INFO) should be removed.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:50: bool
SendReply(const char* message) {
Can you explain what this is for? There is already a way to send replies to apps
via ExtensionTestMessageListener.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:57: if
(!key_listener.was_satisfied()) {
Why do you have to check this here? You have just created the listener so it
should be unsatisfied.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:67:
base::RunLoop().RunUntilIdle();
Why do you run the message loop here?

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:77: bool
WaitUntilKeyFocus() { return WaitForKeyEvent(ui::VKEY_Z); }
I'm not sure how this causes focus to be given to the app. Does
ui_controls::SendKeyPressNotifyWhenDone do this? If so it might be worth
mentioning it somewhere.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:81:
scoped_ptr<ExtensionTestMessageListener> launched_listener(
Why is this a scoped_ptr?

Oh, now I see. This code will be much simpler if you don't try and reuse the
Launched message. You should introduce a new message 'Ready' or something which
gets the instructions.

This function would become much simpler (you can just call
LoadAndLaunchPlatformApp(app_path) then ShowAndFocus the window and return, and
this function doesn't have to return a listener at all.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:109:
ui_controls::SendKeyPressNotifyWhenDone(
Can you use SimulateKeyPress here?

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:137:
ASSERT_FALSE(SimulateKeyPress(ui::VKEY_Z)) << message_;
Where exactly does this fail? Does it timeout?

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:322: "U 16 0 false
true false false"}},
Can this data be inferred before checking instead of having to list it all out?

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:323: // ctrl+f which
has aceelerator binding should also reuslt in all keys
Nit: spelling (aceelerator, reuslt).

[cpu_(ooo_6.6-7.5), 2014-07-30 23:41:00]

I guess I don't understand why we want to use the low level keyboard hook. For
example this delivers key events even if some other program is active in the
foreground.

I think this can also be accomplished with a local hook scoped to the UI thread,
something along the lines of

SetWindowsHookEx(WH_KEYBOARD, callback, NULL, GetCurrentThreadId)

[cpu_(ooo_6.6-7.5), 2014-07-31 00:14:48]

ok, I don't know of another way of getting alt-tab except via the current hook
scheme, so I guess nevermind my previous suggestion.

But Scott is right, we need to unhook when we are not active.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:33: hook_handle_ =
SetWindowsHookEx(WH_KEYBOARD_LL, callback_function, NULL, 0);
check that this is a thread with a UI style message pump.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:131: // Extended
key when the event is received as part window event and so skip it
comments need periods at the end.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:164: const int
kKeyboardStateLength = 256;
what is this function trying to do?

A full fledged comment about the general operation would be nice as well.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:172: for (int index
= 0; index < _countof(keys_to_update); index++) {
dont use _countof()

[Sriram, 2014-07-31 00:16:06]

On 2014/07/29 22:22:41, sadrul wrote:
> +sky@ since there are some Windows system calls that I am not familiar with in
> DesktopKeyboardCaptureWin.
> 
> What happens if there's a context menu for the app, and user presses a key?
Does
> the menu get the key-event, or the app?
Keyboard hooks are disabled when the window looses focus and so they should be
disabled
when context menu is opened.

> 
> Instead of plumbing this through views, can you have DesktopKeyboardCaptureWin
> live closer to chrome_native_app_window_views (where it seems to be used)? It
> can get the HWND from views::Widget -> aura::Window -> aura::WindowTreeHost ->
> gfx::AcceleratedWidget, and it can also know when a Widget becomes active from
a
> WidgetObserver. So it should be able to live out of views.
I followed pattern used for SetAlwaysOnTop and other APIs. I would have to hook
this
up with Fullscreen/PointerLock bubble in later CL (and have not checked if that
is accessible
from just view). Do you know if it is? Also from layering point of view, could
you please explain
the division of responsibility? 


> 
> Also, would you mind updating the CL description to have shorter line-lengths,
> because the UI unfortunately breaks (the rietveld web-interface, git log,
etc.)
> with very long lines? Thanks!
Done.

I have fixed these as part of Sky's CL comments but have not uploaded a patch
with these yet. Will do when I respond to his comments as well.

> 
>
https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):
> 
>
https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:19:
> DesktopKeyboardCaptureWin::registrations_;
> I don't think you can have non-POD globals like this.
> 
>
https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):
> 
>
https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:1: // Copyright
(c)
> 2012 The Chromium Authors. All rights reserved.
> 2014.5
> 
>
https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
> VIEWS_EXPORT DesktopKeyboardCaptureWin {
> You probably want this to be a base::Singleton object to avoid having the
> non-pod global/static objects.
> 
>
https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:37:
KBDLLHOOKSTRUCT
> hook_struct);
> Please add comments on what these do

[Sriram, 2014-07-31 00:21:14]

Addresses Sky's comments.

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:19:
DesktopKeyboardCaptureWin::registrations_;
On 2014/07/29 22:22:41, sadrul wrote:
> I don't think you can have non-POD globals like this.

Done.

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/07/29 22:22:41, sadrul wrote:
> 2014.5

Done.

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
On 2014/07/29 22:22:41, sadrul wrote:
> You probably want this to be a base::Singleton object to avoid having the
> non-pod global/static objects.

Done.

https://codereview.chromium.org/297123002/diff/560001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:37: KBDLLHOOKSTRUCT
hook_struct);
On 2014/07/29 22:22:41, sadrul wrote:
> Please add comments on what these do

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin::KeyboardInterceptRegistration {
On 2014/07/29 23:07:25, sky wrote:
> You don't need VIEWS_EXPORT in a .cc file.

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:26: if
(hook_handle_ != NULL) {
On 2014/07/29 23:07:25, sky wrote:
> no {}

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:35: DLOG(ERROR) <<
"Error calling SetWindowsHookEx() - GLE = "
On 2014/07/29 23:07:25, sky wrote:
> DVLOG

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:67:
task_runner->PostTask(
Hook callbacks happen in the thread that calls SetWindowsHookEx() and so posting
to the thread that is supposed to be active for the lifetime (i.e. the UI
thread).

On 2014/07/29 23:07:25, sky wrote:
> Why do you PostTask here?

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:76: base::AutoLock
lock(registration_lock_);
The registration is accessed from UI thread (as part of handling
callbacks/registration) and any thread on which Release() could get called from.
So using a lock to protect it.

On 2014/07/29 23:07:25, sky wrote:
> Why are you using a lock?

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:95: if
(registrations_.contains(handle)) {
On 2014/07/29 23:07:25, sky wrote:
> no {}

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:111: bool
DesktopKeyboardCaptureWin::IsKeyUp(WPARAM w_param) {
On 2014/07/29 23:07:25, sky wrote:
> Move functions like this that don't need any state from
> DesktopKeyboardCaptureWin into an anonymous namespace.

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:191: HWND
current_active_window = GetActiveWindow();
We do uninstall the hook as soon as the window losses focus. This is to handle
race when the keyboard event is already in flight.

On 2014/07/29 23:07:25, sky wrote:
> I think we should uninstall the hook when we don't have an active window any
> longer. Less errors then and less calling into chrome from other apps.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/07/29 23:07:26, sky wrote:
> 2014.

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
The registration should be done on a thread that would live for the lifetime of
the hook and so we would either have to create a dedicated thread (or I am using
UI thread here for this). That is the reason for it not done inline.

On 2014/07/29 23:07:26, sky wrote:
> I think this code would be less error prone if it was effectively a one shot.
In
> the constructor/Init() it installs the hook and in its destructor it releases
> it.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
On 2014/07/29 23:07:26, sky wrote:
> Please add class level descriptions and in general more comments.

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:23: static void
Capture(HWND window_handle, base::TaskRunner* task_runner);
See above.

On 2014/07/29 23:07:26, sky wrote:
> Why does this take a task_runner?

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:45: static
base::Lock registration_lock_;
On 2014/07/29 23:07:26, sky wrote:
> statics should only be POD (see style guide).

Done.

[Sriram, 2014-07-31 00:35:33]

On 2014/07/30 23:41:00, cpu wrote:
> I guess I don't understand why we want to use the low level keyboard hook. For
> example this delivers key events even if some other program is active in the
> foreground.
> 
> I think this can also be accomplished with a local hook scoped to the UI
thread,
> something along the lines of
> 
> SetWindowsHookEx(WH_KEYBOARD, callback, NULL, GetCurrentThreadId)

We want to capture ALT+TAB so applications like chrome remote desktop can send
it to
remote host. Based on my understanding it is not possible unless you use low
level hooks.

[Sriram, 2014-07-31 00:50:00]

Yet to address Ben's comment on the unit test (and adding unit tests at view
level) but otherwise rest of it is done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:131: // Extended
key when the event is received as part window event and so skip it
On 2014/07/31 00:14:48, cpu wrote:
> comments need periods at the end.

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:172: for (int index
= 0; index < _countof(keys_to_update); index++) {
On 2014/07/31 00:14:48, cpu wrote:
> dont use _countof()

Done.

[Sriram, 2014-07-31 00:55:28]

On 2014/07/31 00:14:48, cpu wrote:
> ok, I don't know of another way of getting alt-tab except via the current hook
> scheme, so I guess nevermind my previous suggestion.
> 
> But Scott is right, we need to unhook when we are not active.
We do uninstall (see KeyboardInterceptRegistration::Unhook()) and we do that
whenever
the window looses focus.

> 
>
https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):
> 
>
https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:33: hook_handle_
=
> SetWindowsHookEx(WH_KEYBOARD_LL, callback_function, NULL, 0);
> check that this is a thread with a UI style message pump.
> 
>
https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:131: // Extended
> key when the event is received as part window event and so skip it
> comments need periods at the end.
> 
>
https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:164: const int
> kKeyboardStateLength = 256;
> what is this function trying to do?
> 
> A full fledged comment about the general operation would be nice as well.
> 
>
https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:172: for (int
index
> = 0; index < _countof(keys_to_update); index++) {
> dont use _countof()

[Sriram, 2014-07-31 00:55:42]

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:33: hook_handle_ =
SetWindowsHookEx(WH_KEYBOARD_LL, callback_function, NULL, 0);
I am not sure as how I could check this. Could you please give me pointers?
Thanks!

On 2014/07/31 00:14:48, cpu wrote:
> check that this is a thread with a UI style message pump.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:164: const int
kKeyboardStateLength = 256;
On 2014/07/31 00:14:48, cpu wrote:
> what is this function trying to do?
> 
> A full fledged comment about the general operation would be nice as well.

Done.

[sky, 2014-07-31 15:21:23]

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:67:
task_runner->PostTask(
On 2014/07/31 00:21:13, Sriram wrote:
> Hook callbacks happen in the thread that calls SetWindowsHookEx() and so
posting
> to the thread that is supposed to be active for the lifetime (i.e. the UI
> thread).
> 
> On 2014/07/29 23:07:25, sky wrote:
> > Why do you PostTask here?
> 

I'm still confused by your use of threads. At the current time there is only
ever one thread using views. That may change, but currently its the case.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
On 2014/07/31 00:21:14, Sriram wrote:
> The registration should be done on a thread that would live for the lifetime
of
> the hook and so we would either have to create a dedicated thread (or I am
using
> UI thread here for this). That is the reason for it not done inline.
> 
> On 2014/07/29 23:07:26, sky wrote:
> > I think this code would be less error prone if it was effectively a one
shot.
> In
> > the constructor/Init() it installs the hook and in its destructor it
releases
> > it.
> 

I don't understand. I'm suggesting the following:
{
  DesktopKeyboardCaptureWin capturer(hwnd);   // sets capture.
}  // capturer is deleted and releases capture.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:23: static void
Capture(HWND window_handle, base::TaskRunner* task_runner);
On 2014/07/31 00:21:14, Sriram wrote:
> See above.
> 
> On 2014/07/29 23:07:26, sky wrote:
> > Why does this take a task_runner?
> 

And see my other comments about thread usage.

[Sriram, 2014-07-31 17:00:06]

Updated based on Ben's comments on test.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:27: LOG(INFO) <<
"Sending: " << key << " control = " << control << " shift = "
Is LOG(INFO) not allowed even in tests? What is the right way to log from tese?
I would like to continue to have someway to debug failures so would you suggest
using DVLOG?

On 2014/07/30 02:50:25, benwells wrote:
> Please remove logging here and elsewhere, LOG(INFO)'s should not be committed
as
> they cause log spam.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:37: LOG(INFO) <<
"Failed to send key to app";
On 2014/07/30 02:50:25, benwells wrote:
> You can use LOG(WARNING) here, but LOG(INFO) should be removed.

Done.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:50: bool
SendReply(const char* message) {
It is using ExtensionTestMessageListener to send the message but gets Reply()
function by sending a key event.

On 2014/07/30 02:50:25, benwells wrote:
> Can you explain what this is for? There is already a way to send replies to
apps
> via ExtensionTestMessageListener.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:57: if
(!key_listener.was_satisfied()) {
On 2014/07/30 02:50:25, benwells wrote:
> Why do you have to check this here? You have just created the listener so it
> should be unsatisfied.

Done.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:67:
base::RunLoop().RunUntilIdle();
On 2014/07/30 02:50:25, benwells wrote:
> Why do you run the message loop here?

Done.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:77: bool
WaitUntilKeyFocus() { return WaitForKeyEvent(ui::VKEY_Z); }
Intermediate iteration changed focus but not any more. Removed this function.

On 2014/07/30 02:50:25, benwells wrote:
> I'm not sure how this causes focus to be given to the app. Does
> ui_controls::SendKeyPressNotifyWhenDone do this? If so it might be worth
> mentioning it somewhere.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:81:
scoped_ptr<ExtensionTestMessageListener> launched_listener(
On 2014/07/30 02:50:24, benwells wrote:
> Why is this a scoped_ptr?
> 
> Oh, now I see. This code will be much simpler if you don't try and reuse the
> Launched message. You should introduce a new message 'Ready' or something
which
> gets the instructions.
> 
> This function would become much simpler (you can just call
> LoadAndLaunchPlatformApp(app_path) then ShowAndFocus the window and return,
and
> this function doesn't have to return a listener at all.

Done.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:109:
ui_controls::SendKeyPressNotifyWhenDone(
On 2014/07/30 02:50:25, benwells wrote:
> Can you use SimulateKeyPress here?

Done.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:137:
ASSERT_FALSE(SimulateKeyPress(ui::VKEY_Z)) << message_;
SendKeyPressNotifyWhenDone() will fail if the window to which key event needs to
be sent is in focus.

On 2014/07/30 02:50:25, benwells wrote:
> Where exactly does this fail? Does it timeout?

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:322: "U 16 0 false
true false false"}},
Not sure if there is a clean way as the only way I can think of is to encode the
knowledge of sequence in generic function which would be used in only one
specific case for each kind. 

I followed pattern that is used in Keyevents browser tests
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

On 2014/07/30 02:50:25, benwells wrote:
> Can this data be inferred before checking instead of having to list it all
out?

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:323: // ctrl+f which
has aceelerator binding should also reuslt in all keys
On 2014/07/30 02:50:25, benwells wrote:
> Nit: spelling (aceelerator, reuslt).

Done.

[Sriram, 2014-07-31 19:40:05]

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:67:
task_runner->PostTask(
On 2014/07/31 15:21:22, sky wrote:
> On 2014/07/31 00:21:13, Sriram wrote:
> > Hook callbacks happen in the thread that calls SetWindowsHookEx() and so
> posting
> > to the thread that is supposed to be active for the lifetime (i.e. the UI
> > thread).
> > 
> > On 2014/07/29 23:07:25, sky wrote:
> > > Why do you PostTask here?
> > 
> 
> I'm still confused by your use of threads. At the current time there is only
> ever one thread using views. That may change, but currently its the case.

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:21: class
VIEWS_EXPORT DesktopKeyboardCaptureWin {
On 2014/07/31 15:21:22, sky wrote:
> On 2014/07/31 00:21:14, Sriram wrote:
> > The registration should be done on a thread that would live for the lifetime
> of
> > the hook and so we would either have to create a dedicated thread (or I am
> using
> > UI thread here for this). That is the reason for it not done inline.
> > 
> > On 2014/07/29 23:07:26, sky wrote:
> > > I think this code would be less error prone if it was effectively a one
> shot.
> > In
> > > the constructor/Init() it installs the hook and in its destructor it
> releases
> > > it.
> > 
> 
> I don't understand. I'm suggesting the following:
> {
>   DesktopKeyboardCaptureWin capturer(hwnd);   // sets capture.
> }  // capturer is deleted and releases capture.

Done.

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:23: static void
Capture(HWND window_handle, base::TaskRunner* task_runner);
On 2014/07/31 15:21:22, sky wrote:
> On 2014/07/31 00:21:14, Sriram wrote:
> > See above.
> > 
> > On 2014/07/29 23:07:26, sky wrote:
> > > Why does this take a task_runner?
> > 
> 
> And see my other comments about thread usage.

Done.

[sky, 2014-07-31 21:36:56]

AFACIT you are not properly removing/adding back the hook when the active window
changes. Where is that being done?

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:184: bool
LowLevelHookHandler::Register(HWND window_handle) {
Why do we need to support more than one hwnd captured at a time?

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:221: 
Seems like we should unhook and/or DCHECK if you don't know about the window.

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:16:
DesktopKeyboardCaptureWin(HWND window_handle);
explicit.

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:19: bool Capture();
Make the constructor capture and the destructor release. No worry about
unbalanced calls or anything then.

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:25: };
DISALLOW...

[sky, 2014-07-31 21:40:30]

Actually, I see it now, it's done from
DesktopNativeWidgetAura::OnWindowActivated .

On Thu, Jul 31, 2014 at 2:36 PM,  <sky@chromium.org> wrote:
> AFACIT you are not properly removing/adding back the hook when the active
> window
> changes. Where is that being done?
>
>
>
https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc
> (right):
>
>
https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:184: bool
> LowLevelHookHandler::Register(HWND window_handle) {
> Why do we need to support more than one hwnd captured at a time?
>
>
https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:221:
> Seems like we should unhook and/or DCHECK if you don't know about the
> window.
>
>
https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h
> (right):
>
>
https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:16:
> DesktopKeyboardCaptureWin(HWND window_handle);
> explicit.
>
>
https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:19: bool
> Capture();
> Make the constructor capture and the destructor release. No worry about
> unbalanced calls or anything then.
>
>
https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:25: };
> DISALLOW...
>
> https://codereview.chromium.org/297123002/

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

[cpu_(ooo_6.6-7.5), 2014-07-31 21:45:21]

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/580001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:33: hook_handle_ =
SetWindowsHookEx(WH_KEYBOARD_LL, callback_function, NULL, 0);
On 2014/07/31 00:55:42, Sriram wrote:
> I am not sure as how I could check this. Could you please give me pointers?
> Thanks!
> 
> On 2014/07/31 00:14:48, cpu wrote:
> > check that this is a thread with a UI style message pump.
> 

DCHECK(base::MessageLoopForUI::IsCurrent())

[Sriram, 2014-07-31 22:54:18]

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:184: bool
LowLevelHookHandler::Register(HWND window_handle) {
I wanted to protect against register/unregistered ordering issues.  

On 2014/07/31 21:36:55, sky wrote:
> Why do we need to support more than one hwnd captured at a time?

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:221: 
We could get called after we call unhook (Windows API does not prevent this) so
we would have to handle it this way.

On 2014/07/31 21:36:55, sky wrote:
> Seems like we should unhook and/or DCHECK if you don't know about the window.

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:16:
DesktopKeyboardCaptureWin(HWND window_handle);
On 2014/07/31 21:36:56, sky wrote:
> explicit.

Done.

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:19: bool Capture();
On 2014/07/31 21:36:55, sky wrote:
> Make the constructor capture and the destructor release. No worry about
> unbalanced calls or anything then.

Done.

https://codereview.chromium.org/297123002/diff/680001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:25: };
On 2014/07/31 21:36:55, sky wrote:
> DISALLOW...

Done.

[benwells, 2014-08-01 00:32:43]

More comments (I've reviewed all the apps code now).

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:27: LOG(INFO) <<
"Sending: " << key << " control = " << control << " shift = "
On 2014/07/31 17:00:06, Sriram wrote:
> Is LOG(INFO) not allowed even in tests? What is the right way to log from
tese?
> I would like to continue to have someway to debug failures so would you
suggest
> using DVLOG?
> 
> On 2014/07/30 02:50:25, benwells wrote:
> > Please remove logging here and elsewhere, LOG(INFO)'s should not be
committed
> as
> > they cause log spam.
> 

Yes, DVLOG(1) or VLOG(1) is fine.

https://codereview.chromium.org/297123002/diff/580001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:50: bool
SendReply(const char* message) {
On 2014/07/31 17:00:05, Sriram wrote:
> It is using ExtensionTestMessageListener to send the message but gets Reply()
> function by sending a key event.
> 
> On 2014/07/30 02:50:25, benwells wrote:
> > Can you explain what this is for? There is already a way to send replies to
> apps
> > via ExtensionTestMessageListener.
> 

I'm unsure how this is working. In the .js it uses a normal reply to look for
'enabled' and 'disabled', but also has code in the key up handler which disables
whenever an 'A' is received.

Instead of sending instructions to the test app via keyboard events, can you use
another listener to send the reply instead of via the keyboard? In tests like
this where multiple things may need to happen I typically loop on the javascript
side, passing commands in from c++ via a normal reply. When the test is done an
empty command or a 'Done' command is sent.

https://codereview.chromium.org/297123002/diff/700001/apps/ui/native_app_wind...
File apps/ui/native_app_window.h (right):

https://codereview.chromium.org/297123002/diff/700001/apps/ui/native_app_wind...
apps/ui/native_app_window.h:52: // Set whether the window should receive all
keyboard messages from the
Ah, the suspense - from the what? :)

https://codereview.chromium.org/297123002/diff/700001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/app/generated_re...
chrome/app/generated_resources.grd:4509: +        Read and change anything you
type including task switching key like CMD+TAB
Nit: I think key should be plural

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:80:
ExtensionTestMessageListener launched_listener("Launched", true);
You don't need to have this listener at all. If you don't pass in a listener to
LoadAndLaunchPlatformApp it will wait for a "Launched" message by default.

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/extensio...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/extensio...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
As mentioned I don't think this is needed as you are doing the permission check.
As you have a test you can verify this.

Other parts of the API have to do it this way as the features system doesn't
support limiting new parameters and things like that by channel, but this is
particular case is handled automagically.

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:998: // TODO(sriramsr):
implement
Please add a bug link to the TODO.

https://codereview.chromium.org/297123002/diff/700001/chrome/common/extension...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:376: static void
setInterceptAllKeys(boolean wantAllKeys);
It would be mentioned that this is only available in dev channel.

[Sriram, 2014-08-11 21:01:28]

Fix all the comments. 

Also fixed unit test to not disable on key event (which was left over from old
iteration) which exposed bug in keyboard state handling. I have update the CL
with fix for that as well. It would be great if you could take a look.

Thanks,
Sriram

https://codereview.chromium.org/297123002/diff/700001/apps/ui/native_app_wind...
File apps/ui/native_app_window.h (right):

https://codereview.chromium.org/297123002/diff/700001/apps/ui/native_app_wind...
apps/ui/native_app_window.h:52: // Set whether the window should receive all
keyboard messages from the
On 2014/08/01 00:32:43, benwells wrote:
> Ah, the suspense - from the what? :)

Done.

https://codereview.chromium.org/297123002/diff/700001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/app/generated_re...
chrome/app/generated_resources.grd:4509: +        Read and change anything you
type including task switching key like CMD+TAB
On 2014/08/01 00:32:43, benwells wrote:
> Nit: I think key should be plural

Done.

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:80:
ExtensionTestMessageListener launched_listener("Launched", true);
On 2014/08/01 00:32:43, benwells wrote:
> You don't need to have this listener at all. If you don't pass in a listener
to
> LoadAndLaunchPlatformApp it will wait for a "Launched" message by default.

Done.

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/extensio...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/extensio...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:411:
if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
On 2014/08/01 00:32:43, benwells wrote:
> As mentioned I don't think this is needed as you are doing the permission
check.
> As you have a test you can verify this.
> 
> Other parts of the API have to do it this way as the features system doesn't
> support limiting new parameters and things like that by channel, but this is
> particular case is handled automagically.

Done.

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:998: // TODO(sriramsr):
implement
On 2014/08/01 00:32:43, benwells wrote:
> Please add a bug link to the TODO.

Done.

https://codereview.chromium.org/297123002/diff/700001/chrome/common/extension...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/297123002/diff/700001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:376: static void
setInterceptAllKeys(boolean wantAllKeys);
On 2014/08/01 00:32:43, benwells wrote:
> It would be mentioned that this is only available in dev channel.

Done.

[sky, 2014-08-11 22:42:56]

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:130: }
End with // namespace

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:135: class
KeyboardInterceptRegistration {
These classes should be in the anonymous namespace too. You should probably
move:
namespace views {
to 15.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:146: // Make sure
that hook is set from main thread as it has to be valid for
It's confusing that some of these classes are totally inlined and some are a
hanful. In general the same rules for a header should apply here.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:223: bool
IsRegistered(HWND handle);
AFAICT this isn't used.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:269: bool
LowLevelHookHandler::Register(HWND window_handle) {
AFAICT you don't actually use the return value.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:273: if
(!RegisterMessageFilter()) {
nit: no {}

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:274: return FALSE;
false

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:279: if
(registration->Hook(KeyboardHook)) {
Why do we hook for each HWND? Shouldn't we only ever allow a single hook?

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:300: return TRUE;
true (in fact go through all make sure you really need caps in any of these (eg
307/309 should be lower case too).

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:371: if
(registration) {
Can you really end up with a situation where there is no one registered for
current_active_window?

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:386:
PostMessage(current_active_window,
I don't understand the need to post messages. Can't you process the keyboard
event immediately?

[Sriram, 2014-08-11 23:49:57]

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:130: }
On 2014/08/11 22:42:56, sky wrote:
> End with // namespace

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:135: class
KeyboardInterceptRegistration {
On 2014/08/11 22:42:56, sky wrote:
> These classes should be in the anonymous namespace too. You should probably
> move:
> namespace views {
> to 15.

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:146: // Make sure
that hook is set from main thread as it has to be valid for
On 2014/08/11 22:42:56, sky wrote:
> It's confusing that some of these classes are totally inlined and some are a
> hanful. In general the same rules for a header should apply here.

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:223: bool
IsRegistered(HWND handle);
On 2014/08/11 22:42:56, sky wrote:
> AFAICT this isn't used.

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:269: bool
LowLevelHookHandler::Register(HWND window_handle) {
On 2014/08/11 22:42:56, sky wrote:
> AFAICT you don't actually use the return value.

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:273: if
(!RegisterMessageFilter()) {
On 2014/08/11 22:42:56, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:273: if
(!RegisterMessageFilter()) {
On 2014/08/11 22:42:56, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:274: return FALSE;
On 2014/08/11 22:42:56, sky wrote:
> false

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:279: if
(registration->Hook(KeyboardHook)) {
Currently the scope of register/unregister is controlled outside of this calls
which could result in register being called before unregister is called. I
decided to keep it per HWND to avoid refcouting on registration.

On 2014/08/11 22:42:56, sky wrote:
> Why do we hook for each HWND? Shouldn't we only ever allow a single hook?

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:300: return TRUE;
On 2014/08/11 22:42:55, sky wrote:
> true (in fact go through all make sure you really need caps in any of these
(eg
> 307/309 should be lower case too).

Done.

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:371: if
(registration) {
Yes, this is possible as you can get a callback after you have called unregister
(based on window API behavior). In this case we don't want to send message.

On 2014/08/11 22:42:55, sky wrote:
> Can you really end up with a situation where there is no one registered for
> current_active_window?

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:386:
PostMessage(current_active_window,
Keyboard events needs to be posted so that char events are interested into
message loop when TranslateMessage() is called after delegates are called. Using
SendMessage() would bypass both which is not ideal and so need to inject one
keyboard event at a time and handling queuing.

On 2014/08/11 22:42:56, sky wrote:
> I don't understand the need to post messages. Can't you process the keyboard
> event immediately?

[benwells, 2014-08-12 04:30:33]

https://codereview.chromium.org/297123002/diff/780001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/780001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:53: return
SendKeyEvent(ui::VKEY_A, message);
I'm still not sure what you're doing here. I don't think this will do anything,
which makes me wonder if your tests are testing enabling and disabling.

[Sriram, 2014-08-12 05:32:52]

I have updated the js (has permission) to actually not do any
enable/disable on getting keyboard event but just respond with function
that can be used to disable/enabled intercept. Did I miss something?
On Aug 11, 2014 9:30 PM, <benwells@chromium.org> wrote:

>
> https://codereview.chromium.org/297123002/diff/780001/
> chrome/browser/apps/app_window_intercept_all_keys_uitest.cc
> File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc
> (right):
>
> https://codereview.chromium.org/297123002/diff/780001/
> chrome/browser/apps/app_window_intercept_all_keys_uitest.cc#newcode53
> chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:53: return
> SendKeyEvent(ui::VKEY_A, message);
> I'm still not sure what you're doing here. I don't think this will do
> anything, which makes me wonder if your tests are testing enabling and
> disabling.
>
> https://codereview.chromium.org/297123002/
>

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

[sky, 2014-08-12 16:18:47]

+ananta

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:371: if
(registration) {
Are you sure about that? Once you unhook you shouldn't see any more messages.

On 2014/08/11 23:49:56, Sriram wrote:
> Yes, this is possible as you can get a callback after you have called
unregister
> (based on window API behavior). In this case we don't want to send message.
> 
> On 2014/08/11 22:42:55, sky wrote:
> > Can you really end up with a situation where there is no one registered for
> > current_active_window?
>

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:386:
PostMessage(current_active_window,
This seems wrong to me. I'm adding ananta to the review as he knows more about
the low level details like this.

On 2014/08/11 23:49:57, Sriram wrote:
> Keyboard events needs to be posted so that char events are interested into
> message loop when TranslateMessage() is called after delegates are called.
Using
> SendMessage() would bypass both which is not ideal and so need to inject one
> keyboard event at a time and handling queuing.
> 
> On 2014/08/11 22:42:56, sky wrote:
> > I don't understand the need to post messages. Can't you process the keyboard
> > event immediately?
>

[Sriram, 2014-08-12 16:24:04]

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/740001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:371: if
(registration) {
See remarks for UnhookWindowsHookEx():
http://msdn.microsoft.com/en-us/library/windows/desktop/ms644993(v=vs.85).aspx

The hook procedure can be in the state of being called by another thread even
after UnhookWindowsHookEx returns. If the hook procedure is not being called
concurrently, the hook procedure is removed immediately before
UnhookWindowsHookEx returns.


On 2014/08/12 16:18:47, sky wrote:
> Are you sure about that? Once you unhook you shouldn't see any more messages.
> 
> On 2014/08/11 23:49:56, Sriram wrote:
> > Yes, this is possible as you can get a callback after you have called
> unregister
> > (based on window API behavior). In this case we don't want to send message.
> > 
> > On 2014/08/11 22:42:55, sky wrote:
> > > Can you really end up with a situation where there is no one registered
for
> > > current_active_window?
> > 
>

https://codereview.chromium.org/297123002/diff/780001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/780001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:53: return
SendKeyEvent(ui::VKEY_A, message);
Event listener in app always returns the function that can be used to toggle
keyboard intercept state. So here we listen for that function and reply with
message that is passed.

On 2014/08/12 04:30:33, benwells wrote:
> I'm still not sure what you're doing here. I don't think this will do
anything,
> which makes me wonder if your tests are testing enabling and disabling.

[ananta, 2014-08-12 17:51:49]

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:309: if
(registration->Hook(KeyboardHook)) {
We can record the window handle but register once if that is doable

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:348: if (code !=
base::MessagePumpForUI::kMessageFilterCode)
Shouldn't we be calling CallNextHookEx here?

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:435:
LowLevelHookHandler::GetInstance()->Register(window_handle_);
Registering on a per window basis seems like overkill. In any case low level
keyboard hooks are expensive due to
the context switches involved from the thread for which the input was intended
to the keyboard hook thread.

We should do this once for the main thread.

[Sriram, 2014-08-12 19:02:49]

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:309: if
(registration->Hook(KeyboardHook)) {
On 2014/08/12 17:51:49, ananta wrote:
> We can record the window handle but register once if that is doable

Done.

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:348: if (code !=
base::MessagePumpForUI::kMessageFilterCode)
On 2014/08/12 17:51:49, ananta wrote:
> Shouldn't we be calling CallNextHookEx here?

Done.

https://codereview.chromium.org/297123002/diff/780001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:435:
LowLevelHookHandler::GetInstance()->Register(window_handle_);
Changed to register low level hook per thread but left the keyboard hook
handling to be per window.

On 2014/08/12 17:51:49, ananta wrote:
> Registering on a per window basis seems like overkill. In any case low level
> keyboard hooks are expensive due to
> the context switches involved from the thread for which the input was intended
> to the keyboard hook thread.
> 
> We should do this once for the main thread.

[ananta, 2014-08-12 19:43:26]

https://codereview.chromium.org/297123002/diff/800001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/800001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:367: HWND
current_active_window = GetActiveWindow();
Shouldn't you be using GetFocus here?. GetFocus returns the window with keyboard
focus. If the API returns
NULL, then we should not do anything?

[Sriram, 2014-08-12 22:26:35]

https://codereview.chromium.org/297123002/diff/800001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/800001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:367: HWND
current_active_window = GetActiveWindow();
On 2014/08/12 19:43:26, ananta wrote:
> Shouldn't you be using GetFocus here?. GetFocus returns the window with
keyboard
> focus. If the API returns
> NULL, then we should not do anything?

Done.

[benwells, 2014-08-12 22:28:24]

https://codereview.chromium.org/297123002/diff/800001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/800001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:52: bool
SendReply(const char* message) {
After studying this I think I understand now what is happening, but this is very
confusing and overly complicated. To get your commands to the javascript what
you are doing is:
- send keyboard event 'A'
- in the javascript catch this and send the message 'KeyReceived'
- the c++ replies with a command

Instead I think you can simply:
- in the javascript send the message 'ReadyForCommand' or similar. You can use
"Launched" but it is probably simpler if you don't.
- in the c++ reply wait for this and reply with the command.

This is how we do this kind of communication between c++ and js, there is no
need for keyboard events. If you're using keyboard events to improve coverage,
do that some other way.

Also in the js you can remove the message "onReplyReceived" as it is never
listened for in c++, and also you don't need the callback when sending
"Launched" as you never reply to this message.

https://codereview.chromium.org/297123002/diff/800001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:137: std::string
message_;
Please change this field name. It is confusing to have message_ and message in
functions that refer to different things.

[Sriram, 2014-08-13 00:50:09]

https://codereview.chromium.org/297123002/diff/800001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/800001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:52: bool
SendReply(const char* message) {
On 2014/08/12 22:28:24, benwells wrote:
> After studying this I think I understand now what is happening, but this is
very
> confusing and overly complicated. To get your commands to the javascript what
> you are doing is:
> - send keyboard event 'A'
> - in the javascript catch this and send the message 'KeyReceived'
> - the c++ replies with a command
> 
> Instead I think you can simply:
> - in the javascript send the message 'ReadyForCommand' or similar. You can use
> "Launched" but it is probably simpler if you don't.
> - in the c++ reply wait for this and reply with the command.
> 
> This is how we do this kind of communication between c++ and js, there is no
> need for keyboard events. If you're using keyboard events to improve coverage,
> do that some other way.
> 
> Also in the js you can remove the message "onReplyReceived" as it is never
> listened for in c++, and also you don't need the callback when sending
> "Launched" as you never reply to this message.

Done.

https://codereview.chromium.org/297123002/diff/800001/chrome/browser/apps/app...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:137: std::string
message_;
On 2014/08/12 22:28:24, benwells wrote:
> Please change this field name. It is confusing to have message_ and message in
> functions that refer to different things.

Done.

[benwells, 2014-08-13 06:20:28]

I think all my comments have been addressed now, so lgtm.

[Sriram, 2014-08-13 21:13:43]

Thanks Ben!

Ananta/Scott - can you take a look as well? 

On 2014/08/13 06:20:28, benwells wrote:
> I think all my comments have been addressed now, so lgtm.

[Sriram, 2014-08-14 16:53:02]

Ping...

On 2014/08/13 21:13:43, Sriram wrote:
> Thanks Ben!
> 
> Ananta/Scott - can you take a look as well? 
> 
> On 2014/08/13 06:20:28, benwells wrote:
> > I think all my comments have been addressed now, so lgtm.

[sky, 2014-08-14 17:24:03]

That ping is for Ananta. I'm waiting for him to be happy.

On Thu, Aug 14, 2014 at 9:53 AM,  <sriramsr@chromium.org> wrote:
> Ping...
>
>
> On 2014/08/13 21:13:43, Sriram wrote:
>>
>> Thanks Ben!
>
>
>> Ananta/Scott - can you take a look as well?
>
>
>> On 2014/08/13 06:20:28, benwells wrote:
>> > I think all my comments have been addressed now, so lgtm.
>
>
>
>
> https://codereview.chromium.org/297123002/

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

[ananta, 2014-08-14 23:12:45]

https://codereview.chromium.org/297123002/diff/840001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/840001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:367: HWND
current_active_window = GetFocus();
If there is no focus window we should just call CallNextHookEx and return

Other than that LGTM for the hook stuff

[Sriram, 2014-08-15 00:33:17]

Thanks Ananta! 

Sky/Sadrul/CPU - are you guys okay with me submitting the change?

https://codereview.chromium.org/297123002/diff/840001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/840001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:367: HWND
current_active_window = GetFocus();
On 2014/08/14 23:12:45, ananta wrote:
> If there is no focus window we should just call CallNextHookEx and return
> 
> Other than that LGTM for the hook stuff

Done.

[Sriram, 2014-08-15 17:39:40]

Ping Scott.
Ping Sadrul.


On 2014/08/15 00:33:17, Sriram wrote:
> Thanks Ananta! 
> 
> Sky/Sadrul/CPU - are you guys okay with me submitting the change?
> 
>
https://codereview.chromium.org/297123002/diff/840001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):
> 
>
https://codereview.chromium.org/297123002/diff/840001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:367: HWND
> current_active_window = GetFocus();
> On 2014/08/14 23:12:45, ananta wrote:
> > If there is no focus window we should just call CallNextHookEx and return
> > 
> > Other than that LGTM for the hook stuff
> 
> Done.

[sadrul, 2014-08-15 17:48:05]

My earlier comment in this thread:

> Instead of plumbing this through views, can you have DesktopKeyboardCaptureWin
> live closer to chrome_native_app_window_views (where it seems to be used)? It
> can get the HWND from views::Widget -> aura::Window -> aura::WindowTreeHost ->
> gfx::AcceleratedWidget, and it can also know when a Widget becomes active from
a
> WidgetObserver. So it should be able to live out of views.

I still think having a separate object (say, a 'SystemKeyInterceptor') that
takes care of creating the DesktopKeyboardCaptureWin() (or other
platform-specific objects) when the widget gets [de]activated would be better
than plumbing this all through the Widget API.

[sky, 2014-08-15 17:49:01]

I like Sadrul's suggestion too. I do have a couple of questions still
on the patch. I'm going to talk with Ananta when he is in later as
well.

On Fri, Aug 15, 2014 at 10:48 AM,  <sadrul@chromium.org> wrote:
> My earlier comment in this thread:
>
>> Instead of plumbing this through views, can you have
>> DesktopKeyboardCaptureWin
>> live closer to chrome_native_app_window_views (where it seems to be used)?
>> It
>> can get the HWND from views::Widget -> aura::Window ->
>> aura::WindowTreeHost ->
>> gfx::AcceleratedWidget, and it can also know when a Widget becomes active
>> from
>
> a
>>
>> WidgetObserver. So it should be able to live out of views.
>
>
> I still think having a separate object (say, a 'SystemKeyInterceptor') that
> takes care of creating the DesktopKeyboardCaptureWin() (or other
> platform-specific objects) when the widget gets [de]activated would be
> better
> than plumbing this all through the Widget API.
>
> https://codereview.chromium.org/297123002/

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

[sky, 2014-08-15 17:49:36]

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:76: int
keys_to_save[] = {VK_SHIFT, VK_CONTROL, VK_MENU};
const and then rename to kKeysToSave.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:207: };
DISALLOW...

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:303: return false;
If this does happen do you need to unregister keyboard_hook_?

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:337:
PostMessage(msg->hwnd,
If I understand this correctly the sequence for this code is:
1. get real key event, put on queue, post task
2. when get task restore key state, and post another task with potentially
modified key event
3. process key event.

If this is correct, there is a ton of asynchronous logic in here. How do you
know the key state hasn't changed out from under you from steps 1-3? Say the
user is holding a key down or the system is slow and lots of key events are
coming in?

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:347: if
(!registration->IsKeyboardEventQueueEmpty()) {
Is there a reason you don't process the rest immediately?

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2014

[ananta, 2014-08-15 18:07:27]

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:337:
PostMessage(msg->hwnd,
+1 to sky's comment about the keyboard state changing before the posted message
is processed. This may make the
behavior unpredictable.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:367: HWND
current_active_window = GetFocus();
We don't want to interfere with the default OS processing if our app is not in
the foreground. Focus and active windows are thread specific states as in a
thread may have a focus window which only has keyboard focus when the
thread is in the foreground.

We should use GetForegroundWindow and check if the window returned belongs to
our process. If not bail.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:819: if (IsActive())
{
IsActive is not going to work correctly as it uses GetActiveWindow which is a
thread specific state. We should use GetForegroundWindow and check if our thread
is the foreground thread.

[Sriram, 2014-08-15 20:59:21]

On 2014/08/15 17:48:05, sadrul wrote:
> My earlier comment in this thread:
> 
> > Instead of plumbing this through views, can you have
DesktopKeyboardCaptureWin
> > live closer to chrome_native_app_window_views (where it seems to be used)?
It
> > can get the HWND from views::Widget -> aura::Window -> aura::WindowTreeHost
->
> > gfx::AcceleratedWidget, and it can also know when a Widget becomes active
from
> a
> > WidgetObserver. So it should be able to live out of views.
> 
> I still think having a separate object (say, a 'SystemKeyInterceptor') that
> takes care of creating the DesktopKeyboardCaptureWin() (or other
> platform-specific objects) when the widget gets [de]activated would be better
> than plumbing this all through the Widget API.

Ok. I will look into this. Would going down this approach make it harder/uglier
to
interact with fullscreen/PointerLock bubble UI? This is the question I had for
you
as I am not as familiar as you are with the code base.

Also from layering, what is the separation of duties between the widget and
view?

[Sriram, 2014-08-15 20:59:54]

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc (right):

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:76: int
keys_to_save[] = {VK_SHIFT, VK_CONTROL, VK_MENU};
On 2014/08/15 17:49:35, sky wrote:
> const and then rename to kKeysToSave.

Done.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:207: };
On 2014/08/15 17:49:35, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:303: return false;
On 2014/08/15 17:49:35, sky wrote:
> If this does happen do you need to unregister keyboard_hook_?

Done.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:337:
PostMessage(msg->hwnd,
Keyboard state associated with Get/SetKeyState state is state of the keys
when the message was generated and GetAsyncKeyState() returns the current
interrupt level based state. KeyState should be constant till WM_CHAR event
is processed and so that is the reason to not to process inline.

The sequence of event is:
1. Intercept key event in hook and queue message after saving key state.
2. Inject key restore and key event along with inject key event if there are
  more key events to be processed. This is done so that the key state is
  preserved if WM_CHAR event is inserted when TranslateMessage() is called when
  processing Key event.
3. Process key restore and key event (which could insert WM_CHAR event).

My understanding is that key state of a thread is independent of current
interrupt
state and reflects keyboard state when message was actually inserted in the
queue.

On 2014/08/15 17:49:35, sky wrote:
> If I understand this correctly the sequence for this code is:
> 1. get real key event, put on queue, post task
> 2. when get task restore key state, and post another task with potentially
> modified key event
> 3. process key event.
> 
> If this is correct, there is a ton of asynchronous logic in here. How do you
> know the key state hasn't changed out from under you from steps 1-3? Say the
> user is holding a key down or the system is slow and lots of key events are
> coming in?

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:337:
PostMessage(msg->hwnd,
See response below.

On 2014/08/15 18:07:26, ananta wrote:
> +1 to sky's comment about the keyboard state changing before the posted
message
> is processed. This may make the
> behavior unpredictable.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:347: if
(!registration->IsKeyboardEventQueueEmpty()) {
See above.

On 2014/08/15 17:49:35, sky wrote:
> Is there a reason you don't process the rest immediately?

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.cc:367: HWND
current_active_window = GetFocus();
We unregistered callback window losses focus. This is done outside of this
class.

On 2014/08/15 18:07:26, ananta wrote:
> We don't want to interfere with the default OS processing if our app is not in
> the foreground. Focus and active windows are thread specific states as in a
> thread may have a focus window which only has keyboard focus when the
> thread is in the foreground.
> 
> We should use GetForegroundWindow and check if the window returned belongs to
> our process. If not bail.

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h (right):

https://codereview.chromium.org/297123002/diff/860001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_keyboard_capture_win.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/08/15 17:49:35, sky wrote:
> 2014

Done.

[Sriram, 2014-11-14 20:57:36]

Back from parental leave and would be great if you can take a look. 

PTAL.

[benwells, 2014-11-16 23:10:54]

On 2014/11/14 20:57:36, Sriram wrote:
> Back from parental leave and would be great if you can take a look. 
> 
> PTAL.

This is an epic patch. I don't think I need to look at it more, but maybe I do.

It would help your reviewers if you remind them of what you want them to look
at.

[Sriram, 2014-11-17 17:50:19]

Quick summary of the change since last time:
Used WidgetObserver as per Sadrul's suggestion and reverted all the changes in
view/widget. So changes are all contained to browser/ui/views/apps.

[sadrul, 2014-11-18 22:21:42]

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

[sadrul, 2014-11-18 22:21:46]

Removing myself from the reviewers' list since I don't own any code in the
current patchset. But the general approach looks good to me. A couple of nits:

https://codereview.chromium.org/297123002/diff/1000001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/desktop_keyboard_capture.h (right):

https://codereview.chromium.org/297123002/diff/1000001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/desktop_keyboard_capture.h:12: // callback.
Mention that the hooks are installed/uninstalled as appropriate with the
widget's activation changes.

https://codereview.chromium.org/297123002/diff/1000001/extensions/browser/app...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/297123002/diff/1000001/extensions/browser/app...
extensions/browser/app_window/app_window.h:557: bool cached_want_all_keys_;
Is this cache useful?

[Sriram, 2014-11-18 22:45:43]

https://codereview.chromium.org/297123002/diff/1000001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/desktop_keyboard_capture.h (right):

https://codereview.chromium.org/297123002/diff/1000001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/desktop_keyboard_capture.h:12: // callback.
On 2014/11/18 22:21:46, sadrul wrote:
> Mention that the hooks are installed/uninstalled as appropriate with the
> widget's activation changes.

Done.

https://codereview.chromium.org/297123002/diff/1000001/extensions/browser/app...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/297123002/diff/1000001/extensions/browser/app...
extensions/browser/app_window/app_window.h:557: bool cached_want_all_keys_;
It is used to unregister from destructor if hooks were registered.

On 2014/11/18 22:21:46, sadrul wrote:
> Is this cache useful?

[Sriram, 2014-11-18 22:48:00]

The CQ bit was checked by sriramsr@chromium.org

[Sriram, 2014-11-18 22:48:01]

The CQ bit was unchecked by sriramsr@chromium.org

[benwells, 2014-11-18 22:56:07]

Based on the conversation that just happened I can deduce that the stuff I have
already lg'd has now substantially changed, and I should look again.

So please wait until I have given another lg before landing.

[benwells, 2014-11-21 00:52:01]

For me, just a little question about the need to cache that value, but I'd also
like someone with more windows / event handling expertise to have a look at the
keyboard hooking code.

BTW I'd find it easier to review if you rebased less frequently. It is great
that you keep rebases in their own patch sets, please continue doing that!

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/desktop_keyboard_capture.cc (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/desktop_keyboard_capture.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
Nit: no (c)

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/desktop_keyboard_capture.h (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/desktop_keyboard_capture.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
Nit: no (c)

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/keyboard_hook_handler.cc (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
Nit: no (c)

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.cc:10: // Empty
implementation for other platform.
Nit: platformS

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.cc:17: KeyboardHookHandler*
KeyboardHookHandler::GetInstance() {
nit: // static on line above.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/keyboard_hook_handler.h (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
Nit: no (c)

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
Nit: no (c)

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:5: #include
"chrome/browser/ui/views/apps/keyboard_hook_handler.h"
I'd prefer it if either ananta or sky could review this file. I think there has
been much discussion and changes since ananta gave his lg.

https://codereview.chromium.org/297123002/diff/940001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2508:
'browser/ui/views/apps/keyboard_hook_handler.cc',
mac?

https://codereview.chromium.org/297123002/diff/940001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/297123002/diff/940001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:352: if (cached_want_all_keys_) {
Why is this necessary? Shouldn't the new WidgetObserver you've created get
destroyed in this case and clear all it needs to clear?

https://codereview.chromium.org/297123002/diff/1020001/chrome/browser/apps/ap...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/1020001/chrome/browser/apps/ap...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:223: const
extensions::Extension* extension = NULL;
nit: nullptr instead of NULL (here and elsewhere in this file).

[Sriram, 2014-11-21 05:35:21]

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/desktop_keyboard_capture.cc (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/desktop_keyboard_capture.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/11/21 00:52:00, benwells wrote:
> Nit: no (c)

Done.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/desktop_keyboard_capture.h (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/desktop_keyboard_capture.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/11/21 00:52:01, benwells wrote:
> Nit: no (c)

Done.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/keyboard_hook_handler.cc (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/11/21 00:52:01, benwells wrote:
> Nit: no (c)

Done.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.cc:10: // Empty
implementation for other platform.
On 2014/11/21 00:52:01, benwells wrote:
> Nit: platformS

Done.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.cc:17: KeyboardHookHandler*
KeyboardHookHandler::GetInstance() {
On 2014/11/21 00:52:01, benwells wrote:
> nit: // static on line above.

Done.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/keyboard_hook_handler.h (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/11/21 00:52:01, benwells wrote:
> Nit: no (c)

Done.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/11/21 00:52:01, benwells wrote:
> Nit: no (c)

Done.

https://codereview.chromium.org/297123002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:5: #include
"chrome/browser/ui/views/apps/keyboard_hook_handler.h"
On 2014/11/21 00:52:01, benwells wrote:
> I'd prefer it if either ananta or sky could review this file. I think there
has
> been much discussion and changes since ananta gave his lg.

I have ping Ananta to take a look again but only change since Ananta LGTM is
that the code has moved location but the original keyboard event intercept
handling has not changed.

https://codereview.chromium.org/297123002/diff/940001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/297123002/diff/940001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:352: if (cached_want_all_keys_) {
On 2014/11/21 00:52:01, benwells wrote:
> Why is this necessary? Shouldn't the new WidgetObserver you've created get
> destroyed in this case and clear all it needs to clear?

Done.

https://codereview.chromium.org/297123002/diff/1020001/chrome/browser/apps/ap...
File chrome/browser/apps/app_window_intercept_all_keys_uitest.cc (right):

https://codereview.chromium.org/297123002/diff/1020001/chrome/browser/apps/ap...
chrome/browser/apps/app_window_intercept_all_keys_uitest.cc:223: const
extensions::Extension* extension = NULL;
On 2014/11/21 00:52:01, benwells wrote:
> nit: nullptr instead of NULL (here and elsewhere in this file).

Done.

[Sriram, 2014-11-21 20:39:45]

https://codereview.chromium.org/297123002/diff/940001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/297123002/diff/940001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2508:
'browser/ui/views/apps/keyboard_hook_handler.cc',
On 2014/11/21 00:52:01, benwells wrote:
> mac?

Missed replying to this one - Native app window for mac (i.e.
native_app_window_cocoa.mm) is not implemented and so does not need one.

[benwells, 2014-11-25 03:11:16]

On 2014/11/21 20:39:45, Sriram wrote:
>
https://codereview.chromium.org/297123002/diff/940001/chrome/chrome_browser_u...
> File chrome/chrome_browser_ui.gypi (right):
> 
>
https://codereview.chromium.org/297123002/diff/940001/chrome/chrome_browser_u...
> chrome/chrome_browser_ui.gypi:2508:
> 'browser/ui/views/apps/keyboard_hook_handler.cc',
> On 2014/11/21 00:52:01, benwells wrote:
> > mac?
> 
> Missed replying to this one - Native app window for mac (i.e.
> native_app_window_cocoa.mm) is not implemented and so does not need one.

ok. lgtm, but please wait for ananta@. I want to make sure his comments in #73
have been addressed as these were made after he gave his lg.

[ananta, 2014-11-25 23:47:30]

https://codereview.chromium.org/297123002/diff/1040001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc (right):

https://codereview.chromium.org/297123002/diff/1040001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:308:
GetCurrentThreadId());
Instead of having to disable hooks here on failure to register a message filter
hook, it might be better to reverse the order of hook installation, i.e. install
message filter hook first.

https://codereview.chromium.org/297123002/diff/1040001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:371: HWND
current_active_window = GetFocus();
As mentioned earlier, GetFocus will return the focused window for the thread.
This may not be the actual foreground thread. 

I think we should use GetForegroundWindow here and bail with CallNextHookEx if
the window is not in our process.

[Sriram, 2014-11-26 00:36:16]

https://codereview.chromium.org/297123002/diff/1040001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc (right):

https://codereview.chromium.org/297123002/diff/1040001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:308:
GetCurrentThreadId());
On 2014/11/25 23:47:30, ananta wrote:
> Instead of having to disable hooks here on failure to register a message
filter
> hook, it might be better to reverse the order of hook installation, i.e.
install
> message filter hook first.
> 

Done.

https://codereview.chromium.org/297123002/diff/1040001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:371: HWND
current_active_window = GetFocus();
On 2014/11/25 23:47:30, ananta wrote:
> As mentioned earlier, GetFocus will return the focused window for the thread.
> This may not be the actual foreground thread. 
> 
> I think we should use GetForegroundWindow here and bail with CallNextHookEx if
> the window is not in our process. 

Done.

[ananta, 2014-11-26 22:38:57]

https://codereview.chromium.org/297123002/diff/1060001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc (right):

https://codereview.chromium.org/297123002/diff/1060001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:372: HWND
current_active_window = GetForegroundWindow();
We need to check here that the foreground window is us. You can do this via the
GetWindowThreadProcessId API.

[Sriram, 2014-11-26 23:37:11]

https://codereview.chromium.org/297123002/diff/1060001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc (right):

https://codereview.chromium.org/297123002/diff/1060001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/keyboard_hook_handler_win.cc:372: HWND
current_active_window = GetForegroundWindow();
On 2014/11/26 22:38:56, ananta wrote:
> We need to check here that the foreground window is us. You can do this via
the
> GetWindowThreadProcessId API.

Done.

[ananta, 2014-11-26 23:46:11]

lgtm

[Sriram, 2014-11-27 00:15:17]

sriramsr@chromium.org changed reviewers:
+ mpearson@google.com

[Sriram, 2014-11-27 00:16:08]

Adding Mark (mpearson) as OWNERS for histogram has changed since I got LGTM from
Alexei Svitkine.

[cpu_(ooo_6.6-7.5), 2014-12-01 16:45:33]

cpu@chromium.org changed reviewers:
- cpu@chromium.org

[Sriram, 2014-12-01 18:56:09]

The CQ bit was checked by sriramsr@chromium.org

[commit-bot: I haz the power, 2014-12-01 18:57:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/297123002/1100001

[commit-bot: I haz the power, 2014-12-01 19:01:37]

Committed patchset #56 (id:1100001)

[commit-bot: I haz the power, 2014-12-01 19:02:15]

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

[Mark P, 2014-12-01 19:26:22]

mpearson@chromium.org changed reviewers:
+ asvitkine@chromium.org
- mpearson@google.com

[markusheintz_, 2014-12-02 13:14:31]

On 2014/12/01 19:02:15, I haz the power (commit-bot) wrote:
> Patchset 56 (id:??) landed as
> https://crrev.com/c811bb98b489b67a5f85ed79d7acd288533a080d
> Cr-Commit-Position: refs/heads/master@{#306219}

FYI: This CL is causing the interactive_ui_tests on Windows-XP-SP3 to fail. See
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...

I can't revert it due to conflicts.

[markusheintz_, 2014-12-02 13:14:35]

On 2014/12/01 19:02:15, I haz the power (commit-bot) wrote:
> Patchset 56 (id:??) landed as
> https://crrev.com/c811bb98b489b67a5f85ed79d7acd288533a080d
> Cr-Commit-Position: refs/heads/master@{#306219}

FYI: This CL is causing the interactive_ui_tests on Windows-XP-SP3 to fail. See
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...

I can't revert it due to conflicts.