Extract PasswordController functionality into a shareable location

- Created SharedPasswordController to share ios specific code without
  being either ios/chrome or ios/web_view specific.
- Move MakeSimpleFormData into test_helpers.
- Make PasswordSuggestionHelper and PasswordFormHelper injected at
  initialization to aid with mocking in unit test.
- Left some TODOs to continue cleanup of shared code in
  //components/password_manager/ios/...

Change-Id: Ia877ecef4cb44d87f069500fbccb1d0f7320a598
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2254351
Commit-Queue: John Wu <jzw@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Maria Kazinova <kazinova@google.com>
Cr-Commit-Position: refs/heads/master@{#790517}

components/password_manager/ios/BUILD.gn

@@ -9,6 +9,7 @@ component("ios") {
 
   deps = [
     "//base",
+    "//components/autofill/core/browser",
     "//components/autofill/core/common",
     "//components/autofill/ios/browser",
     "//components/autofill/ios/form_util",
@@ -16,10 +17,12 @@ component("ios") {
     "//components/password_manager/core/browser/form_parsing",
     "//components/password_manager/core/common",
     "//components/security_state/ios",
+    "//components/strings:components_strings_grit",
     "//ios/web/common",
     "//ios/web/public",
     "//ios/web/public/deprecated",
     "//ios/web/public/js_messaging",
+    "//ui/base",
     "//url",
   ]
 
@@ -36,6 +39,8 @@ component("ios") {
     "password_manager_driver_bridge.h",
     "password_suggestion_helper.h",
     "password_suggestion_helper.mm",
+    "shared_password_controller.h",
+    "shared_password_controller.mm",
     "unique_id_tab_helper.h",
     "unique_id_tab_helper.mm",
   ]
@@ -63,6 +68,7 @@ source_set("unit_tests") {
     "account_select_fill_data_unittest.cc",
     "credential_manager_util_unittest.cc",
     "password_form_helper_unittest.mm",
+    "shared_password_controller_unittest.mm",
     "unique_id_tab_helper_unittest.mm",
   ]
   deps = [
@@ -72,11 +78,14 @@ source_set("unit_tests") {
     "//base",
     "//base/test:test_support",
     "//components/autofill/core/common",
+    "//components/autofill/ios/browser",
     "//components/password_manager/core/browser",
     "//components/password_manager/core/browser:test_support",
     "//ios/web/public/test",
     "//ios/web/public/test/fakes",
+    "//testing/gmock",
     "//testing/gtest",
+    "//third_party/ocmock",
   ]
 }
 

components/password_manager/ios/DEPS

@@ -7,4 +7,5 @@ include_rules = [
   "+ios/web/public",
   "+ios/web/common",
   "+services/network/public/cpp",
+  "+third_party/ocmock",
 ]

components/password_manager/ios/password_form_helper.h

@@ -46,6 +46,8 @@ class WebState;
 
 // Handles common form processing logic of password controller for both
 // ios/chrome and ios/web_view.
+// TODO(crbug.com/1097353): Consider folding this class into
+// SharedPasswordController.
 @interface PasswordFormHelper
     : NSObject<FormActivityObserver, CRWWebStateObserver>
 
@@ -65,6 +67,9 @@ class WebState;
 @property(nonatomic, readonly) scoped_refptr<autofill::FieldDataManager>
     fieldDataManager;
 
+// The associated delegate.
+@property(nonatomic, nullable, weak) id<PasswordFormHelperDelegate> delegate;
+
 // Uses JavaScript to find password forms. Calls |completionHandler| with the
 // extracted information used for matching and saving passwords. Calls
 // |completionHandler| with an empty vector if no password forms are found.
@@ -113,10 +118,8 @@ class WebState;
 - (void)updateFieldDataOnUserInput:(autofill::FieldRendererId)field_id
                         inputValue:(NSString*)field_value;
 
-// Creates a instance with the given WebState, observer and delegate.
+// Creates a instance with the given |webState|.
 - (instancetype)initWithWebState:(web::WebState*)webState
-                        delegate:
-                            (nullable id<PasswordFormHelperDelegate>)delegate
     NS_DESIGNATED_INITIALIZER;
 
 - (instancetype)init NS_UNAVAILABLE;

components/password_manager/ios/password_form_helper.mm

@@ -56,8 +56,6 @@ constexpr char kCommandPrefix[] = "passwordForm";
 
 @interface PasswordFormHelper ()
 
-@property(nonatomic, weak) id<PasswordFormHelperDelegate> delegate;
-
 // Handler for injected JavaScript callbacks.
 - (BOOL)handleScriptCommand:(const base::DictionaryValue&)JSONCommand;
 
@@ -95,7 +93,6 @@ constexpr char kCommandPrefix[] = "passwordForm";
 
 #pragma mark - Properties
 
-@synthesize delegate = _delegate;
 @synthesize jsPasswordManager = _jsPasswordManager;
 @synthesize fieldDataManager = _fieldDataManager;
 
@@ -105,13 +102,11 @@ constexpr char kCommandPrefix[] = "passwordForm";
 
 #pragma mark - Initialization
 
-- (instancetype)initWithWebState:(web::WebState*)webState
-                        delegate:(id<PasswordFormHelperDelegate>)delegate {
+- (instancetype)initWithWebState:(web::WebState*)webState {
   self = [super init];
   if (self) {
     DCHECK(webState);
     _webState = webState;
-    _delegate = delegate;
     _webStateObserverBridge =
         std::make_unique<web::WebStateObserverBridge>(self);
     _webState->AddObserver(_webStateObserverBridge.get());

components/password_manager/ios/password_form_helper_unittest.mm

@@ -74,8 +74,7 @@ class PasswordFormHelperTest : public web::WebTestWithWebState {
 
   void SetUp() override {
     WebTestWithWebState::SetUp();
-    helper_ =
-        [[PasswordFormHelper alloc] initWithWebState:web_state() delegate:nil];
+    helper_ = [[PasswordFormHelper alloc] initWithWebState:web_state()];
   }
 
   void TearDown() override {

components/password_manager/ios/password_manager_driver_bridge.h

@@ -16,7 +16,6 @@ class GURL;
 
 namespace password_manager {
 class PasswordGenerationFrameHelper;
-class PasswordManager;
 }  // namespace password_manager
 
 // C++ to ObjC bridge for methods of PasswordManagerDriver.
@@ -24,9 +23,6 @@ class PasswordManager;
 
 @property(readonly, nonatomic) const GURL& lastCommittedURL;
 
-@property(readonly, nonatomic)
-    password_manager::PasswordManager* passwordManager;
-
 // Finds and fills the password form using the supplied |formData| to
 // match the password form and to populate the field values. Calls
 // |completionHandler| with YES if a form field has been filled, NO otherwise.

components/password_manager/ios/password_suggestion_helper.h

@@ -40,13 +40,12 @@ class WebState;
 
 // Provides common logic of password autofill suggestions for both ios/chrome
 // and ios/web_view.
+// TODO(crbug.com/1097353): Consider folding this class into
+// SharedPasswordController.
 @interface PasswordSuggestionHelper : NSObject
 
-// Creates an instance with the given delegate.
-- (instancetype)initWithDelegate:(id<PasswordSuggestionHelperDelegate>)delegate
-    NS_DESIGNATED_INITIALIZER;
-
-- (instancetype)init NS_UNAVAILABLE;
+// Delegate to receive callbacks.
+@property(nonatomic, weak) id<PasswordSuggestionHelperDelegate> delegate;
 
 // Retrieves suggestions as username and realm pairs
 // (defined in |password_manager::UsernameAndRealm|) and converts

components/password_manager/ios/password_suggestion_helper.mm

@@ -30,13 +30,6 @@ using password_manager::FillData;
 typedef void (^PasswordSuggestionsAvailableCompletion)(
     const password_manager::AccountSelectFillData* __nullable);
 
-@interface PasswordSuggestionHelper ()
-// Delegate to receive callbacks.
-@property(nonatomic, weak, readonly) id<PasswordSuggestionHelperDelegate>
-    delegate;
-
-@end
-
 @implementation PasswordSuggestionHelper {
   // The C++ interface to cache and retrieve password suggestions.
   AccountSelectFillData _fillData;
@@ -50,14 +43,10 @@ typedef void (^PasswordSuggestionsAvailableCompletion)(
   PasswordSuggestionsAvailableCompletion _suggestionsAvailableCompletion;
 }
 
-@synthesize delegate = _delegate;
-
-- (instancetype)initWithDelegate:
-    (id<PasswordSuggestionHelperDelegate>)delegate {
+- (instancetype)init {
   self = [super init];
   if (self) {
     _sentPasswordFormToPasswordManager = NO;
-    _delegate = delegate;
   }
   return self;
 }

components/password_manager/ios/shared_password_controller.h

+// Copyright 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 COMPONENTS_PASSWORD_MANAGER_IOS_SHARED_PASSWORD_CONTROLLER_H_
+#define COMPONENTS_PASSWORD_MANAGER_IOS_SHARED_PASSWORD_CONTROLLER_H_
+
+#import <Foundation/Foundation.h>
+#include <memory>
+
+#import "components/autofill/ios/browser/form_suggestion_provider.h"
+#import "components/autofill/ios/form_util/form_activity_observer.h"
+#include "components/password_manager/core/browser/password_manager_interface.h"
+#import "components/password_manager/ios/password_form_helper.h"
+#import "components/password_manager/ios/password_manager_driver_bridge.h"
+#import "components/password_manager/ios/password_suggestion_helper.h"
+#import "ios/web/public/web_state_observer_bridge.h"
+
+namespace password_manager {
+class PasswordManagerClient;
+class PasswordManagerDriver;
+}  // namespace password_manager
+
+@class FormSuggestion;
+@class SharedPasswordController;
+
+// Protocol to define methods that must be implemented by the embedder.
+@protocol SharedPasswordControllerDelegate <NSObject>
+
+// The PasswordManagerClient owned by the delegate.
+@property(nonatomic, readonly)
+    password_manager::PasswordManagerClient* passwordManagerClient;
+
+// The PasswordManagerClient owned by the delegate.
+@property(nonatomic, readonly)
+    password_manager::PasswordManagerDriver* passwordManagerDriver;
+
+// Called to inform the delegate that it should prompt the user for a decision
+// on whether or not to use the |generatedPotentialPassword|.
+// |decisionHandler| takes a single BOOL indicating if the user accepted the
+// the suggested |generatedPotentialPassword|.
+- (void)sharedPasswordController:(SharedPasswordController*)controller
+    showGeneratedPotentialPassword:(NSString*)generatedPotentialPassword
+                   decisionHandler:(void (^)(BOOL accept))decisionHandler;
+
+// Called when SharedPasswordController accepts a suggestion displayed to the
+// user. This can be used to additional client specific behavior.
+- (void)sharedPasswordController:(SharedPasswordController*)controller
+             didAcceptSuggestion:(FormSuggestion*)suggestion;
+
+@end
+
+// Per-tab shared password controller. Handles parsing forms, loading
+// suggestions, filling forms, and generating passwords.
+@interface SharedPasswordController
+    : NSObject <CRWWebStateObserver,
+                PasswordManagerDriverBridge,
+                PasswordSuggestionHelperDelegate,
+                PasswordFormHelperDelegate,
+                FormSuggestionProvider,
+                FormActivityObserver>
+
+// Helper contains common password form processing logic.
+@property(nonatomic, readonly) PasswordFormHelper* formHelper;
+
+- (instancetype)initWithWebState:(web::WebState*)webState
+                         manager:(password_manager::PasswordManagerInterface*)
+                                     passwordManager
+                      formHelper:(PasswordFormHelper*)formHelper
+                suggestionHelper:(PasswordSuggestionHelper*)suggestionHelper
+                        delegate:(id<SharedPasswordControllerDelegate>)delegate
+    NS_DESIGNATED_INITIALIZER;
+
+- (instancetype)init NS_UNAVAILABLE;
+
+@end
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_IOS_SHARED_PASSWORD_CONTROLLER_H_

components/password_manager/ios/test_helpers.cc

@@ -96,4 +96,30 @@ void SetFormData(const std::string& origin,
   form_data->fields.push_back(field);
 }
 
+autofill::FormData MakeSimpleFormData() {
+  autofill::FormData form_data;
+  form_data.url = GURL("http://www.google.com/a/LoginAuth");
+  form_data.action = GURL("http://www.google.com/a/Login");
+  form_data.name = base::ASCIIToUTF16("login_form");
+
+  autofill::FormFieldData field;
+  field.name = base::ASCIIToUTF16("Username");
+  field.id_attribute = field.name;
+  field.name_attribute = field.name;
+  field.value = base::ASCIIToUTF16("googleuser");
+  field.form_control_type = "text";
+  field.unique_id = field.id_attribute;
+  form_data.fields.push_back(field);
+
+  field.name = base::ASCIIToUTF16("Passwd");
+  field.id_attribute = field.name;
+  field.name_attribute = field.name;
+  field.value = base::ASCIIToUTF16("p4ssword");
+  field.form_control_type = "password";
+  field.unique_id = field.id_attribute;
+  form_data.fields.push_back(field);
+
+  return form_data;
+}
+
 }  // namespace  test_helpers

components/password_manager/ios/test_helpers.h

@@ -10,6 +10,7 @@
 namespace autofill {
 struct FormData;
 struct PasswordFormFillData;
+struct FormData;
 }  // namespace autofill
 
 namespace password_manager {
@@ -51,6 +52,9 @@ void SetFormData(const std::string& origin,
                  const char* password_value,
                  autofill::FormData* form_data);
 
+// Returns a simple FormData with test values.
+autofill::FormData MakeSimpleFormData();
+
 }  // namespace test_helpers
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_IOS_TEST_HELPERS_H_

ios/chrome/browser/passwords/ios_chrome_password_manager_driver.h

@@ -26,7 +26,8 @@ class IOSChromePasswordManagerDriver
     : public password_manager::PasswordManagerDriver {
  public:
   explicit IOSChromePasswordManagerDriver(
-      id<PasswordManagerDriverBridge> bridge);
+      id<PasswordManagerDriverBridge> bridge,
+      password_manager::PasswordManager* password_manager);
   ~IOSChromePasswordManagerDriver() override;
 
   // password_manager::PasswordManagerDriver implementation.
@@ -55,6 +56,7 @@ class IOSChromePasswordManagerDriver
 
  private:
   __weak id<PasswordManagerDriverBridge> bridge_;  // (weak)
+  password_manager::PasswordManager* password_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(IOSChromePasswordManagerDriver);
 };

ios/chrome/browser/passwords/ios_chrome_password_manager_driver.mm

@@ -18,8 +18,9 @@ using password_manager::PasswordAutofillManager;
 using password_manager::PasswordManager;
 
 IOSChromePasswordManagerDriver::IOSChromePasswordManagerDriver(
-    id<PasswordManagerDriverBridge> bridge)
-    : bridge_(bridge) {}
+    id<PasswordManagerDriverBridge> bridge,
+    password_manager::PasswordManager* password_manager)
+    : bridge_(bridge), password_manager_(password_manager) {}
 
 IOSChromePasswordManagerDriver::~IOSChromePasswordManagerDriver() = default;
 
@@ -74,7 +75,7 @@ IOSChromePasswordManagerDriver::GetPasswordGenerationHelper() {
 }
 
 PasswordManager* IOSChromePasswordManagerDriver::GetPasswordManager() {
-  return [bridge_ passwordManager];
+  return password_manager_;
 }
 
 PasswordAutofillManager*

ios/chrome/browser/passwords/password_controller.h

 // Copyright 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 IOS_CHROME_BROWSER_PASSWORDS_PASSWORD_CONTROLLER_H_
 #define IOS_CHROME_BROWSER_PASSWORDS_PASSWORD_CONTROLLER_H_
 
-#import <Foundation/NSObject.h>
+#import <UIKit/UIKit.h>
 
 #include <memory>
 
@@ -21,7 +22,7 @@ class Browser;
 @class NotifyUserAutoSigninViewController;
 @protocol PasswordBreachCommands;
 @protocol PasswordsUiDelegate;
-@class UIViewController;
+@class SharedPasswordController;
 
 namespace password_manager {
 class PasswordManagerClient;
@@ -42,10 +43,8 @@ class PasswordManagerClient;
 @end
 
 // Per-tab password controller. Handles password autofill and saving.
-@interface PasswordController : NSObject <CRWWebStateObserver,
-                                          IOSChromePasswordManagerClientBridge,
-                                          PasswordManagerDriverBridge,
-                                          PasswordFormHelperDelegate>
+@interface PasswordController
+    : NSObject <CRWWebStateObserver, IOSChromePasswordManagerClientBridge>
 
 // An object that can provide suggestions from this PasswordController.
 @property(nonatomic, readonly) id<FormSuggestionProvider> suggestionProvider;
@@ -67,6 +66,11 @@ class PasswordManagerClient;
 // The browser.
 @property(nonatomic, assign) Browser* browser;
 
+// The shared password controller that handles all non //ios/chrome specific
+// business logic.
+@property(nonatomic, readonly)
+    SharedPasswordController* sharedPasswordController;
+
 // |webState| should not be nil.
 - (instancetype)initWithWebState:(web::WebState*)webState;
 

ios/chrome/browser/passwords/password_tab_helper.mm

@@ -44,7 +44,7 @@ id<FormSuggestionProvider> PasswordTabHelper::GetSuggestionProvider() {
 
 password_manager::PasswordGenerationFrameHelper*
 PasswordTabHelper::GetGenerationHelper() {
-  return controller_.passwordGenerationHelper;
+  return controller_.passwordManagerDriver->GetPasswordGenerationHelper();
 }
 
 password_manager::PasswordManager* PasswordTabHelper::GetPasswordManager() {

ios/web_view/internal/passwords/cwv_password_controller.mm

@@ -123,10 +123,10 @@ typedef void (^PasswordSuggestionsAvailableCompletion)(
     _webStateObserverBridge =
         std::make_unique<web::WebStateObserverBridge>(self);
     _webState->AddObserver(_webStateObserverBridge.get());
-    _formHelper =
-        [[PasswordFormHelper alloc] initWithWebState:webState delegate:self];
-    _suggestionHelper =
-        [[PasswordSuggestionHelper alloc] initWithDelegate:self];
+    _formHelper = [[PasswordFormHelper alloc] initWithWebState:webState];
+    _formHelper.delegate = self;
+    _suggestionHelper = [[PasswordSuggestionHelper alloc] init];
+    _suggestionHelper.delegate = self;
     _passwordManagerClient = std::move(passwordManagerClient);
     _passwordManagerClient->set_delegate(self);
     _passwordManager = std::move(passwordManager);