Always Close the Autofill UI through the same path

chrome/browser/ui/autofill/autofill_popup_controller.h

Index: chrome/browser/ui/autofill/autofill_popup_controller.h
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller.h b/chrome/browser/ui/autofill/autofill_popup_controller.h
index d680082a258d599b6b305b607edbc7826a622baf..4c621ae5669fedaf273fc7e7a5e0405065f8751f 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller.h
+++ b/chrome/browser/ui/autofill/autofill_popup_controller.h
@@ -21,6 +21,9 @@ class RectF;
 // This interface provides data to an AutofillPopupView.
 class AutofillPopupController {
  public:
+  // Called when the popup should get hidden.
+  virtual void Hide() = 0;
+
   // Called when the view is going down.
   virtual void ViewDestroyed() = 0;

[Ilya Sherman, 2013/02/22 00:29:04]

Is this method still necessary?  If so, I don't really understand what this CL
is aiming at.  If not, could you explain how memory management occurs when the
user e.g. closes the tab?

[csharp, 2013/02/22 15:41:31]

Removed, all hiding/deleting is now done through the controller hide function.

[Ilya Sherman, 2013/02/23 00:02:36]

So, the view is owned by its parent view.  What happens when the parent view
decides to delete the view?  How does the controller hear back about it?  It's
not at all clear to me that the view code currently manages to communicate this,
esp. on Mac.

I think likely you want to actually call controller_->Hide() from the view's
destructor, but make sure that any calls the controller might try to make back
into the view are then safe, perhaps using a variable like is_hiding_ similar to
what the controller does.

[csharp, 2013/02/25 20:29:29]

The views aren't really owned by the parent, they usually own themselves (the
gtk version deletes itself when hidden, the views one either deletes itself or
has it parent widget delete it). Everything operates through the hide
functionality, the controller just tells the popup to hide itself, but it
doesn't worry about deleting it.

I don't think we need to call controller_->hide from the destructor. If the
popups are only destroyed when they are hidden (which should be true), then that
means that controller_->hide had already been called (maybe by the popup or
maybe by someone else).

[Ilya Sherman, 2013/02/26 01:02:32]

So, let's zoom in on the second Views case: Suppose the parent widget deletes
this window.  Is that guaranteed to go through the controller?  If so, why?

Note: I tried closing the containing tab or window on Mac, and it looks like we
do go through the controller in that case, because the WebContents destructor
destroys the AutofillExternalDelegate, which calls Hide() on the controller. 
Maybe this is true in all of the edge cases on all of the platforms, but I'm
still a little concerned that it might not be...
Yes, my concern is that the popup might be destroyed by some path other than the
Hide() method being called.

[csharp, 2013/02/26 18:33:31]

That second view case doesn't actually happen anymore (when fixing that aura
placement bug the view popup was modified to not have a parent, so it always
needs to delete itself), so for views we are guaranteed to have the correctly
flow.

I could modify the hide functions to set a variable, and then DCHECK that the
variable was set in the deconstructor, so we'd catch any cases where the popup's
where destroyed without hide. That doesn't seem to be the cleanest thing to do
though (I don't have any better ideas on catching those potential edge cases)

[Ilya Sherman, 2013/02/26 23:50:34]

Ok.
Yeah, let's do this, with a CHECK() rather than a DCHECK().  If there's no crash
reports after a Dev channel release, we can feel pretty confident that we're in
a good state, and drop the CHECK().

[csharp, 2013/02/27 18:18:13]

Done.