ChromeKeyboardUI: Fix method order and other cleanup

Bug: 843332
Change-Id: Iff950164803f31d013a8a80fdded73f1e7f3bd7e
Reviewed-on: https://chromium-review.googlesource.com/1182411
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584846}

chrome/browser/ui/ash/chrome_keyboard_ui.cc

@@ -67,42 +67,6 @@ GURL& GetOverrideVirtualKeyboardUrl() {
   return *url;
 }
 
-class WindowBoundsChangeObserver : public aura::WindowObserver {
- public:
-  explicit WindowBoundsChangeObserver(ChromeKeyboardUI* ui) : ui_(ui) {}
-  ~WindowBoundsChangeObserver() override {}
-
-  void AddObservedWindow(aura::Window* window) {
-    if (!window->HasObserver(this)) {
-      window->AddObserver(this);
-      observed_windows_.insert(window);
-    }
-  }
-  void RemoveAllObservedWindows() {
-    for (aura::Window* window : observed_windows_)
-      window->RemoveObserver(this);
-    observed_windows_.clear();
-  }
-
- private:
-  void OnWindowBoundsChanged(aura::Window* window,
-                             const gfx::Rect& old_bounds,
-                             const gfx::Rect& new_bounds,
-                             ui::PropertyChangeReason reason) override {
-    ui_->UpdateInsetsForWindow(window);
-  }
-  void OnWindowDestroyed(aura::Window* window) override {
-    if (window->HasObserver(this))
-      window->RemoveObserver(this);
-    observed_windows_.erase(window);
-  }
-
-  ChromeKeyboardUI* const ui_;
-  std::set<aura::Window*> observed_windows_;
-
-  DISALLOW_COPY_AND_ASSIGN(WindowBoundsChangeObserver);
-};
-
 // The WebContentsDelegate for the chrome keyboard.
 // The delegate deletes itself when the keyboard is destroyed.
 class ChromeKeyboardContentsDelegate : public content::WebContentsDelegate,
@@ -204,7 +168,7 @@ class AshKeyboardControllerObserver
  public:
   explicit AshKeyboardControllerObserver(content::BrowserContext* context)
       : context_(context) {}
-  ~AshKeyboardControllerObserver() override {}
+  ~AshKeyboardControllerObserver() override = default;
 
   // KeyboardControllerObserver:
   void OnKeyboardVisibleBoundsChanged(const gfx::Rect& bounds) override {
@@ -260,6 +224,45 @@ class AshKeyboardControllerObserver
 
 }  // namespace
 
+class ChromeKeyboardUI::WindowBoundsChangeObserver
+    : public aura::WindowObserver {
+ public:
+  explicit WindowBoundsChangeObserver(ChromeKeyboardUI* ui) : ui_(ui) {}
+  ~WindowBoundsChangeObserver() override {}
+
+  void AddObservedWindow(aura::Window* window) {
+    if (!window->HasObserver(this)) {
+      window->AddObserver(this);
+      observed_windows_.insert(window);
+    }
+  }
+
+  void RemoveAllObservedWindows() {
+    for (aura::Window* window : observed_windows_)
+      window->RemoveObserver(this);
+    observed_windows_.clear();
+  }
+
+ private:
+  void OnWindowBoundsChanged(aura::Window* window,
+                             const gfx::Rect& old_bounds,
+                             const gfx::Rect& new_bounds,
+                             ui::PropertyChangeReason reason) override {
+    ui_->UpdateInsetsForWindow(window);
+  }
+
+  void OnWindowDestroyed(aura::Window* window) override {
+    if (window->HasObserver(this))
+      window->RemoveObserver(this);
+    observed_windows_.erase(window);
+  }
+
+  ChromeKeyboardUI* const ui_;
+  std::set<aura::Window*> observed_windows_;
+
+  DISALLOW_COPY_AND_ASSIGN(WindowBoundsChangeObserver);
+};
+
 void ChromeKeyboardUI::TestApi::SetOverrideVirtualKeyboardUrl(const GURL& url) {
   GURL& override_url = GetOverrideVirtualKeyboardUrl();
   override_url = url;
@@ -276,7 +279,7 @@ ChromeKeyboardUI::~ChromeKeyboardUI() {
 }
 
 void ChromeKeyboardUI::UpdateInsetsForWindow(aura::Window* window) {
-  if (!ShouldWindowOverscroll(window) || !HasKeyboardWindow())
+  if (!HasKeyboardWindow() || !ShouldWindowOverscroll(window))
     return;
 
   std::unique_ptr<content::RenderWidgetHostIterator> widgets(
@@ -298,17 +301,20 @@ void ChromeKeyboardUI::UpdateInsetsForWindow(aura::Window* window) {
 }
 
 aura::Window* ChromeKeyboardUI::GetKeyboardWindow() {
-  if (!keyboard_contents_) {
+  if (keyboard_contents_)
+    return keyboard_contents_->GetNativeView();
+
   GURL keyboard_url = GetVirtualKeyboardUrl();
   content::WebContents::CreateParams web_contents_params(
-        browser_context(),
-        content::SiteInstance::CreateForURL(browser_context(), keyboard_url));
+      browser_context_,
+      content::SiteInstance::CreateForURL(browser_context_, keyboard_url));
   keyboard_contents_ = content::WebContents::Create(web_contents_params);
   keyboard_contents_->SetDelegate(new ChromeKeyboardContentsDelegate());
   SetupWebContents(keyboard_contents_.get());
   LoadContents(keyboard_url);
-    keyboard_contents_->GetNativeView()->AddObserver(this);
-    keyboard_contents_->GetNativeView()->set_owned_by_parent(false);
+  aura::Window* keyboard_window = keyboard_contents_->GetNativeView();
+  keyboard_window->AddObserver(this);
+  keyboard_window->set_owned_by_parent(false);
   content::RenderWidgetHostView* view =
       keyboard_contents_->GetMainFrame()->GetView();
 
@@ -326,35 +332,34 @@ aura::Window* ChromeKeyboardUI::GetKeyboardWindow() {
   // By default, layers in WebContents are clipped at the window bounds,
   // but this causes the shadows to be clipped too, so clipping needs to
   // be disabled.
-    keyboard_contents_->GetNativeView()->layer()->SetMasksToBounds(false);
+  keyboard_window->layer()->SetMasksToBounds(false);
 
-    keyboard_contents_->GetNativeView()->SetProperty(
-        ui::kAXRoleOverride, ax::mojom::Role::kKeyboard);
-  }
+  keyboard_window->SetProperty(ui::kAXRoleOverride, ax::mojom::Role::kKeyboard);
 
-  return keyboard_contents_->GetNativeView();
+  return keyboard_window;
 }
 
 bool ChromeKeyboardUI::HasKeyboardWindow() const {
   return !!keyboard_contents_;
 }
 
-bool ChromeKeyboardUI::ShouldWindowOverscroll(aura::Window* window) const {
-  aura::Window* root_window = window->GetRootWindow();
-  if (!root_window)
-    return true;
-
-  if (root_window != GetKeyboardRootWindow())
-    return false;
+ui::InputMethod* ChromeKeyboardUI::GetInputMethod() {
+  aura::Window* root_window = ash::Shell::GetRootWindowForNewWindows();
+  DCHECK(root_window);
+  return root_window->GetHost()->GetInputMethod();
+}
 
-  ash::RootWindowController* root_window_controller =
-      ash::RootWindowController::ForWindow(root_window);
-  // Shell ime window container contains virtual keyboard windows and IME
-  // windows(IME windows are created by chrome.app.window.create api). They
-  // should never be overscrolled.
-  return !root_window_controller
-              ->GetContainer(ash::kShellWindowId_ImeWindowParentContainer)
-              ->Contains(window);
+void ChromeKeyboardUI::SetController(keyboard::KeyboardController* controller) {
+  // During KeyboardController destruction, controller can be set to null.
+  if (!controller) {
+    DCHECK(keyboard_controller());
+    keyboard_controller()->RemoveObserver(observer_.get());
+    KeyboardUI::SetController(nullptr);
+    return;
+  }
+  KeyboardUI::SetController(controller);
+  observer_ = std::make_unique<AshKeyboardControllerObserver>(browser_context_);
+  keyboard_controller()->AddObserver(observer_.get());
 }
 
 void ChromeKeyboardUI::ReloadKeyboardIfNeeded() {
@@ -383,17 +388,21 @@ void ChromeKeyboardUI::InitInsets(const gfx::Rect& new_bounds) {
       content::RenderWidgetHost::GetRenderWidgetHosts());
   while (content::RenderWidgetHost* widget = widgets->GetNextHost()) {
     content::RenderWidgetHostView* view = widget->GetView();
-    // Can be NULL, e.g. if the RenderWidget is being destroyed or
+    // Can be null, e.g. if the RenderWidget is being destroyed or
     // the render process crashed.
-    if (view) {
+    if (!view)
+      continue;
+
     aura::Window* window = view->GetNativeView();
     // Added while we determine if RenderWidgetHostViewChildFrame can be
-      // changed to always return a non-null value: https://crbug.com/644726 .
+    // changed to always return a non-null value: https://crbug.com/644726.
     // If we cannot guarantee a non-null value, then this may need to stay.
     if (!window)
       continue;
 
-      if (ShouldWindowOverscroll(window)) {
+    if (!ShouldWindowOverscroll(window))
+      continue;
+
     gfx::Rect view_bounds = view->GetViewBounds();
     gfx::Rect intersect = gfx::IntersectRects(view_bounds, new_bounds);
     int overlap = intersect.height();
@@ -403,8 +412,6 @@ void ChromeKeyboardUI::InitInsets(const gfx::Rect& new_bounds) {
       view->SetInsets(gfx::Insets());
     AddBoundsChangedObserver(window);
   }
-    }
-  }
 }
 
 void ChromeKeyboardUI::ResetInsets() {
@@ -435,12 +442,6 @@ void ChromeKeyboardUI::OnWindowParentChanged(aura::Window* window,
   SetShadowAroundKeyboard();
 }
 
-const aura::Window* ChromeKeyboardUI::GetKeyboardRootWindow() const {
-  return keyboard_contents_
-             ? keyboard_contents_->GetNativeView()->GetRootWindow()
-             : nullptr;
-}
-
 void ChromeKeyboardUI::LoadContents(const GURL& url) {
   if (keyboard_contents_) {
     TRACE_EVENT0("vk", "LoadContents");
@@ -480,6 +481,28 @@ bool ChromeKeyboardUI::ShouldEnableInsets(aura::Window* window) {
           keyboard_controller()->IsKeyboardVisible());
 }
 
+bool ChromeKeyboardUI::ShouldWindowOverscroll(aura::Window* window) {
+  aura::Window* root_window = window->GetRootWindow();
+  if (!root_window)
+    return true;
+
+  if (!HasKeyboardWindow())
+    return false;
+
+  aura::Window* keyboard_window = GetKeyboardWindow();
+  if (root_window != keyboard_window->GetRootWindow())
+    return false;
+
+  ash::RootWindowController* root_window_controller =
+      ash::RootWindowController::ForWindow(root_window);
+  // Shell ime window container contains virtual keyboard windows and IME
+  // windows (IME windows are created by chrome.app.window.create api). They
+  // should never be overscrolled.
+  return !root_window_controller
+              ->GetContainer(ash::kShellWindowId_ImeWindowParentContainer)
+              ->Contains(window);
+}
+
 void ChromeKeyboardUI::AddBoundsChangedObserver(aura::Window* window) {
   aura::Window* target_window = window ? window->GetToplevelWindow() : nullptr;
   if (target_window)
@@ -487,7 +510,8 @@ void ChromeKeyboardUI::AddBoundsChangedObserver(aura::Window* window) {
 }
 
 void ChromeKeyboardUI::SetShadowAroundKeyboard() {
-  aura::Window* contents_window = keyboard_contents_->GetNativeView();
+  DCHECK(HasKeyboardWindow());
+  aura::Window* contents_window = GetKeyboardWindow();
   if (!shadow_) {
     shadow_ = std::make_unique<ui::Shadow>();
     shadow_->Init(kShadowElevationVirtualKeyboard);
@@ -514,30 +538,10 @@ void ChromeKeyboardUI::SetupWebContents(content::WebContents* contents) {
   Observe(contents);
 }
 
-ui::InputMethod* ChromeKeyboardUI::GetInputMethod() {
-  aura::Window* root_window = ash::Shell::GetRootWindowForNewWindows();
-  DCHECK(root_window);
-  return root_window->GetHost()->GetInputMethod();
-}
-
-void ChromeKeyboardUI::SetController(keyboard::KeyboardController* controller) {
-  // During KeyboardController destruction, controller can be set to null.
-  if (!controller) {
-    DCHECK(keyboard_controller());
-    keyboard_controller()->RemoveObserver(observer_.get());
-    KeyboardUI::SetController(nullptr);
-    return;
-  }
-  KeyboardUI::SetController(controller);
-  observer_ =
-      std::make_unique<AshKeyboardControllerObserver>(browser_context());
-  keyboard_controller()->AddObserver(observer_.get());
-}
-
 void ChromeKeyboardUI::RenderViewCreated(
     content::RenderViewHost* render_view_host) {
   content::HostZoomMap* zoom_map =
-      content::HostZoomMap::GetDefaultForBrowserContext(browser_context());
+      content::HostZoomMap::GetDefaultForBrowserContext(browser_context_);
   DCHECK(zoom_map);
   int render_process_id = render_view_host->GetProcess()->GetID();
   int render_view_id = render_view_host->GetRoutingID();

chrome/browser/ui/ash/chrome_keyboard_ui.h

@@ -15,23 +15,23 @@
 #include "ui/keyboard/keyboard_controller_observer.h"
 #include "ui/keyboard/keyboard_ui.h"
 
-namespace {
-class WindowBoundsChangeObserver;
-}
 namespace aura {
 class Window;
 }
+
 namespace gfx {
 class Rect;
 }
+
 namespace content {
 class BrowserContext;
 class WebContents;
 }  // namespace content
+
 namespace ui {
 class InputMethod;
 class Shadow;
-}
+}  // namespace ui
 
 // Subclass of KeyboardUI. It is used by KeyboardController to get
 // access to the virtual keyboard window and setup Chrome extension functions.
@@ -57,11 +57,12 @@ class ChromeKeyboardUI : public keyboard::KeyboardUI,
   // Overridden from KeyboardUI:
   aura::Window* GetKeyboardWindow() override;
   bool HasKeyboardWindow() const override;
+  ui::InputMethod* GetInputMethod() override;
+  void SetController(keyboard::KeyboardController* controller) override;
   void ReloadKeyboardIfNeeded() override;
   void InitInsets(const gfx::Rect& new_bounds) override;
   void ResetInsets() override;
 
- protected:
   // aura::WindowObserver overrides:
   void OnWindowBoundsChanged(aura::Window* window,
                              const gfx::Rect& old_bounds,
@@ -71,11 +72,9 @@ class ChromeKeyboardUI : public keyboard::KeyboardUI,
   void OnWindowParentChanged(aura::Window* window,
                              aura::Window* parent) override;
 
-  content::BrowserContext* browser_context() { return browser_context_; }
-
-  const aura::Window* GetKeyboardRootWindow() const;
-
  private:
+  class WindowBoundsChangeObserver;
+
   std::unique_ptr<content::WebContents> CreateWebContents();
 
   // Loads the web contents for the given |url|.
@@ -91,7 +90,7 @@ class ChromeKeyboardUI : public keyboard::KeyboardUI,
   // Whether this window should do an overscroll to avoid occlusion by the
   // virtual keyboard. IME windows and virtual keyboard windows should always
   // avoid overscroll.
-  bool ShouldWindowOverscroll(aura::Window* window) const;
+  bool ShouldWindowOverscroll(aura::Window* window);
 
   // Adds an observer for tracking changes to a window size or
   // position while the keyboard is displayed. Any window repositioning
@@ -108,10 +107,6 @@ class ChromeKeyboardUI : public keyboard::KeyboardUI,
   // loading the keyboard page.
   void SetupWebContents(content::WebContents* contents);
 
-  // Overridden from KeyboardUI:
-  ui::InputMethod* GetInputMethod() override;
-  void SetController(keyboard::KeyboardController* controller) override;
-
   // content::WebContentsObserver overrides
   void RenderViewCreated(content::RenderViewHost* render_view_host) override;
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,