Always Close the Autofill UI through the same path

chrome/browser/ui/autofill/autofill_popup_controller.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_CONTROLLER_H_
 #define CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_CONTROLLER_H_
 
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/string16.h"
 #include "ui/gfx/native_widget_types.h"
 
 namespace gfx {
 class Font;
 class Point;
 class Rect;
 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.

 
   // Recalculates the height and width of the popup and triggers a redraw.
   virtual void UpdateBoundsAndRedrawPopup() = 0;
 
   // The user has moved the mouse within the popup.
   virtual void MouseHovered(int x, int y) = 0;
 
   // The user clicked the mouse within the popup.
   virtual void MouseClicked(int x, int y) = 0;
 
@@ skipping 53 matching lines @@
   virtual int selected_line() const = 0;
 
   // Returns true if the delete icon of the selected line is currently hovered.
   virtual bool delete_icon_hovered() const = 0;
 
  protected:
   virtual ~AutofillPopupController() {}
 };
 
 #endif  // CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_CONTROLLER_H_