The DisplayVirtualKeyboard function on Windows 8 and beyond should not be displaying the OSK if a p…

The DisplayVirtualKeyboard function on Windows 8 and beyond should not be displaying the OSK if a physical keyboard is attached to the machine.

Added a function IsKeyboardPresent locally to win_util.cc which uses the setup API's to enumerate
keyboard devices and check for the common ones like ACPI\PNP and HID\VID. This function returns
true if the number of keyboards is greater than 1. This is a hacky approach and is being done
for lack of a good way to determine the presence of a keyboard. The documented methods don't work
well.

BUG=335735
Committed: https://crrev.com/74ca738f038557de0e53fd91a8d15774da36f85a
Cr-Commit-Position: refs/heads/master@{#300383}

[ananta, 2014-10-16 00:54:09]

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

[cpu_(ooo_6.6-7.5), 2014-10-16 18:03:19]

cpu@chromium.org changed reviewers:
+ scottmg@chromium.org

[cpu_(ooo_6.6-7.5), 2014-10-16 18:11:33]

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

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode63
base/win/win_util.cc:63: bool IsKeyboardPresent() {
please rename this to IsKeyboardPresentOnSlate()  or .. OnTablet()

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode97
base/win/win_util.cc:97: // Get the device ID
period at the end of comments.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode98
base/win/win_util.cc:98: WCHAR device_id[MAX_DEVICE_ID_LEN];
WCHAR --> wchar_t

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode107
base/win/win_util.cc:107: !base::strncmp16(L"HID\\VID", device_id,
wcslen(L"HID\\VID"))) {
use StartsWith( and set it to be case insensitive?

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode113
base/win/win_util.cc:113: }
comment explaining why > 1.

[scottmg, 2014-10-16 18:30:46]

build files lgtm

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

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode90
base/win/win_util.cc:90: bool has_keyboard = false;
this is unused

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode91
base/win/win_util.cc:91: int32 keyboard_count = 0;
just int

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode96
base/win/win_util.cc:96: break;
maybe continue?

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode111
base/win/win_util.cc:111: break;
maybe no else to check any others?

[ananta, 2014-10-16 19:35:59]

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

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode63
base/win/win_util.cc:63: bool IsKeyboardPresent() {
On 2014/10/16 18:11:33, cpu wrote:
> please rename this to IsKeyboardPresentOnSlate()  or .. OnTablet()

Done.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode90
base/win/win_util.cc:90: bool has_keyboard = false;
On 2014/10/16 18:30:46, scottmg wrote:
> this is unused

Done.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode91
base/win/win_util.cc:91: int32 keyboard_count = 0;
On 2014/10/16 18:30:46, scottmg wrote:
> just int

Done.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode96
base/win/win_util.cc:96: break;
On 2014/10/16 18:30:46, scottmg wrote:
> maybe continue?

The loop above continues indefinitely. That would need to change for continue.
leaving as is

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode97
base/win/win_util.cc:97: // Get the device ID
On 2014/10/16 18:11:33, cpu wrote:
> period at the end of comments.

Done.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode98
base/win/win_util.cc:98: WCHAR device_id[MAX_DEVICE_ID_LEN];
On 2014/10/16 18:11:33, cpu wrote:
> WCHAR --> wchar_t

Done.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode107
base/win/win_util.cc:107: !base::strncmp16(L"HID\\VID", device_id,
wcslen(L"HID\\VID"))) {
On 2014/10/16 18:11:33, cpu wrote:
> use StartsWith( and set it to be case insensitive?

Done.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode111
base/win/win_util.cc:111: break;
On 2014/10/16 18:30:46, scottmg wrote:
> maybe no else to check any others?

Good point. Done.

https://codereview.chromium.org/644783005/diff/1/base/win/win_util.cc#newcode113
base/win/win_util.cc:113: }
On 2014/10/16 18:11:33, cpu wrote:
> comment explaining why > 1.

Done.

[cpu_(ooo_6.6-7.5), 2014-10-17 18:18:33]

lgtm

[ananta, 2014-10-20 18:28:59]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:31:09]

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

[commit-bot: I haz the power, 2014-10-20 18:39:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:39:31]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ananta, 2014-10-20 21:35:18]

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

[ananta, 2014-10-20 21:35:42]

+thestig for base build changes and owners stamp

[Lei Zhang, 2014-10-20 21:53:15]

lgtm with some nitty gritty comments.

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

https://codereview.chromium.org/644783005/diff/60001/base/win/win_util.cc#new...
base/win/win_util.cc:65: if (base::win::GetVersion() < base::win::VERSION_WIN8)
{
Since this function is internal to win_util.cc, there should be very few callers
and they should get it right. Just make this a DCHECK instead of trying to
handle it? See philosophy here:
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

https://codereview.chromium.org/644783005/diff/60001/base/win/win_util.cc#new...
base/win/win_util.cc:90: for (int i = 0;; ++i) {
Does the compiler care about int vs DWORD?

https://codereview.chromium.org/644783005/diff/60001/base/win/win_util.cc#new...
base/win/win_util.cc:92: device_info_data.cbSize = sizeof (device_info_data);
nit: sizeof(

[ananta, 2014-10-20 22:00:13]

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

https://codereview.chromium.org/644783005/diff/60001/base/win/win_util.cc#new...
base/win/win_util.cc:65: if (base::win::GetVersion() < base::win::VERSION_WIN8)
{
On 2014/10/20 21:53:15, Lei Zhang wrote:
> Since this function is internal to win_util.cc, there should be very few
callers
> and they should get it right. Just make this a DCHECK instead of trying to
> handle it? See philosophy here:
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

Done.

https://codereview.chromium.org/644783005/diff/60001/base/win/win_util.cc#new...
base/win/win_util.cc:90: for (int i = 0;; ++i) {
On 2014/10/20 21:53:15, Lei Zhang wrote:
> Does the compiler care about int vs DWORD?

They are the same size. Changed for correctness to DWORD.

https://codereview.chromium.org/644783005/diff/60001/base/win/win_util.cc#new...
base/win/win_util.cc:92: device_info_data.cbSize = sizeof (device_info_data);
On 2014/10/20 21:53:15, Lei Zhang wrote:
> nit: sizeof(

Done.

[ananta, 2014-10-20 22:07:13]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-10-20 22:08:47]

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

[commit-bot: I haz the power, 2014-10-21 00:15:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-21 00:15:53]

Patchset 6 (id:??) landed as
https://crrev.com/74ca738f038557de0e53fd91a8d15774da36f85a
Cr-Commit-Position: refs/heads/master@{#300383}

[iueweb.webmanager, 2015-01-26 15:33:27]

On 2014/10/21 00:15:53, I haz the power (commit-bot) wrote:
> Patchset 6 (id:??) landed as
> https://crrev.com/74ca738f038557de0e53fd91a8d15774da36f85a
> Cr-Commit-Position: refs/heads/master@{#300383}

Please re-open this bug or re-examine fix if possible; there are still people,
myself included, experiencing the OSK popping up with external keyboard attached
(see https://code.google.com/p/chromium/issues/detail?id=335735).  I'm on a
Surface Pro 3, Chrome version 40.0.2214.91 m.  Thanks for any help!

[Lei Zhang, 2015-01-26 20:09:45]

On 2015/01/26 15:33:27, iueweb.webmanager wrote:
> On 2014/10/21 00:15:53, I haz the power (commit-bot) wrote:
> > Patchset 6 (id:??) landed as
> > https://crrev.com/74ca738f038557de0e53fd91a8d15774da36f85a
> > Cr-Commit-Position: refs/heads/master@{#300383}
> 
> Please re-open this bug or re-examine fix if possible; there are still people,
> myself included, experiencing the OSK popping up with external keyboard
attached
> (see https://code.google.com/p/chromium/issues/detail?id=335735).  I'm on a
> Surface Pro 3, Chrome version 40.0.2214.91 m.  Thanks for any help!

The bug you referenced is open. You already commented on the bug. Further
comments outside of the bug tracker is not helpful.