Properly Remove Autofill Keyboard Listener.

chrome/browser/autofill/autofill_external_delegate.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_AUTOFILL_AUTOFILL_EXTERNAL_DELEGATE_H_
 #define CHROME_BROWSER_AUTOFILL_AUTOFILL_EXTERNAL_DELEGATE_H_
 
 #include <vector>
 
 #include "base/compiler_specific.h"
@@ skipping 168 matching lines @@
   FormData autofill_query_form_;
   FormFieldData autofill_query_field_;
 
   // Should we display a warning if Autofill is disabled?
   bool display_warning_if_disabled_;
 
   // Have we already shown Autofill suggestions for the field the user is
   // currently editing?  Used to keep track of state for metrics logging.
   bool has_shown_autofill_popup_for_current_edit_;
 
+  // The RenderViewHost that this object has been registered with as a
+  // keyboard listener.
+  content::RenderViewHost* registered_keyboard_listener_with_;

[Ilya Sherman, 2013/02/13 21:58:12]

If this is only needed for tests, then you should make the change in test code,
not in the production code.

[csharp, 2013/02/13 22:16:04]

I'm not if this is only needed for tests. It was one of the browser tests that
triggered the problem (the RenderViewHost at construction was different from the
one when the popup was hidden), so I think that it could occur in the wild
(although I haven't repo it.

So I think this should stay, since it might occur in production.

[Ilya Sherman, 2013/02/14 06:19:18]

As far as I understand, it really shouldn't be possible for the RenderViewHost
to change between construction and when the popup is hidden.  The RenderViewHost
should not change when there is no page navigation, and the popup should be
dismissed prior to page navigation.  Let's do some more investigation to
understand why the test is failing.

[Ilya Sherman, 2013/02/15 05:57:55]

After digging through the WebContents code a bunch more, I'm now convinced that
this or something like it is indeed necessary, as there is no guarantee that
we'll discover that a navigation has happened until after the RenderViewHost
might already have been swapped out.  If you keep the other navigation in the
browser test, the DCHECK will still be hit without this code.  It looks like
it's also possible for web_contents_->GetRenderViewHost() to return NULL during
shutdown.

Now, it looks like rather than tracking this state ourselves, we can potentially
listen for the notification NOTIFICATION_RENDER_VIEW_HOST_CHANGED.  If we get
that notification, and the new RenderViewHost is the current one for our
WebContents, then we can unregister from the old host and set a boolean not to
unregister from the new one.  This seems somewhat circuitous and potentially
fragile, but maybe it's the right way to go.

Alternately, we can do what you were doing in this patch set.  The thing that
makes me unhappy about this is that it seems like every keyboard listener will
have to implement this sort of logic, which kind of sucks.  For now though, this
is the only class that's a KeyboardListener, so I guess it's ok to implement the
solution just within this class.

[csharp, 2013/02/15 14:52:27]

Ok, I revert to this handling and file a bug to better handle removing the
KeyboardListener.

+
   // The current data list values.
   std::vector<string16> data_list_values_;
   std::vector<string16> data_list_labels_;
   std::vector<string16> data_list_icons_;
   std::vector<int> data_list_unique_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(AutofillExternalDelegate);
 };
 
 #endif  // CHROME_BROWSER_AUTOFILL_AUTOFILL_EXTERNAL_DELEGATE_H_