The on screen keyboard on Windows 8+ should not obscure the input field.

base/win/osk_display_manager.h

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[grt (UTC plus 2), 2016/05/18 15:41:20]

nit: no "(c)" in new header
(http://www.chromium.org/developers/coding-style#TOC-File-headers)

[ananta, 2016/05/19 01:57:12]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef BASE_WIN_OSK_DISPLAY_MANAGER_H_

[grt (UTC plus 2), 2016/05/18 15:41:20]

nit: blank line before this

[ananta, 2016/05/19 01:57:12]

Done.

+#define BASE_WIN_OSK_DISPLAY_MANAGER_H_
+
+#include "base/base_export.h"
+#include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+
+namespace base {
+namespace win {
+
+// Implemented by classes who wish to get notified about the on screen keyboard
+// becoming visible/hidden.
+class BASE_EXPORT OnScreenKeyboardObserver {
+ public:
+  virtual void OnKeyboardVisible(const RECT& keyboard_rect) {}

[sky, 2016/05/18 19:54:51]

RECT->gfx::Rect for these too. Also, you should make it clear what coordinates
these are. I think it's screen pixels, in which case you might want to convert
to screen dip.

[ananta, 2016/05/19 01:57:12]

Done.

+  virtual void OnKeyboardHidden(const RECT& keyboard_rect) {}
+};
+
+// This class provides functionality to detect when the on screen keyboard
+// is displayed and move the main window up if it is obscured by the keyboard.
+class BASE_EXPORT OnScreenKeyboardDetector {

[sky, 2016/05/18 19:54:51]

Is there a reason you need this in base? I would be inclined to put it in
ui/base.

[ananta, 2016/05/19 01:57:12]

Done.

+ public:
+  OnScreenKeyboardDetector();
+  ~OnScreenKeyboardDetector();
+
+  // Runs a task which detects if the on screen keyboard was displayed.

[grt (UTC plus 2), 2016/05/18 15:41:20]

nit: "Schedules a delayed task that will..." since it doesn't actually run the
task.

[ananta, 2016/05/19 01:57:12]

Done.

+  void DetectKeyboard(HWND main_window);
+
+  // Dismisses the on screen keyboard.
+  bool DismissKeyboard();
+
+  // Add/Remove keyboard observers.
+  // Please note that this class does not track the |observer| destruction. It
+  // is upto the classes which set up these observers to remove them when they
+  // are destroyed.
+  void AddObserver(OnScreenKeyboardObserver* observer);
+  void RemoveObserver(OnScreenKeyboardObserver* observer);
+
+ private:
+  // Executes as a task and detects if the on screen keyboard is displayed.
+  // The |check_for_activation| parameter controls whether the function detects
+  // whether the keyboard wad displayed or hidden.
+  void CheckOSKState(bool check_for_activation);

[grt (UTC plus 2), 2016/05/18 15:41:20]

does this detection run periodically for the entire duration that the OSK is
visible to detect when it is hidden? if so, please document it as such.

[ananta, 2016/05/19 01:57:12]

Done.

+
+  // Notifies observers that the keyboard was displayed.

[grt (UTC plus 2), 2016/05/18 15:41:20]

please also note that a recurring task is started to detect when the OSK
disappears.

[ananta, 2016/05/19 01:57:12]

Done.

+  void HandleKeyboardVisible();
+
+  // Notifies observers that the keyboard was hidden.
+  void HandleKeyboardHidden();
+
+  // Removes all observers from the list.
+  void ClearObservers();
+
+  // The main window which displays the on screen keyboard.
+  HWND main_window_;

[grt (UTC plus 2), 2016/05/18 15:41:20]

 = nullptr;

[ananta, 2016/05/19 01:57:12]

This is initialized in the ctor.

[grt (UTC plus 2), 2016/05/19 03:51:03]

with the new C++-11 style, this sort of initialization should be in the class;
see "Non-Static Class Member Initializers" in
http://chromium-cpp.appspot.com/#core-whitelist

[ananta, 2016/05/19 19:33:37]

Done.

+
+  // Tracks if the keyboard was displayed.
+  bool osk_visible_notification_received_;

[grt (UTC plus 2), 2016/05/18 15:41:20]

 = false;

[ananta, 2016/05/19 01:57:12]

ditto

[ananta, 2016/05/19 19:33:37]

Done.

+
+  // The keyboard dimensions.
+  RECT osk_rect_;
+
+  base::ObserverList<OnScreenKeyboardObserver, true> observers_;
+
+  // Should be the last member in the class. Helps ensure that tasks spawned
+  // by this class instance are canceled when it is destroyed.
+  base::WeakPtrFactory<OnScreenKeyboardDetector> keyboard_detector_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(OnScreenKeyboardDetector);
+};
+
+// This class provides functionality to display the on screen keyboard on
+// Windows 8+. It ensures that the content on the page is scrolled if the

[grt (UTC plus 2), 2016/05/18 15:41:20]

how does this do the scrolling? is that the responsibility of the observer?

[ananta, 2016/05/19 01:57:12]

Thanks it does not. I started out with a impl which did the scrolling and then
moved that to the observers. The comment was stale.

+// keyboard obscures it.
+class BASE_EXPORT OnScreenKeyboardDisplayManager {
+ public:
+  static OnScreenKeyboardDisplayManager* GetInstance();
+
+  // Functions to display and dismiss the keyboard.
+  // The |observer| parameters allow the caller to optionally set up
+  // notification observers for detecting when the keyboard is displayed
+  // hidden etc.
+  bool DisplayVirtualKeyboard(OnScreenKeyboardObserver* observer);
+  bool DismissVirtualKeyboard(OnScreenKeyboardObserver* observer);
+
+ private:
+  OnScreenKeyboardDisplayManager() {}

[grt (UTC plus 2), 2016/05/18 15:41:20]

nit: = default;

[ananta, 2016/05/19 01:57:12]

Please clarify what you mean here.

[grt (UTC plus 2), 2016/05/19 03:51:02]

Gangam style! uh, i mean C++-11 style:
  OnScreenKeyboardDisplayManager() = default;

[sky, 2016/05/19 16:05:25]

I'm curious here as well as it has come up in other reviews. What benefit do we
get by using '= default' of {} here?

[grt (UTC plus 2), 2016/05/19 16:53:17]

My reason for suggesting it is so that there's just one way we do things in the
code. I don't know if there's a compelling reason such as better codegen from
the compiler or something. Perhaps Dana or one of the other C++-11 gurus would
know. I don't feel strongly either way.

[ananta, 2016/05/19 19:33:37]

I left it as is as clang needs the destructor to be in the cc file. It seems
better to have the ctor and dtor together.

+
+  friend struct base::StaticMemorySingletonTraits<
+      OnScreenKeyboardDisplayManager>;
+
+  std::unique_ptr<OnScreenKeyboardDetector> keyboard_detector_;
+
+  DISALLOW_COPY_AND_ASSIGN(OnScreenKeyboardDisplayManager);
+};
+
+}  // namespace win
+}  // namespace base
+
+#endif  // BASE_WIN_OSK_DISPLAY_MANAGER_H_