Detect if osk is visible on windows

Detect if osk is visible on windows

BUG=733710
Review-Url: https://codereview.chromium.org/2971363002
Cr-Commit-Position: refs/heads/master@{#488244}
Committed: https://chromium.googlesource.com/chromium/src/+/931654e2e2b4c27ff7f2234fb3115f4ad2eb1cd3

[girard, 2017-07-07 19:57:22]

girard@chromium.org changed reviewers:
+ pkasting@chromium.org, sadrul@chromium.org

[girard, 2017-07-07 19:57:23]

sadrul@chromium.org: Please review changes in osk_display_manager.*

pkasting@chromium.org: Please review changes in location_bar_view.cc

Checks osk visibility, which determines if we should display the "cancel all
edits" button in the omnibox.

No tests, since this is an os-level call and I can't see any reasonable way to
mock or evaluate it. Thoughts/suggestions?

[Peter Kasting, 2017-07-08 02:19:31]

https://codereview.chromium.org/2971363002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2971363002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:749: return
ui::OnScreenKeyboardDisplayManager::GetInstance()->IsKeyboardVisible();
Should we just make Aura's keyboard::IsKeyboardVisible() call through to this?

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
File ui/base/win/osk_display_manager.cc (right):

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
ui/base/win/osk_display_manager.cc:363: bool
OnScreenKeyboardDisplayManager::IsKeyboardVisible() {
This file already has CheckIfKeyboardVisible() above, which looks like a
stricter version of this check.  Having both these is confusing.  Can we unify? 
If not, can we make it really clear why these need to differ?

(I found this because I was checking to see whether other functions could
replace calls to e.g. IsWindowEnabled() with calls to this function, to which my
answer is "maybe".)

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
File ui/base/win/osk_display_manager.h (right):

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
ui/base/win/osk_display_manager.h:45: bool IsKeyboardVisible();
Nit: const?

[girard, 2017-07-10 20:23:51]

New patch based on pkasting's feedback - thanks

https://codereview.chromium.org/2971363002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2971363002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:749: return
ui::OnScreenKeyboardDisplayManager::GetInstance()->IsKeyboardVisible();
On 2017/07/08 02:19:31, Peter Kasting wrote:
> Should we just make Aura's keyboard::IsKeyboardVisible() call through to this?

I thought about this, but I'm worried about all the other functions in Aura's
keyboard:: namespace (seems wrong to make some of them cross-platform but not
others.)

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
File ui/base/win/osk_display_manager.cc (right):

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
ui/base/win/osk_display_manager.cc:363: bool
OnScreenKeyboardDisplayManager::IsKeyboardVisible() {
On 2017/07/08 02:19:31, Peter Kasting wrote:
> This file already has CheckIfKeyboardVisible() above, which looks like a
> stricter version of this check.  Having both these is confusing.  Can we
unify? 
> If not, can we make it really clear why these need to differ?
> 
> (I found this because I was checking to see whether other functions could
> replace calls to e.g. IsWindowEnabled() with calls to this function, to which
my
> answer is "maybe".)

Patchset 2 contains this fix. The refactor was a bit strange because of the
function's side-effects, but I think the new code is cleaner.

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
File ui/base/win/osk_display_manager.h (right):

https://codereview.chromium.org/2971363002/diff/1/ui/base/win/osk_display_man...
ui/base/win/osk_display_manager.h:45: bool IsKeyboardVisible();
On 2017/07/08 02:19:31, Peter Kasting wrote:
> Nit: const?

Done.

[Peter Kasting, 2017-07-10 21:10:12]

LGTM

https://codereview.chromium.org/2971363002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2971363002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:749: return
ui::OnScreenKeyboardDisplayManager::GetInstance()->IsKeyboardVisible();
On 2017/07/10 20:23:51, girard wrote:
> On 2017/07/08 02:19:31, Peter Kasting wrote:
> > Should we just make Aura's keyboard::IsKeyboardVisible() call through to
this?
> 
> I thought about this, but I'm worried about all the other functions in Aura's
> keyboard:: namespace (seems wrong to make some of them cross-platform but not
> others.)

I'm not knowledgeable about that space.  Can you consult with a relevant Aura
OWNER about that?  Would be nice to clearly understand (and state in comments?)
the policies around that stuff.

[sadrul, 2017-07-10 21:50:35]

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

[sadrul, 2017-07-10 21:50:36]

+ananta@ since looks like he wrote the code.

[ananta, 2017-07-12 18:42:04]

lgtm

[sadrul, 2017-07-19 20:56:01]

lgtm

[sadrul, 2017-07-19 20:56:59]

On 2017/07/19 20:56:01, sadrul wrote:
> lgtm

(sorry about the delay, I missed reviews on rietveld over the last ~week)

[girard, 2017-07-20 13:44:40]

The CQ bit was checked by girard@chromium.org

[commit-bot: I haz the power, 2017-07-20 13:44:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-20 14:54:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1500558280190260,
"parent_rev": "692035c3b67ed1fa9176dd83624d448a04569fac", "commit_rev":
"931654e2e2b4c27ff7f2234fb3115f4ad2eb1cd3"}

[commit-bot: I haz the power, 2017-07-20 14:55:15]

Description was changed from

==========
Detect if osk is visible on windows

BUG=733710
==========

to

==========
Detect if osk is visible on windows

BUG=733710

Review-Url: https://codereview.chromium.org/2971363002
Cr-Commit-Position: refs/heads/master@{#488244}
Committed:
https://chromium.googlesource.com/chromium/src/+/931654e2e2b4c27ff7f2234fb311...
==========

[commit-bot: I haz the power, 2017-07-20 14:55:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/931654e2e2b4c27ff7f2234fb311...