Improve tablet mode detection on Windows 10+

Improve tablet mode detection on Windows 10+

This patch introduces the following:-
1. A function IsWindows10TabletMode which uses the new WRL API's on Windows 10 to query the current interaction mode.
   As per msdn these API's can be called from Win32 apps.
   https://msdn.microsoft.com/en-us/library/windows/hardware/dn917883(v=vs.85).aspx
   https://software.intel.com/en-us/articles/detecting-slateclamshell-mode-screen-orientation-in-convertible-pc
   These API's however appear to always return UserModeTouch which indicates tablet mode on the Surface 4 configuration
   I was testing with.

2. If the IsTabletDevice function returns true we don't attempt to detect keyboards and just assume that there is
   no keyboard attached.

BUG=497381
Committed: https://crrev.com/d67aae27c1137d077f228d27cae2337d2b8caefa
Cr-Commit-Position: refs/heads/master@{#380864}

[ananta, 2016-03-11 01:43:24]

ananta@chromium.org changed reviewers:
+ jschuh@chromium.org

[jschuh, 2016-03-11 22:01:45]

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:116: // These definitions need to be removed once we have
the new headers.
How close are we to landing the Win10 SDK? Because I'd prefer to avoid this if
possible.

If we can't avoid it, then please add a TODO with a bug and put this behind an
"#if VER_PRODUCTBUILD > X" macro and a "#pragma message()" in the else case so
we notice that the version rolled forward.

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:158: typedef HRESULT (WINAPI* GetActivationFactory)(HSTRING
class_id,
ROGetActivationFactoryFunction

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:162: typedef HRESULT (WINAPI* CreateString)(LPCWSTR source,
WindowsCreateStringFunction

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:193: static HSTRING view_settings_guid = NULL;
Is there a race here where you could allocate the string more than once?

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:197: wcslen(L"Windows.UI.ViewManagement.UIViewSettings")),
Declare this as a static constant so you don't have to repeat it, and use
base::arraysize() - 1 to get the length.

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:224: view_settings->get_UserInteractionMode(&mode);
Are we sure the overhead is acceptable in doing this on every gesture event? It
seems like we should really just be setting the tablet/keyboard state at at key
times like start, focus change, or when we get state change events. And then we
should globally track that state.

[Will Harris, 2016-03-11 22:05:06]

drive by from the peanut gallery

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:116: // These definitions need to be removed once we have
the new headers.
On 2016/03/11 22:01:44, jschuh (very slow) wrote:
> How close are we to landing the Win10 SDK? Because I'd prefer to avoid this if
> possible.
> 
> If we can't avoid it, then please add a TODO with a bug and put this behind an
> "#if VER_PRODUCTBUILD > X" macro and a "#pragma message()" in the else case so
> we notice that the version rolled forward.

I thought we had landed Win10 SDK since it was needed for the new process
mitigations.

SDK in my build dir is 10.0.10586.0 - is that not recent enough?

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:158: typedef HRESULT (WINAPI* GetActivationFactory)(HSTRING
class_id,
On 2016/03/11 22:01:45, jschuh (very slow) wrote:
> ROGetActivationFactoryFunction

consider using typedef decltype if the prototype is declared in one of the
headers.

[ananta, 2016-03-11 23:14:15]

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:116: // These definitions need to be removed once we have
the new headers.
On 2016/03/11 22:01:44, jschuh (very slow) wrote:
> How close are we to landing the Win10 SDK? Because I'd prefer to avoid this if
> possible.
> 
> If we can't avoid it, then please add a TODO with a bug and put this behind an
> "#if VER_PRODUCTBUILD > X" macro and a "#pragma message()" in the else case so
> we notice that the version rolled forward.

Removed these definitions. Thanks to Will for letting me know that we can use
the win10 sdk includes

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:158: typedef HRESULT (WINAPI* GetActivationFactory)(HSTRING
class_id,
On 2016/03/11 22:05:06, Will Harris wrote:
> On 2016/03/11 22:01:45, jschuh (very slow) wrote:
> > ROGetActivationFactoryFunction
> 
> consider using typedef decltype if the prototype is declared in one of the
> headers.

Thanks. Removed these

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:162: typedef HRESULT (WINAPI* CreateString)(LPCWSTR source,
On 2016/03/11 22:01:45, jschuh (very slow) wrote:
> WindowsCreateStringFunction

Done

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:193: static HSTRING view_settings_guid = NULL;
On 2016/03/11 22:01:45, jschuh (very slow) wrote:
> Is there a race here where you could allocate the string more than once?

No. This only gets called from the UI thread

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:197: wcslen(L"Windows.UI.ViewManagement.UIViewSettings")),
On 2016/03/11 22:01:45, jschuh (very slow) wrote:
> Declare this as a static constant so you don't have to repeat it, and use
> base::arraysize() - 1 to get the length.

We use the defined constant from the include file now

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:224: view_settings->get_UserInteractionMode(&mode);
On 2016/03/11 22:01:45, jschuh (very slow) wrote:
> Are we sure the overhead is acceptable in doing this on every gesture event?
It
> seems like we should really just be setting the tablet/keyboard state at at
key
> times like start, focus change, or when we get state change events. And then
we
> should globally track that state.

There should not be much overhead here. The functions just check a state
variable to determine the mode. I am not worried about the cost of the QI's etc.

[jschuh, 2016-03-11 23:50:26]

One meta question: do we have any way at all to test this?

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/1783123003/diff/20001/base/win/win_util.cc#ne...
base/win/win_util.cc:224: view_settings->get_UserInteractionMode(&mode);
On 2016/03/11 23:14:14, ananta wrote:
> On 2016/03/11 22:01:45, jschuh (very slow) wrote:
> > Are we sure the overhead is acceptable in doing this on every gesture event?
> It
> > seems like we should really just be setting the tablet/keyboard state at at
> key
> > times like start, focus change, or when we get state change events. And then
> we
> > should globally track that state.
> 
> There should not be much overhead here. The functions just check a state
> variable to determine the mode. I am not worried about the cost of the QI's
etc.

Okay. It just seems like we keep piling more logic in this call stack when all
we really want is a boolean that's tied to state changes. How about filing a bug
for someone to refactor it that way, if we ever get around to it?

https://codereview.chromium.org/1783123003/diff/40001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/1783123003/diff/40001/base/win/win_util.cc#ne...
base/win/win_util.cc:126: typedef decltype(&RoGetActivationFactory)
RoGetActivationFactoryFunction;
The style guide now says to do it this way:
using RoGetActivationFactoryFunction = decltype(&RoGetActivationFactory);

https://codereview.chromium.org/1783123003/diff/40001/base/win/win_util.cc#ne...
base/win/win_util.cc:127: typedef decltype(&WindowsCreateString)
WindowsCreateStringFunction;
Same as above.

[ananta, 2016-03-12 00:27:59]

Currently there is no way to test this functionality. I think we need to
seriously look into exposing this behavior via accessibility and rely on Windows
to display the keyboard.

That for a later patchset and a bigger discussion. 

I logged bug 594297 for the refactor work.

https://codereview.chromium.org/1783123003/diff/40001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/1783123003/diff/40001/base/win/win_util.cc#ne...
base/win/win_util.cc:126: typedef decltype(&RoGetActivationFactory)
RoGetActivationFactoryFunction;
On 2016/03/11 23:50:26, jschuh (very slow) wrote:
> The style guide now says to do it this way:
> using RoGetActivationFactoryFunction = decltype(&RoGetActivationFactory);

Done.

https://codereview.chromium.org/1783123003/diff/40001/base/win/win_util.cc#ne...
base/win/win_util.cc:127: typedef decltype(&WindowsCreateString)
WindowsCreateStringFunction;
On 2016/03/11 23:50:26, jschuh (very slow) wrote:
> Same as above.

Done.

[jschuh, 2016-03-12 00:37:11]

lgtm

[ananta, 2016-03-12 01:06:13]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2016-03-12 01:06:47]

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

[commit-bot: I haz the power, 2016-03-12 04:58:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-12 04:59:12]

Description was changed from

==========
Improve tablet mode detection on Windows 10+

This patch introduces the following:-
1. A function IsWindows10TabletMode which uses the new WRL API's on Windows 10
to query the current interaction mode.
   As per msdn these API's can be called from Win32 apps.
  
https://msdn.microsoft.com/en-us/library/windows/hardware/dn917883(v=vs.85).aspx
  
https://software.intel.com/en-us/articles/detecting-slateclamshell-mode-scree...
   These API's however appear to always return UserModeTouch which indicates
tablet mode on the Surface 4 configuration
   I was testing with.

2. If the IsTabletDevice function returns true we don't attempt to detect
keyboards and just assume that there is
   no keyboard attached.

BUG=497381
==========

to

==========
Improve tablet mode detection on Windows 10+

This patch introduces the following:-
1. A function IsWindows10TabletMode which uses the new WRL API's on Windows 10
to query the current interaction mode.
   As per msdn these API's can be called from Win32 apps.
  
https://msdn.microsoft.com/en-us/library/windows/hardware/dn917883(v=vs.85).aspx
  
https://software.intel.com/en-us/articles/detecting-slateclamshell-mode-scree...
   These API's however appear to always return UserModeTouch which indicates
tablet mode on the Surface 4 configuration
   I was testing with.

2. If the IsTabletDevice function returns true we don't attempt to detect
keyboards and just assume that there is
   no keyboard attached.

BUG=497381

Committed: https://crrev.com/d67aae27c1137d077f228d27cae2337d2b8caefa
Cr-Commit-Position: refs/heads/master@{#380864}
==========

[commit-bot: I haz the power, 2016-03-12 04:59:12]

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