In components/autofill, move notification handling into content driver.

chrome/browser/autofill/autofill_driver_impl_browsertest.cc

 // 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.
 
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_pref_service_syncable.h"
-#include "components/autofill/core/browser/autofill_manager.h"
-#include "components/autofill/core/browser/test_autofill_driver.h"
-#include "components/autofill/core/browser/test_autofill_external_delegate.h"
+#include "components/autofill/content/browser/autofill_driver_impl.h"
 #include "components/autofill/core/browser/test_autofill_manager_delegate.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ skipping 25 matching lines @@
                     base::WeakPtr<AutofillPopupDelegate> delegate));
 
   MOCK_METHOD0(HideAutofillPopup, void());
 
  private:
   TestingPrefServiceSyncable prefs_;
 
   DISALLOW_COPY_AND_ASSIGN(MockAutofillManagerDelegate);
 };
 
-// Subclass AutofillManager so we can create AutofillManager instance.
-class TestAutofillManager : public AutofillManager {
+// Subclass AutofillDriverImpl so we can create AutofillDriverImpl instance.

[Ilya Sherman, 2013/06/26 22:36:18]

nit: "AutofillDriverImpl instance" -> "an AutofillDriverImpl instance"

[blundell, 2013/06/27 14:50:35]

Done.

+class TestAutofillDriverImpl : public AutofillDriverImpl {
  public:
-  TestAutofillManager(AutofillDriver* driver,
-                      autofill::AutofillManagerDelegate* delegate)
-      : AutofillManager(driver,
-                        delegate,
-                        g_browser_process->GetApplicationLocale(),
-                        AutofillManager::ENABLE_AUTOFILL_DOWNLOAD_MANAGER) {}
-  virtual ~TestAutofillManager() {}
+  TestAutofillDriverImpl(content::WebContents* web_contents,
+                         AutofillManagerDelegate* delegate)
+      : AutofillDriverImpl(web_contents,
+                           delegate,
+                           g_browser_process->GetApplicationLocale(),
+          AutofillManager::ENABLE_AUTOFILL_DOWNLOAD_MANAGER) {}

[Ilya Sherman, 2013/06/26 22:36:18]

nit: Reduce the indentation of this line by two spaces.

[blundell, 2013/06/27 14:50:35]

4-space indenting is correct for a wrapped argument
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...).
I moved all the other arguments to that same indentation per what the style
guide seems to indicate for this case.

On 2013/06/26 22:36:18, Ilya Sherman wrote:

[Ilya Sherman, 2013/06/27 22:16:10]

Ah, I'd missed that this was also an argument to the AutofillDriverImpl
constructor.  In that case, I think you actually want to *increase* the
indentation by two spaces, as it should be four spaces from the 'A' in
'AutofillDriverImpl'.  Since this change has already landed, though, I wouldn't
worry about going back to fix this -- just something to keep in mind for the
future :)

(FWIW, I think most editors can be configured to apply these indentation rules
more or less automatically, which removes a lot of the tedium and guesswork in
making sure that things are aligned correctly.)

On 2013/06/27 14:50:35, blundell wrote:

+  virtual ~TestAutofillDriverImpl() {}
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(TestAutofillManager);
-};
-
-// Subclass AutofillExternalDelegate so we can create an
-// AutofillExternalDelegate instance.
-class TestAutofillExternalDelegate : public AutofillExternalDelegate {
- public:
-  TestAutofillExternalDelegate(content::WebContents* web_contents,
-                               AutofillManager* autofill_manager)
-      : AutofillExternalDelegate(web_contents, autofill_manager) {}
-  virtual ~TestAutofillExternalDelegate() {}
+  DISALLOW_COPY_AND_ASSIGN(TestAutofillDriverImpl);
 };
 
 }  // namespace
 
-class AutofillExternalDelegateBrowserTest
+class AutofillDriverImplBrowserTest
     : public InProcessBrowserTest,
       public content::WebContentsObserver {
  public:
-  AutofillExternalDelegateBrowserTest() {}
-  virtual ~AutofillExternalDelegateBrowserTest() {}
+  AutofillDriverImplBrowserTest() {}
+  virtual ~AutofillDriverImplBrowserTest() {}
 
   virtual void SetUpOnMainThread() OVERRIDE {
     web_contents_ = browser()->tab_strip_model()->GetActiveWebContents();
     ASSERT_TRUE(web_contents_ != NULL);
     Observe(web_contents_);
-
-    AutofillManager::RegisterUserPrefs(manager_delegate_.GetPrefRegistry());

[blundell, 2013/06/26 20:42:31]

Deleting this call doesn't seem to have any effect. Is there anything I'm
mising?

[Ilya Sherman, 2013/06/26 22:36:18]

Not sure.  This might be meant to suppress some DCHECK or debug logging... but I
don't remember at this point.

[blundell, 2013/06/27 14:50:35]

Restored the line, as neither of us has a definitive argument that it's not
needed for some subtle reason.

-
-    autofill_driver_.reset(new TestAutofillDriver(web_contents_));
-    autofill_manager_.reset(
-        new TestAutofillManager(autofill_driver_.get(), &manager_delegate_));
-    autofill_external_delegate_.reset(
-        new TestAutofillExternalDelegate(web_contents_,
-                                         autofill_manager_.get()));
+    autofill_driver_.reset(new TestAutofillDriverImpl(web_contents_,
+                                                      &manager_delegate_));
   }
 
-  // Normally the WebContents will automatically delete the delegate, but here
-  // the delegate is owned by this test, so we have to manually destroy.
+  // Normally the WebContents will automatically delete the driver, but here
+  // the driver is owned by this test, so we have to manually destroy.
   virtual void WebContentsDestroyed(content::WebContents* web_contents)
       OVERRIDE {
     DCHECK_EQ(web_contents_, web_contents);
-    autofill_external_delegate_.reset();
-    autofill_manager_.reset();
     autofill_driver_.reset();
   }
 
  protected:
   content::WebContents* web_contents_;
 
   testing::NiceMock<MockAutofillManagerDelegate> manager_delegate_;
-  scoped_ptr<TestAutofillDriver> autofill_driver_;
-  scoped_ptr<TestAutofillManager> autofill_manager_;
-  scoped_ptr<TestAutofillExternalDelegate> autofill_external_delegate_;
+  scoped_ptr<TestAutofillDriverImpl> autofill_driver_;
 };
 
-IN_PROC_BROWSER_TEST_F(AutofillExternalDelegateBrowserTest,
+IN_PROC_BROWSER_TEST_F(AutofillDriverImplBrowserTest,
                        SwitchTabAndHideAutofillPopup) {
-  autofill::GenerateTestAutofillPopup(autofill_external_delegate_.get());

[blundell, 2013/06/26 20:42:31]

Code inspection made me think that these calls were unnecessary here, and
indeed, removing them doesn't affect the tests. Is there anything I'm missing?

[Ilya Sherman, 2013/06/26 22:36:18]

These were probably needed when the test relied less on mocks, or the tested
code had a different memory model, or something like that.  I agree that it
looks like they are no longer needed.

-
   // Notification is different on platforms. On linux this will be called twice,
   // while on windows only once.
   EXPECT_CALL(manager_delegate_, HideAutofillPopup())
       .Times(testing::AtLeast(1));
 
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED,
       content::Source<content::WebContents>(web_contents_));
   chrome::AddSelectedTabWithURL(browser(), GURL(content::kAboutBlankURL),
                                 content::PAGE_TRANSITION_AUTO_TOPLEVEL);
   observer.Wait();
 }
 
-IN_PROC_BROWSER_TEST_F(AutofillExternalDelegateBrowserTest,
+IN_PROC_BROWSER_TEST_F(AutofillDriverImplBrowserTest,
                        TestPageNavigationHidingAutofillPopup) {
-  autofill::GenerateTestAutofillPopup(autofill_external_delegate_.get());
-
   // Notification is different on platforms. On linux this will be called twice,
   // while on windows only once.
   EXPECT_CALL(manager_delegate_, HideAutofillPopup())
       .Times(testing::AtLeast(1));
 
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED,
       content::Source<content::NavigationController>(
           &(web_contents_->GetController())));
   browser()->OpenURL(content::OpenURLParams(
       GURL(chrome::kChromeUIBookmarksURL), content::Referrer(),
       CURRENT_TAB, content::PAGE_TRANSITION_TYPED, false));
   browser()->OpenURL(content::OpenURLParams(
       GURL(chrome::kChromeUIAboutURL), content::Referrer(),
       CURRENT_TAB, content::PAGE_TRANSITION_TYPED, false));
   observer.Wait();
 }
 
 }  // namespace autofill