[android] Autofill popup behavior fixes.

chrome/browser/ui/views/autofill/autofill_popup_base_view.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/autofill/autofill_popup_base_view.h"
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop.h"
 #include "chrome/browser/ui/autofill/popup_constants.h"
 #include "ui/views/border.h"
+#include "ui/views/focus/focus_manager.h"
 #include "ui/views/widget/widget.h"
 
 namespace autofill {
 
 const SkColor AutofillPopupBaseView::kBorderColor =
     SkColorSetARGB(0xFF, 0xC7, 0xCA, 0xCE);
 const SkColor AutofillPopupBaseView::kHoveredBackgroundColor =
     SkColorSetARGB(0xFF, 0xCD, 0xCD, 0xCD);
 const SkColor AutofillPopupBaseView::kItemTextColor =
     SkColorSetARGB(0xFF, 0x7F, 0x7F, 0x7F);
 const SkColor AutofillPopupBaseView::kPopupBackground =
     SkColorSetARGB(0xFF, 0xFF, 0xFF, 0xFF);
 const SkColor AutofillPopupBaseView::kValueTextColor =
     SkColorSetARGB(0xFF, 0x00, 0x00, 0x00);
 const SkColor AutofillPopupBaseView::kWarningTextColor =
     SkColorSetARGB(0xFF, 0x7F, 0x7F, 0x7F);
 
 AutofillPopupBaseView::AutofillPopupBaseView(
     AutofillPopupViewDelegate* delegate,
-    views::Widget* observing_widget)
+    views::FocusManager* focus_manager)
     : delegate_(delegate),
-      observing_widget_(observing_widget),
+      focus_manager_(focus_manager),
       weak_ptr_factory_(this) {}
 
 AutofillPopupBaseView::~AutofillPopupBaseView() {
   if (delegate_) {
     delegate_->ViewDestroyed();
 
     RemoveObserver();
   }
 }
 
 void AutofillPopupBaseView::DoShow() {
   const bool initialize_widget = !GetWidget();
   if (initialize_widget) {
-    observing_widget_->AddObserver(this);
-
-    views::FocusManager* focus_manager = observing_widget_->GetFocusManager();
-    focus_manager->RegisterAccelerator(
+    focus_manager_->RegisterAccelerator(
         ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE),
         ui::AcceleratorManager::kNormalPriority,
         this);
-    focus_manager->RegisterAccelerator(
+    focus_manager_->RegisterAccelerator(
         ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE),
         ui::AcceleratorManager::kNormalPriority,
         this);
 
     // The widget is destroyed by the corresponding NativeWidget, so we use
     // a weak pointer to hold the reference and don't have to worry about
     // deletion.
     views::Widget* widget = new views::Widget;
     views::Widget::InitParams params(views::Widget::InitParams::TYPE_POPUP);
     params.delegate = this;
@@ skipping 28 matching lines @@
     // assume the the widget is still valid after this point.
     // http://crbug.com/229224
     // NOTE: This deletes |this|.
     GetWidget()->Close();
   } else {
     delete this;
   }
 }
 
 void AutofillPopupBaseView::RemoveObserver() {
-  observing_widget_->GetFocusManager()->UnregisterAccelerators(this);
-  observing_widget_->RemoveObserver(this);
+  focus_manager_->UnregisterAccelerators(this);

[Evan Stade, 2015/01/26 20:21:54]

Seems like this could cause problems during shutdown in particular. Maybe sky@
knows if this is safe.

[sky, 2015/01/26 22:26:53]

Is focus_manager_ from another widget? If so, I agree, there may be timing
issues.

[please use gerrit instead, 2015/01/26 23:49:03]

An instance of FocusManager is created when initializing a Widget and deleted in
the destructor of the Widget.

1) You cannot show an autofill popup for an unitialized Widget.

2) AutofillPopupBaseView was using a Widget (observing_widget_) to get the
FocusManager before this patch, so both observing_widget_ and its FocusManager
survive past the lifetime of AutofillPopupBaseView.

(1) and (2) together show that AutofillPopupBaseView is created after
FocusManager creation and deleted before FocusManager deletion. Therefore, this
change should be safe.

[Evan Stade, 2015/01/27 00:01:43]

do you even need to pass the focus manager? You can't do something like
GetWidget()->ParentWidget()->GetFocusManager()?

[please use gerrit instead, 2015/01/27 20:02:41]

I've tried using the focus manager of the widget created in DoShow(), but
requestAutocomplete dialog behavior broke: pressing Enter to select a profile
will attempt to submit the dialog instead; pressing Esc to dismiss a profile
suggestion will dismiss the dialog instead. Thus the focus manager needs to come
from a top-level widget.

   views::WidgetFocusManager::GetInstance()->RemoveFocusChangeListener(this);
 }
 
 void AutofillPopupBaseView::DoUpdateBoundsAndRedrawPopup() {
   GetWidget()->SetBounds(delegate_->popup_bounds());
   SchedulePaint();
 }
 
 void AutofillPopupBaseView::OnNativeFocusChange(
     gfx::NativeView focused_before,
     gfx::NativeView focused_now) {
   if (GetWidget() && GetWidget()->GetNativeView() != focused_now)
     HideController();
 }
 
-void AutofillPopupBaseView::OnWidgetBoundsChanged(views::Widget* widget,
-                                                  const gfx::Rect& new_bounds) {
-  DCHECK_EQ(widget, observing_widget_);
-  HideController();
-}
-
 void AutofillPopupBaseView::OnMouseCaptureLost() {
   ClearSelection();
 }
 
 bool AutofillPopupBaseView::OnMouseDragged(const ui::MouseEvent& event) {
   if (HitTestPoint(event.location())) {
     SetSelection(event.location());
 
     // We must return true in order to get future OnMouseDragged and
     // OnMouseReleased events.
@@ skipping 96 matching lines @@
 void AutofillPopupBaseView::HideController() {
   if (delegate_)
     delegate_->Hide();
 }
 
 gfx::NativeView AutofillPopupBaseView::container_view() {
   return delegate_->container_view();
 }
 
 }  // namespace autofill