[android] Autofill popup behavior fixes.

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

Index: chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
diff --git a/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc b/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
index 0630b482ae47ce709a9e6538511f6915337cd2c7..2c73a69060a54a7c8fd0566853b7be93189b3b70 100644
--- a/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
+++ b/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
@@ -9,6 +9,7 @@
 #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 {
@@ -28,9 +29,9 @@ const SkColor AutofillPopupBaseView::kWarningTextColor =
 
 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() {
@@ -44,14 +45,11 @@ AutofillPopupBaseView::~AutofillPopupBaseView() {
 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);
@@ -100,8 +98,7 @@ void AutofillPopupBaseView::DoHide() {
 }
 
 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);
 }
 
@@ -117,12 +114,6 @@ void AutofillPopupBaseView::OnNativeFocusChange(
     HideController();
 }
 
-void AutofillPopupBaseView::OnWidgetBoundsChanged(views::Widget* widget,
-                                                  const gfx::Rect& new_bounds) {
-  DCHECK_EQ(widget, observing_widget_);
-  HideController();
-}
-
 void AutofillPopupBaseView::OnMouseCaptureLost() {
   ClearSelection();
 }