Eliminate AutofillExternalDelegate being a WebContentsUserData

components/autofill/content/browser/autofill_driver_impl.h

 // Copyright 2013 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 COMPONENTS_AUTOFILL_CONTENT_BROWSER_AUTOFILL_DRIVER_IMPL_H_
 #define COMPONENTS_AUTOFILL_CONTENT_BROWSER_AUTOFILL_DRIVER_IMPL_H_
 
 #include <string>
 
+#include "base/memory/scoped_ptr.h"
 #include "base/supports_user_data.h"
 #include "components/autofill/browser/autofill_driver.h"
+#include "components/autofill/browser/autofill_external_delegate.h"
 #include "components/autofill/browser/autofill_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 
 namespace content {
 class WebContents;
 }
 
 namespace IPC {
 class Message;
 }
 
 namespace autofill {
 
 class AutofillContext;
-class AutofillExternalDelegate;
 class AutofillManagerDelegate;
 
 // Class that drives autofill flow in the browser process based on
 // communication from the renderer and from the external world. There is one
 // instance per WebContents.
 class AutofillDriverImpl : public AutofillDriver,
                            public content::WebContentsObserver,
                            public base::SupportsUserData::Data {
  public:
   static void CreateForWebContentsAndDelegate(
       content::WebContents* contents,
       autofill::AutofillManagerDelegate* delegate,
       const std::string& app_locale,
       AutofillManager::AutofillDownloadManagerState enable_download_manager,
       bool enable_native_ui);

[blundell, 2013/06/17 15:14:37]

I looked at internalizing this parameter, but AndroidWebview always sets it to
true, so I think it doesn't make sense to internalize.

   static AutofillDriverImpl* FromWebContents(content::WebContents* contents);
 
   // AutofillDriver:
   virtual content::WebContents* GetWebContents() OVERRIDE;
 
+  AutofillExternalDelegate* autofill_external_delegate() {
+      return autofill_external_delegate_.get();

[Ilya Sherman, 2013/06/17 21:25:12]

nit: Reduce indentation by two spaces.

[blundell, 2013/06/18 06:20:47]

Done.

+  }
+
+  // Sets the external delegate to |delegate| both within this class and in the
+  // shared Autofill code. Takes ownership of |delegate|.
+  void SetAutofillExternalDelegate(
+      scoped_ptr<AutofillExternalDelegate> delegate);

[Ilya Sherman, 2013/06/17 21:25:12]

Why is this method, as well as the one above, public?  AFAICT they're only used
by tests.

[blundell, 2013/06/17 21:31:48]

I had thought about this. The tests can't operate on a subclass of this class
with expanded visibility since they get the instance from the WebContents. It
seems weird to friend tests in chrome/ from the autofill component. Do you see a
possibility that I'm missing?

On 2013/06/17 21:25:12, Ilya Sherman wrote:

[Ilya Sherman, 2013/06/17 21:45:27]

Seems like tests should be able to set a different instance of this class on the
WebContents, rather than operating on the one that's already present.  At least,
I'd hope that WCUD classes are test-friendly in that way.

On 2013/06/17 21:31:48, blundell wrote:

[blundell, 2013/06/18 06:20:47]

There's nothing like that for WCUD/SupportsUserData as far as I can tell...I'll
push this as-is and follow up with Avi to see if there's something I can do.

On 2013/06/17 21:45:27, Ilya Sherman wrote:

+
   AutofillManager* autofill_manager() { return &autofill_manager_; }
 
  private:
   AutofillDriverImpl(
       content::WebContents* web_contents,
       autofill::AutofillManagerDelegate* delegate,
       const std::string& app_locale,
       AutofillManager::AutofillDownloadManagerState enable_download_manager,
       bool enable_native_ui);
   virtual ~AutofillDriverImpl();
 
   // content::WebContentsObserver:
   virtual void DidNavigateMainFrame(
       const content::LoadCommittedDetails& details,
       const content::FrameNavigateParams& params) OVERRIDE;
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
+  // AutofillExternalDelegate instance that this object instantiates in the
+  // case where the autofill native UI is enabled.
+  scoped_ptr<AutofillExternalDelegate> autofill_external_delegate_;
+
   // AutofillManager instance via which this object drives the shared Autofill
   // code.
   AutofillManager autofill_manager_;
 };
 
 }  // namespace autofill
 
 #endif  // COMPONENTS_AUTOFILL_CONTENT_BROWSER_AUTOFILL_DRIVER_IMPL_H_