Use WeakPtr to simplify AutofillPopupControllerImpl memory management

Use WeakPtr to simplify AutofillPopupControllerImpl memory management

Also change AutofillPopupDelegate to move keyboard listener code to a same place

Note. Still working on unittests changes.

BUG=140037
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181225

[Ilya Sherman, 2013-02-05 23:41:04]

LGTM, thanks.  Please also wait for Evan's review, as he is the person most
familiar with the UI code.

https://codereview.chromium.org/12217024/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://codereview.chromium.org/12217024/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_external_delegate.cc:207: void
AutofillExternalDelegate::OnPopupShown(content::KeyboardListener* popup) {
nit: Pass by const-reference?

https://codereview.chromium.org/12217024/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_external_delegate.cc:211: void
AutofillExternalDelegate::OnPopupHidden(content::KeyboardListener* popup) {
nit: Ditto

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right):

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:33: public
base::SupportsWeakPtr<AutofillPopupControllerImpl> {
nit: Can you use a WeakPtrFactory member variable rather than inheriting from
SupportsWeakPtr?  In general, composition is preferable to inheritance when
possible.  For this case specifically, only the class itself should be able to
generate WeakPtrs to itself; other classes that use it should not be able to do
so.

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_popup_delegate.h (right):

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_delegate.h:18: // Called when autofill
popup is shown. The KeyboardListener should be
nit: "autofill" -> "the Autofill"

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_delegate.h:19: // registered if the
popup needs to be controlled by keyboard.
The second sentence here is somewhat misleading.  The popup is always
controllable by keyboard.  However, the AutofillDialogController forwards the
keypresses to the popup directly, whereas the AutofillExternalDelegate registers
the class as a keyboard listener on its WebContents.

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_delegate.h:22: // Called when autofill
popup is hidden. The KeyboardListener must be
nit: "autofill" -> "the Autofill"

[kaiwang, 2013-02-06 00:28:17]

https://codereview.chromium.org/12217024/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://codereview.chromium.org/12217024/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_external_delegate.cc:207: void
AutofillExternalDelegate::OnPopupShown(content::KeyboardListener* popup) {
RenderViewHost::AddKeayboardListener() requires a KeyboardListener*. With const
ref, I can only get a const KeyboardListener*

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right):

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:33: public
base::SupportsWeakPtr<AutofillPopupControllerImpl> {
On 2013/02/05 23:41:04, Ilya Sherman wrote:
> nit: Can you use a WeakPtrFactory member variable rather than inheriting from
> SupportsWeakPtr?  In general, composition is preferable to inheritance when
> possible.  For this case specifically, only the class itself should be able to
> generate WeakPtrs to itself; other classes that use it should not be able to
do
> so.

Done.

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_popup_delegate.h (right):

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_delegate.h:18: // Called when autofill
popup is shown. The KeyboardListener should be
On 2013/02/05 23:41:04, Ilya Sherman wrote:
> nit: "autofill" -> "the Autofill"

Done.

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_delegate.h:19: // registered if the
popup needs to be controlled by keyboard.
Done, followed estade's suggestion

https://codereview.chromium.org/12217024/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_delegate.h:22: // Called when autofill
popup is hidden. The KeyboardListener must be
On 2013/02/05 23:41:04, Ilya Sherman wrote:
> nit: "autofill" -> "the Autofill"

Done.

[Evan Stade, 2013-02-06 00:30:24]

(I commented on the other CL)

[Ilya Sherman, 2013-02-06 00:53:09]

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:255:
WeakPtr<AutofillPopupControllerImpl> controller;
nit: Why not include the assignment as part of the same statement?

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:265:
controller, &delegate, NULL, gfx::Rect());
You should preserve the test semantics, i.e. you should pass in a
default-constructed WeakPtr<AutofillPopupControllerImpl>.

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:278:
test_controller->GetWeakPtr();
nit: Doesn't seem like you really need to cache this in a variable.

[Evan Stade, 2013-02-06 03:40:04]

I prefer using SupportsWeakPtr over the factory, but lgtm either way

[kaiwang, 2013-02-06 04:15:29]

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:255:
WeakPtr<AutofillPopupControllerImpl> controller;
because controller is used as a parameter. AFAIK the parameter might not be
initialized when used.

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:265:
controller, &delegate, NULL, gfx::Rect());
On 2013/02/06 00:53:09, Ilya Sherman wrote:
> You should preserve the test semantics, i.e. you should pass in a
> default-constructed WeakPtr<AutofillPopupControllerImpl>.

Done.

https://codereview.chromium.org/12217024/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:278:
test_controller->GetWeakPtr();
On 2013/02/06 00:53:09, Ilya Sherman wrote:
> nit: Doesn't seem like you really need to cache this in a variable.

Done.

[commit-bot: I haz the power, 2013-02-06 04:16:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12217024/5003

[commit-bot: I haz the power, 2013-02-06 04:42:26]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Ilya Sherman, 2013-02-06 05:12:52]

https://chromiumcodereview.appspot.com/12217024/diff/5003/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/12217024/diff/5003/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:253:
controller, &delegate, NULL, gfx::Rect());
Rather than passing the controller as an argument here, pass
WeakPtr<AutofillPopupControllerImpl>(), since that's really what you mean.

[kaiwang, 2013-02-06 05:56:21]

https://codereview.chromium.org/12217024/diff/5003/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/12217024/diff/5003/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:253:
controller, &delegate, NULL, gfx::Rect());
On 2013/02/06 05:12:52, Ilya Sherman wrote:
> Rather than passing the controller as an argument here, pass
> WeakPtr<AutofillPopupControllerImpl>(), since that's really what you mean.

Done.

[commit-bot: I haz the power, 2013-02-07 00:56:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12217024/11006

[commit-bot: I haz the power, 2013-02-07 00:56:18]

Failed to apply patch for chrome/browser/ui/autofill/autofill_popup_delegate.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_popup_delegate.h
  Hunk #1 FAILED at 7.
  Hunk #2 succeeded at 23 (offset -2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_popup_delegate.h.rej

Patch:       chrome/browser/ui/autofill/autofill_popup_delegate.h
Index: chrome/browser/ui/autofill/autofill_popup_delegate.h
diff --git a/chrome/browser/ui/autofill/autofill_popup_delegate.h
b/chrome/browser/ui/autofill/autofill_popup_delegate.h
index
a8d6cc16552b79c9ba06d4bcfdab99e99511dec3..febc876ee1a0f39034416eb8c892727d3f72e10f
100644
--- a/chrome/browser/ui/autofill/autofill_popup_delegate.h
+++ b/chrome/browser/ui/autofill/autofill_popup_delegate.h
@@ -7,11 +7,21 @@
 
 #include "base/string16.h"
 
+namespace content {
+class KeyboardListener;
+}
+
 // An interface for interaction with AutofillPopupController. Will be notified
 // of events by the controller.
 class AutofillPopupDelegate {
  public:
-  virtual ~AutofillPopupDelegate() { }
+  // Called when the Autofill popup is shown. |listener| may be used to pass
+  // keyboard events to the popup.
+  virtual void OnPopupShown(content::KeyboardListener* listener) = 0;
+
+  // Called when the Autofill popup is hidden. |listener| must be unregistered
+  // if it was registered in OnPopupShown.
+  virtual void OnPopupHidden(content::KeyboardListener* listener) = 0;
 
   // Called when the autofill suggestion indicated by |identifier| has been
   // temporarily selected (e.g., hovered).
@@ -25,9 +35,6 @@ class AutofillPopupDelegate {
 
   // Informs the delegate that the Autofill previewed form should be cleared.
   virtual void ClearPreviewedForm() = 0;
-
-  // Called to inform the delegate the controller is experiencing destruction.
-  virtual void ControllerDestroyed() = 0;
 };
 
 #endif  // CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_DELEGATE_H_

[commit-bot: I haz the power, 2013-02-07 01:12:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12217024/8005

[commit-bot: I haz the power, 2013-02-07 02:36:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-07 03:09:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12217024/1013

[commit-bot: I haz the power, 2013-02-07 06:06:11]

Change committed as 181225