Credential Manager: Refactor password_manager::CredentialManagerClient.

password_manager::CredentialManagerClient was a RenderProcessObserver,
and sent messages via RenderThread. This couldn't possibly have ever
worked; the messages it sent were simply dropped on the floor.

After this patch, it's a RenderViewObserver and its messages
successfully land in the ChromePasswordManagerClient, just as they
ought to.

BUG=400674

Review URL: https://codereview.chromium.org/533493004

Cr-Commit-Position: refs/heads/master@{#292932}

chrome/renderer/chrome_content_renderer_client.cc

@@ -319,9 +319,6 @@ void ChromeContentRendererClient::RenderThreadStarted() {
 #endif
   search_bouncer_.reset(new SearchBouncer());
 
-  credential_manager_client_.reset(
-      new password_manager::CredentialManagerClient());
-
   thread->AddObserver(chrome_observer_.get());
   thread->AddObserver(extension_dispatcher_.get());
 #if defined(FULL_SAFE_BROWSING)
@@ -330,7 +327,6 @@ void ChromeContentRendererClient::RenderThreadStarted() {
   thread->AddObserver(visited_link_slave_.get());
   thread->AddObserver(prerender_dispatcher_.get());
   thread->AddObserver(search_bouncer_.get());
-  thread->AddObserver(credential_manager_client_.get());
 
 #if defined(ENABLE_WEBRTC)
   thread->AddFilter(webrtc_logging_message_filter_.get());
@@ -507,8 +503,7 @@ void ChromeContentRendererClient::RenderViewCreated(
 
   new ChromeRenderViewObserver(render_view, chrome_observer_.get());
 
-  if (credential_manager_client_)
-    credential_manager_client_->OnRenderViewCreated(render_view);
+  new password_manager::CredentialManagerClient(render_view);
 }
 
 void ChromeContentRendererClient::SetNumberOfViews(int number_of_views) {

components/components_tests.gyp

@@ -432,7 +432,6 @@
               'sources': [
                 'autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc',
                 'dom_distiller/content/dom_distiller_viewer_source_unittest.cc',
-                'password_manager/content/renderer/credential_manager_client_unittest.cc',
                 'power/origin_power_map_unittest.cc',
                 'usb_service/usb_context_unittest.cc',
                 'usb_service/usb_device_filter_unittest.cc',
@@ -462,10 +461,6 @@
                 # Dependencies of precache/content
                 'components.gyp:precache_content',
 
-                # Dependencies of password_manager
-                'components.gyp:password_manager_content_renderer',
-                'components.gyp:password_manager_content_renderer_test_support',
-
                 # Dependencies of power
                 'components.gyp:power',
 
@@ -845,6 +840,7 @@
             'components.gyp:autofill_content_browser',
             'components.gyp:dom_distiller_content',
             'components.gyp:dom_distiller_core',
+            'components.gyp:password_manager_content_renderer',
             'components.gyp:pref_registry_test_support',
             'components_resources.gyp:components_resources',
             '../content/content.gyp:content_common',
@@ -868,6 +864,7 @@
           'sources': [
             'autofill/content/browser/risk/fingerprint_browsertest.cc',
             'dom_distiller/content/distiller_page_web_contents_browsertest.cc',
+            'password_manager/content/renderer/credential_manager_client_browsertest.cc',
           ],
           'actions': [
             {

components/password_manager.gypi

@@ -198,26 +198,6 @@
             'password_manager/content/renderer/credential_manager_client.h',
           ],
         },
-        {
-          # GN version: //components/password_manager/core/browser:test_support
-          'target_name': 'password_manager_content_renderer_test_support',
-          'type': 'static_library',
-          'dependencies': [
-            'password_manager_content_common',
-            '../base/base.gyp:base',
-            '../testing/gmock.gyp:gmock',
-            '../testing/gtest.gyp:gtest',
-            '../third_party/WebKit/public/blink.gyp:blink',
-          ],
-          'include_dirs': [
-            '..',
-          ],
-          'sources': [
-            # Note: sources list duplicated in GN build.
-            'password_manager/content/renderer/test_credential_manager_client.cc',
-            'password_manager/content/renderer/test_credential_manager_client.h',
-          ],
-        },
         {
           # GN version: //components/password_manager/content/browser
           'target_name': 'password_manager_content_browser',

components/password_manager/content/renderer/BUILD.gn

@@ -17,26 +17,13 @@ static_library("renderer") {
   ]
 }
 
-static_library("test_support") {
+source_set("browser_tests") {
   sources = [
-    "test_credential_manager_client.cc",
-    "test_credential_manager_client.h",
-  ]
-
-  deps = [
-    "//testing/gmock",
-    "//third_party/WebKit/public:blink",
-  ]
-}
-
-source_set("unit_tests") {
-  sources = [
-    "credential_manager_client_unittest.cc"
+    "credential_manager_client_browsertest.cc"
   ]
 
   deps = [
     ":renderer",
-    ":test_support",
     "//testing/gmock",
     "//testing/gtest",
     "//third_party/WebKit/public:blink",

components/password_manager/content/renderer/credential_manager_client.cc

@@ -6,7 +6,6 @@
 
 #include "components/password_manager/content/common/credential_manager_messages.h"
 #include "components/password_manager/content/common/credential_manager_types.h"
-#include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
 #include "third_party/WebKit/public/platform/WebCredential.h"
 #include "third_party/WebKit/public/platform/WebCredentialManagerError.h"
@@ -31,45 +30,19 @@ void ClearCallbacksMapWithErrors(T* callbacks_map) {
 
 }  // namespace
 
-CredentialManagerClient::CredentialManagerClient()
-    : routing_id_(MSG_ROUTING_NONE),
-      render_thread_(content::RenderThread::Get()) {
-  DCHECK(render_thread_);
+CredentialManagerClient::CredentialManagerClient(
+    content::RenderView* render_view)
+    : content::RenderViewObserver(render_view) {
+  render_view->GetWebView()->setCredentialManagerClient(this);
 }
 
 CredentialManagerClient::~CredentialManagerClient() {
-  DisconnectFromRenderThread();
-
   ClearCallbacksMapWithErrors(&failed_sign_in_callbacks_);
   ClearCallbacksMapWithErrors(&signed_in_callbacks_);
   ClearCallbacksMapWithErrors(&signed_out_callbacks_);
   ClearCallbacksMapWithErrors(&request_callbacks_);
 }
 
-void CredentialManagerClient::OnRenderViewCreated(
-    content::RenderView* render_view) {
-  render_view->GetWebView()->setCredentialManagerClient(this);
-}
-
-void CredentialManagerClient::OnRenderProcessShutdown() {
-  DisconnectFromRenderThread();
-}
-
-int CredentialManagerClient::GetRoutingID() {
-  if (render_thread_ && routing_id_ == MSG_ROUTING_NONE) {
-    routing_id_ = render_thread_->GenerateRoutingID();
-    render_thread_->AddRoute(routing_id_, this);
-  }
-  return routing_id_;
-}
-
-void CredentialManagerClient::DisconnectFromRenderThread() {
-  if (render_thread_ && routing_id_ != MSG_ROUTING_NONE)
-    render_thread_->RemoveRoute(routing_id_);
-  render_thread_ = NULL;
-  routing_id_ = MSG_ROUTING_NONE;
-}
-
 // -----------------------------------------------------------------------------
 // Handle messages from the browser.
 
@@ -119,10 +92,8 @@ void CredentialManagerClient::dispatchFailedSignIn(
   int request_id = failed_sign_in_callbacks_.Add(callbacks);
   CredentialInfo info(
       credential.id(), credential.name(), credential.avatarURL());
-  if (render_thread_) {
-    render_thread_->Send(new CredentialManagerHostMsg_NotifyFailedSignIn(
-        GetRoutingID(), request_id, info));
-  }
+  Send(new CredentialManagerHostMsg_NotifyFailedSignIn(
+      routing_id(), request_id, info));
 }
 
 void CredentialManagerClient::dispatchSignedIn(
@@ -131,19 +102,14 @@ void CredentialManagerClient::dispatchSignedIn(
   int request_id = signed_in_callbacks_.Add(callbacks);
   CredentialInfo info(
       credential.id(), credential.name(), credential.avatarURL());
-  if (render_thread_) {
-    render_thread_->Send(new CredentialManagerHostMsg_NotifySignedIn(
-        GetRoutingID(), request_id, info));
-  }
+  Send(new CredentialManagerHostMsg_NotifySignedIn(
+      routing_id(), request_id, info));
 }
 
 void CredentialManagerClient::dispatchSignedOut(
     NotificationCallbacks* callbacks) {
   int request_id = signed_out_callbacks_.Add(callbacks);
-  if (render_thread_) {
-    render_thread_->Send(new CredentialManagerHostMsg_NotifySignedOut(
-        GetRoutingID(), request_id));
-  }
+  Send(new CredentialManagerHostMsg_NotifySignedOut(routing_id(), request_id));
 }
 
 void CredentialManagerClient::dispatchRequest(
@@ -154,10 +120,8 @@ void CredentialManagerClient::dispatchRequest(
   std::vector<GURL> federation_vector;
   for (size_t i = 0; i < std::min(federations.size(), kMaxFederations); ++i)
     federation_vector.push_back(federations[i]);
-  if (render_thread_) {
-    render_thread_->Send(new CredentialManagerHostMsg_RequestCredential(
-        GetRoutingID(), request_id, zeroClickOnly, federation_vector));
-  }
+  Send(new CredentialManagerHostMsg_RequestCredential(
+      routing_id(), request_id, zeroClickOnly, federation_vector));
 }
 
 void CredentialManagerClient::RespondToNotificationCallback(

components/password_manager/content/renderer/credential_manager_client.h

@@ -8,7 +8,7 @@
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/id_map.h"
-#include "content/public/renderer/render_process_observer.h"
+#include "content/public/renderer/render_view_observer.h"
 #include "ipc/ipc_listener.h"
 #include "third_party/WebKit/public/platform/WebCredentialManagerClient.h"
 #include "third_party/WebKit/public/platform/WebVector.h"
@@ -45,19 +45,12 @@ struct CredentialInfo;
 // CredentialManagerClient (set in 'OnRenderViewCreated()'). The client is
 // guaranteed to outlive the views that point to it.
 class CredentialManagerClient : public blink::WebCredentialManagerClient,
-                                public content::RenderProcessObserver,
-                                public IPC::Listener {
+                                public content::RenderViewObserver {
  public:
-  CredentialManagerClient();
+  explicit CredentialManagerClient(content::RenderView* render_view);
   virtual ~CredentialManagerClient();
 
-  // When a RenderView is created, we need to set this object as its client.
-  void OnRenderViewCreated(content::RenderView*);
-
-  // content::RenderProcessObserver:
-  virtual void OnRenderProcessShutdown() OVERRIDE;
-
-  // IPC::Listener:
+  // RenderViewObserver:
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
   // Message handlers for messages from the browser process:
@@ -80,9 +73,6 @@ class CredentialManagerClient : public blink::WebCredentialManagerClient,
       const blink::WebVector<blink::WebURL>& federations,
       RequestCallbacks* callbacks) OVERRIDE;
 
- protected:
-  virtual int GetRoutingID();
-
  private:
   typedef IDMap<blink::WebCredentialManagerClient::RequestCallbacks,
                 IDMapOwnPointer> RequestCallbacksMap;
@@ -92,13 +82,6 @@ class CredentialManagerClient : public blink::WebCredentialManagerClient,
   void RespondToNotificationCallback(int request_id,
                                      NotificationCallbacksMap* map);
 
-  // Nulls out the raw pointer to |render_thread_| after ensuring that any
-  // message routing is removed.
-  void DisconnectFromRenderThread();
-
-  int routing_id_;
-  content::RenderThread* render_thread_;
-
   // Track the various blink::WebCredentialManagerClient::*Callbacks objects
   // generated from Blink. This class takes ownership of these objects.
   NotificationCallbacksMap failed_sign_in_callbacks_;

components/password_manager/content/renderer/credential_manager_client_unittest.cc → components/password_manager/content/renderer/credential_manager_client_browsertest.cc

@@ -4,9 +4,7 @@
 
 #include "components/password_manager/content/common/credential_manager_messages.h"
 #include "components/password_manager/content/renderer/credential_manager_client.h"
-#include "components/password_manager/content/renderer/test_credential_manager_client.h"
-#include "content/public/test/mock_render_thread.h"
-#include "content/test/blink_test_environment.h"
+#include "content/public/test/render_view_test.h"
 #include "ipc/ipc_test_sink.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/platform/WebCredential.h"
@@ -17,22 +15,24 @@ namespace password_manager {
 
 namespace {
 
-class CredentialManagerClientTest : public testing::Test {
+class CredentialManagerClientTest : public content::RenderViewTest {
  public:
   CredentialManagerClientTest()
-      : callback_errored_(false),
-        callback_succeeded_(false) {
-    blink::WebString string = blink::WebString::fromUTF8("");
-    GURL url("https://example.com/image");
-    credential_.reset(new blink::WebCredential(string, string, url));
-  }
+      : callback_errored_(false), callback_succeeded_(false) {}
   virtual ~CredentialManagerClientTest() {}
 
-  static void SetUpTestCase() { content::SetUpBlinkTestEnvironment(); }
+  virtual void SetUp() OVERRIDE {
+    content::RenderViewTest::SetUp();
+    credential_.reset(new blink::WebCredential("", "", GURL()));
+    client_.reset(new CredentialManagerClient(view_));
+  }
 
-  static void TearDownTestCase() { content::TearDownBlinkTestEnvironment(); }
+  virtual void TearDown() OVERRIDE {
+    credential_.reset();
+    content::RenderViewTest::TearDown();
+  }
 
-  IPC::TestSink& sink() { return render_thread_.sink(); }
+  IPC::TestSink& sink() { return render_thread_->sink(); }
 
   blink::WebCredential* credential() { return credential_.get(); }
 
@@ -91,8 +91,7 @@ class CredentialManagerClientTest : public testing::Test {
   void set_callback_succeeded(bool state) { callback_succeeded_ = state; }
 
  protected:
-  content::MockRenderThread render_thread_;
-  TestCredentialManagerClient client_;
+  scoped_ptr<CredentialManagerClient> client_;
 
   // True if a message's callback's 'onSuccess'/'onError' methods were called,
   // false otherwise. We put these on the test object rather than on the
@@ -109,8 +108,7 @@ class TestNotificationCallbacks
     : public blink::WebCredentialManagerClient::NotificationCallbacks {
  public:
   explicit TestNotificationCallbacks(CredentialManagerClientTest* test)
-      : test_(test) {
-  }
+      : test_(test) {}
 
   virtual ~TestNotificationCallbacks() {}
 
@@ -128,8 +126,7 @@ class TestRequestCallbacks
     : public blink::WebCredentialManagerClient::RequestCallbacks {
  public:
   explicit TestRequestCallbacks(CredentialManagerClientTest* test)
-      : test_(test) {
-  }
+      : test_(test) {}
 
   virtual ~TestRequestCallbacks() {}
 
@@ -154,12 +151,12 @@ TEST_F(CredentialManagerClientTest, SendNotifyFailedSignIn) {
 
   scoped_ptr<TestNotificationCallbacks> callbacks(
       new TestNotificationCallbacks(this));
-  client_.dispatchFailedSignIn(*credential(), callbacks.release());
+  client_->dispatchFailedSignIn(*credential(), callbacks.release());
 
   EXPECT_TRUE(ExtractRequestId(CredentialManagerHostMsg_NotifyFailedSignIn::ID,
                                request_id));
 
-  client_.OnAcknowledgeFailedSignIn(request_id);
+  client_->OnAcknowledgeFailedSignIn(request_id);
   EXPECT_TRUE(callback_succeeded());
   EXPECT_FALSE(callback_errored());
 }
@@ -171,12 +168,12 @@ TEST_F(CredentialManagerClientTest, SendNotifySignedIn) {
 
   scoped_ptr<TestNotificationCallbacks> callbacks(
       new TestNotificationCallbacks(this));
-  client_.dispatchSignedIn(*credential(), callbacks.release());
+  client_->dispatchSignedIn(*credential(), callbacks.release());
 
   EXPECT_TRUE(ExtractRequestId(CredentialManagerHostMsg_NotifySignedIn::ID,
                                request_id));
 
-  client_.OnAcknowledgeSignedIn(request_id);
+  client_->OnAcknowledgeSignedIn(request_id);
   EXPECT_TRUE(callback_succeeded());
   EXPECT_FALSE(callback_errored());
 }
@@ -188,12 +185,12 @@ TEST_F(CredentialManagerClientTest, SendNotifySignedOut) {
 
   scoped_ptr<TestNotificationCallbacks> callbacks(
       new TestNotificationCallbacks(this));
-  client_.dispatchSignedOut(callbacks.release());
+  client_->dispatchSignedOut(callbacks.release());
 
   EXPECT_TRUE(ExtractRequestId(CredentialManagerHostMsg_NotifySignedOut::ID,
                                request_id));
 
-  client_.OnAcknowledgeSignedOut(request_id);
+  client_->OnAcknowledgeSignedOut(request_id);
   EXPECT_TRUE(callback_succeeded());
   EXPECT_FALSE(callback_errored());
 }
@@ -205,13 +202,13 @@ TEST_F(CredentialManagerClientTest, SendRequestCredential) {
 
   scoped_ptr<TestRequestCallbacks> callbacks(new TestRequestCallbacks(this));
   std::vector<GURL> federations;
-  client_.dispatchRequest(false, federations, callbacks.release());
+  client_->dispatchRequest(false, federations, callbacks.release());
 
   EXPECT_TRUE(ExtractRequestId(CredentialManagerHostMsg_RequestCredential::ID,
                                request_id));
 
   CredentialInfo info;
-  client_.OnSendCredential(request_id, info);
+  client_->OnSendCredential(request_id, info);
   EXPECT_TRUE(callback_succeeded());
   EXPECT_FALSE(callback_errored());
 }

components/password_manager/content/renderer/test_credential_manager_client.cc

-// Copyright 2014 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 "components/password_manager/content/renderer/test_credential_manager_client.h"
-
-namespace password_manager {
-
-TestCredentialManagerClient::TestCredentialManagerClient() {
-}
-
-TestCredentialManagerClient::~TestCredentialManagerClient() {
-}
-
-int TestCredentialManagerClient::GetRoutingID() {
-  // Chosen by fair dice roll.
-  return 4;
-}
-
-}  // namespace password_manager

components/password_manager/content/renderer/test_credential_manager_client.h

-// Copyright 2014 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_PASSWORD_MANAGER_CONTENT_RENDERER_TEST_CREDENTIAL_MANAGER_CLIENT_H_
-#define COMPONENTS_PASSWORD_MANAGER_CONTENT_RENDERER_TEST_CREDENTIAL_MANAGER_CLIENT_H_
-
-#include "base/basictypes.h"
-#include "base/compiler_specific.h"
-#include "components/password_manager/content/renderer/credential_manager_client.h"
-
-namespace password_manager {
-
-class TestCredentialManagerClient : public CredentialManagerClient {
- public:
-  TestCredentialManagerClient();
-  virtual ~TestCredentialManagerClient();
-
-  virtual int GetRoutingID() OVERRIDE;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestCredentialManagerClient);
-};
-
-}  // namespace password_manager
-
-#endif  // COMPONENTS_PASSWORD_MANAGER_CONTENT_RENDERER_TEST_CREDENTIAL_MANAGER_CLIENT_H_