Adding support for Win+L key combo in Windows host.

Adding support for Win+L key combo in Windows host.

This change will add a handler for the WIN+L key combination on non-home SKUs
of Windows.  When the WIN+L combo is pressed, we will call LockWorkstation.

Based on my investigation, it appears that this combo cannot be overridden and
only works when issued from a physical keyboard.  Therefore it isn't something
that can be issued remotely and our function call will not conflict with some
other behavior.  We skip the Home SKUs as locking the workstation doesn't
really make sense there (it brings up the switch users UI instead).

BUG=517322
Committed: https://crrev.com/83eff387b6535ea4d74ed993e0dbc7cf719a71c5
Cr-Commit-Position: refs/heads/master@{#390673}

[joedow, 2016-04-28 20:35:53]

joedow@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[joedow, 2016-04-28 20:35:54]

Implementing LockWorkStation functionality for Windows Host on non-Home SKUs.

PTAL!

Thanks,
Joe

[Jamie, 2016-04-28 22:00:12]

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process.cc (right):

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:79: }
I think this needs to factored out more cleanly into a Windows-specific location
(for example, I'm not sure that the new #include will work on other platforms).
In general, platform-specific ifdefs should be a last resort.

One option would be to have a generic shortcut-handler interface that could be
registered for platform-specific keyboard combinations. Windows could then use
that for Ctrl+Alt+Del and Win+L (with the advantage of moving related
functionality closer together). Other platforms could simply not register a
handler.

WDYT?

[Wez, 2016-04-28 22:28:28]

typo in description: second paragraph says you skip non-Home SKUs.

[Jamie, 2016-04-29 00:41:42]

LGTM.

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process.cc (right):

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:19: #include "base/win/windows_version.h"
Does this also need an ifdef?

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:77: NOTIMPLEMENTED();
Maybe NOTREACHED would be more appropriate here?

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/win/sessi...
File remoting/host/win/session_input_injector.h (right):

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/win/sessi...
remoting/host/win/session_input_injector.h:22: // Ctrl+Alt+Del and Win + L key
combinations by invoking the given callbacks.
Nit: No spaces around "+" for consistency with Ctrl+Alt+Del.

[joedow, 2016-04-29 16:11:51]

Description was changed from

==========
Adding support for Win+L key combo in Windows host.

This change will add a handler for the WIN+L key combination on non-home SKUs
of Windows.  When the WIN+L combo is pressed, we will call LockWorkstation.

Based on my investigation, it appears that this combo cannot be overridden and
only works when issued from a physical keyboard.  Therefore it isn't something
that can be issued remotely and our function call will not conflict with some
other behavior.  We skip the non-Home SKUs as locking the workstation doesn't
really make sense there (it brings up the switch users UI instead).

BUG=517322
==========

to

==========
Adding support for Win+L key combo in Windows host.

This change will add a handler for the WIN+L key combination on non-home SKUs
of Windows.  When the WIN+L combo is pressed, we will call LockWorkstation.

Based on my investigation, it appears that this combo cannot be overridden and
only works when issued from a physical keyboard.  Therefore it isn't something
that can be issued remotely and our function call will not conflict with some
other behavior.  We skip the Home SKUs as locking the workstation doesn't
really make sense there (it brings up the switch users UI instead).

BUG=517322
==========

[joedow, 2016-04-29 16:13:09]

Thanks!

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process.cc (right):

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:19: #include "base/win/windows_version.h"
On 2016/04/29 00:41:42, Jamie wrote:
> Does this also need an ifdef?

Done.

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:77: NOTIMPLEMENTED();
On 2016/04/29 00:41:42, Jamie wrote:
> Maybe NOTREACHED would be more appropriate here?

Done.

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:79: }
On 2016/04/28 22:00:11, Jamie wrote:
> I think this needs to factored out more cleanly into a Windows-specific
location
> (for example, I'm not sure that the new #include will work on other
platforms).
> In general, platform-specific ifdefs should be a last resort.
> 
> One option would be to have a generic shortcut-handler interface that could be
> registered for platform-specific keyboard combinations. Windows could then use
> that for Ctrl+Alt+Del and Win+L (with the advantage of moving related
> functionality closer together). Other platforms could simply not register a
> handler.
> 
> WDYT?

Acknowledged.

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/win/sessi...
File remoting/host/win/session_input_injector.h (right):

https://codereview.chromium.org/1930473003/diff/40001/remoting/host/win/sessi...
remoting/host/win/session_input_injector.h:22: // Ctrl+Alt+Del and Win + L key
combinations by invoking the given callbacks.
On 2016/04/29 00:41:42, Jamie wrote:
> Nit: No spaces around "+" for consistency with Ctrl+Alt+Del.

Done.

[joedow, 2016-04-29 16:22:30]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-04-29 16:22:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1930473003/#ps60001
(title: "Addressing CR feedback")

[commit-bot: I haz the power, 2016-04-29 16:22:39]

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

[commit-bot: I haz the power, 2016-04-29 16:51:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:26:30]

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

[commit-bot: I haz the power, 2016-04-30 18:01:04]

Description was changed from

==========
Adding support for Win+L key combo in Windows host.

This change will add a handler for the WIN+L key combination on non-home SKUs
of Windows.  When the WIN+L combo is pressed, we will call LockWorkstation.

Based on my investigation, it appears that this combo cannot be overridden and
only works when issued from a physical keyboard.  Therefore it isn't something
that can be issued remotely and our function call will not conflict with some
other behavior.  We skip the Home SKUs as locking the workstation doesn't
really make sense there (it brings up the switch users UI instead).

BUG=517322
==========

to

==========
Adding support for Win+L key combo in Windows host.

This change will add a handler for the WIN+L key combination on non-home SKUs
of Windows.  When the WIN+L combo is pressed, we will call LockWorkstation.

Based on my investigation, it appears that this combo cannot be overridden and
only works when issued from a physical keyboard.  Therefore it isn't something
that can be issued remotely and our function call will not conflict with some
other behavior.  We skip the Home SKUs as locking the workstation doesn't
really make sense there (it brings up the switch users UI instead).

BUG=517322

Committed: https://crrev.com/83eff387b6535ea4d74ed993e0dbc7cf719a71c5
Cr-Commit-Position: refs/heads/master@{#390673}
==========