[Mfill Android] Observer PersonalDataManager to update addresses

The address controller will be asked to provide addresses only once when
it is initialized. After that, it needs to push any changes made to
profiles whenever they change.
For that purpose, observe the PersonalDataManager as soon as it was used
for the first time.

Bug: 967380
Change-Id: Ia6b1ad02edce92b31e5d4951f06ae8933be3fed7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1631594
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663819}

chrome/browser/autofill/address_accessory_controller_impl.cc

@@ -77,7 +77,10 @@ std::vector<FooterCommand> CreateManageAddressesFooter() {
 
 }  // namespace
 
-AddressAccessoryControllerImpl::~AddressAccessoryControllerImpl() = default;
+AddressAccessoryControllerImpl::~AddressAccessoryControllerImpl() {
+  if (personal_data_manager_)
+    personal_data_manager_->RemoveObserver(this);
+}
 
 // static
 bool AddressAccessoryController::AllowedForWebContents(
@@ -141,44 +144,42 @@ void AddressAccessoryControllerImpl::RefreshSuggestions() {
           UserInfosForProfiles(profiles), CreateManageAddressesFooter()));
 }
 
+void AddressAccessoryControllerImpl::OnPersonalDataChanged() {
+  RefreshSuggestions();
+}
 // static
 void AddressAccessoryControllerImpl::CreateForWebContentsForTesting(
     content::WebContents* web_contents,
-    base::WeakPtr<ManualFillingController> mf_controller,
-    autofill::PersonalDataManager* personal_data_manager) {
+    base::WeakPtr<ManualFillingController> mf_controller) {
   DCHECK(web_contents) << "Need valid WebContents to attach controller to!";
   DCHECK(!FromWebContents(web_contents)) << "Controller already attached!";
   DCHECK(mf_controller);
 
-  web_contents->SetUserData(
-      UserDataKey(),
-      base::WrapUnique(new AddressAccessoryControllerImpl(
-          web_contents, std::move(mf_controller), personal_data_manager)));
+  web_contents->SetUserData(UserDataKey(),
+                            base::WrapUnique(new AddressAccessoryControllerImpl(
+                                web_contents, std::move(mf_controller))));
 }
 
 AddressAccessoryControllerImpl::AddressAccessoryControllerImpl(
     content::WebContents* web_contents)
-    : web_contents_(web_contents),
-      personal_data_manager_for_testing_(nullptr) {}
+    : AddressAccessoryControllerImpl(web_contents, nullptr) {}
 
 // Additional creation functions in unit tests only:
 AddressAccessoryControllerImpl::AddressAccessoryControllerImpl(
     content::WebContents* web_contents,
-    base::WeakPtr<ManualFillingController> mf_controller,
-    autofill::PersonalDataManager* personal_data_manager)
+    base::WeakPtr<ManualFillingController> mf_controller)
     : web_contents_(web_contents),
       mf_controller_(std::move(mf_controller)),
-      personal_data_manager_for_testing_(personal_data_manager) {}
+      personal_data_manager_(nullptr) {}
 
 std::vector<AutofillProfile*> AddressAccessoryControllerImpl::GetProfiles() {
-  const autofill::PersonalDataManager* data_manager =
-      personal_data_manager_for_testing_;
-  if (!data_manager)
-    data_manager = autofill::PersonalDataManagerFactory::GetForProfile(
-        Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
-  if (!data_manager)
-    return {};  // No data available yet or anymore!
-  return data_manager->GetProfilesToSuggest();
+  if (!personal_data_manager_) {
+    personal_data_manager_ =
+        autofill::PersonalDataManagerFactory::GetForProfile(
+            Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
+    personal_data_manager_->AddObserver(this);
+  }
+  return personal_data_manager_->GetProfilesToSuggest();
 }
 
 base::WeakPtr<ManualFillingController>

chrome/browser/autofill/address_accessory_controller_impl.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/autofill/address_accessory_controller.h"
+#include "components/autofill/core/browser/personal_data_manager_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "url/gurl.h"
 
@@ -26,6 +27,7 @@ class PersonalDataManager;
 // contents of one of its frames.
 class AddressAccessoryControllerImpl
     : public AddressAccessoryController,
+      public PersonalDataManagerObserver,
       public content::WebContentsUserData<AddressAccessoryControllerImpl> {
  public:
   ~AddressAccessoryControllerImpl() override;
@@ -37,13 +39,15 @@ class AddressAccessoryControllerImpl
   // AddressAccessoryController:
   void RefreshSuggestions() override;
 
+  // PersonalDataManagerObserver:
+  void OnPersonalDataChanged() override;
+
   // Like |CreateForWebContents|, it creates the controller and attaches it to
   // the given |web_contents|. Additionally, it allows inject a manual filling
   // controller.
   static void CreateForWebContentsForTesting(
       content::WebContents* web_contents,
-      base::WeakPtr<ManualFillingController> mf_controller,
-      autofill::PersonalDataManager* personal_data_manager);
+      base::WeakPtr<ManualFillingController> mf_controller);
 
  private:
   friend class content::WebContentsUserData<AddressAccessoryControllerImpl>;
@@ -53,11 +57,10 @@ class AddressAccessoryControllerImpl
 
   std::vector<autofill::AutofillProfile*> GetProfiles();
 
-  // Constructor that allows to inject a mock or fake view.
+  // Constructor that allows to inject a mock filling controller.
   AddressAccessoryControllerImpl(
       content::WebContents* web_contents,
-      base::WeakPtr<ManualFillingController> mf_controller,
-      autofill::PersonalDataManager* personal_data_manager);
+      base::WeakPtr<ManualFillingController> mf_controller);
 
   // Lazy-initializes and returns the ManualFillingController for the current
   // |web_contents_|. The lazy initialization allows injecting mocks for tests.
@@ -69,8 +72,8 @@ class AddressAccessoryControllerImpl
   // The password accessory controller object to forward client requests to.
   base::WeakPtr<ManualFillingController> mf_controller_;
 
-  // The data manager used to retrieve the profiles in tests.
-  const autofill::PersonalDataManager* personal_data_manager_for_testing_;
+  // The data manager used to retrieve the profiles.
+  autofill::PersonalDataManager* personal_data_manager_;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 

chrome/browser/autofill/address_accessory_controller_impl_unittest.cc

@@ -9,12 +9,13 @@
 #include <ostream>
 #include <string>
 #include <vector>
-
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/mock_callback.h"
 #include "chrome/browser/autofill/mock_manual_filling_controller.h"
+#include "chrome/browser/autofill/personal_data_manager_factory.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/autofill/core/browser/autofill_test_utils.h"
 #include "components/autofill/core/browser/test_personal_data_manager.h"
 #include "components/strings/grit/components_strings.h"
@@ -52,6 +53,13 @@ AccessorySheetData::Builder AddressAccessorySheetDataBuilder(
                            AccessoryAction::MANAGE_ADDRESSES);
 }
 
+std::unique_ptr<KeyedService> BuildTestPersonalDataManager(
+    content::BrowserContext* context) {
+  auto personal_data_manager = std::make_unique<TestPersonalDataManager>();
+  personal_data_manager->SetAutofillProfileEnabled(true);
+  return personal_data_manager;
+}
+
 }  // namespace
 
 class AddressAccessoryControllerTest : public ChromeRenderViewHostTestHarness {
@@ -60,22 +68,27 @@ class AddressAccessoryControllerTest : public ChromeRenderViewHostTestHarness {
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
-    personal_data_manager_.SetAutofillProfileEnabled(true);
+    PersonalDataManagerFactory::GetInstance()->SetTestingFactory(
+        GetBrowserContext(),
+        base::BindRepeating(&BuildTestPersonalDataManager));
 
     AddressAccessoryControllerImpl::CreateForWebContentsForTesting(
-        web_contents(), mock_manual_filling_controller_.AsWeakPtr(),
-        &personal_data_manager_);
+        web_contents(), mock_manual_filling_controller_.AsWeakPtr());
   }
 
-  void TearDown() override { personal_data_manager_.ClearProfiles(); }
+  void TearDown() override { personal_data_manager()->ClearProfiles(); }
 
   AddressAccessoryController* controller() {
     return AddressAccessoryControllerImpl::FromWebContents(web_contents());
   }
 
+  TestPersonalDataManager* personal_data_manager() {
+    return static_cast<TestPersonalDataManager*>(
+        PersonalDataManagerFactory::GetForProfile(profile()));
+  }
+
  protected:
   StrictMock<MockManualFillingController> mock_manual_filling_controller_;
-  autofill::TestPersonalDataManager personal_data_manager_;
 };
 
 TEST_F(AddressAccessoryControllerTest, IsNotRecreatedForSameWebContents) {
@@ -89,10 +102,9 @@ TEST_F(AddressAccessoryControllerTest, IsNotRecreatedForSameWebContents) {
 
 TEST_F(AddressAccessoryControllerTest, RefreshSuggestionsCallsUI) {
   AutofillProfile canadian = test::GetFullValidProfileForCanada();
-  personal_data_manager_.AddProfile(canadian);
+  personal_data_manager()->AddProfile(canadian);
 
-  autofill::AccessorySheetData result(autofill::AccessoryTabType::PASSWORDS,
-                                      base::string16());
+  AccessorySheetData result(AccessoryTabType::PASSWORDS, base::string16());
   EXPECT_CALL(mock_manual_filling_controller_,
               RefreshSuggestionsForField(
                   mojom::FocusedFieldType::kFillableTextField, _))
@@ -128,8 +140,7 @@ TEST_F(AddressAccessoryControllerTest, RefreshSuggestionsCallsUI) {
 }
 
 TEST_F(AddressAccessoryControllerTest, ProvidesEmptySuggestionsMessage) {
-  autofill::AccessorySheetData result(autofill::AccessoryTabType::PASSWORDS,
-                                      base::string16());
+  AccessorySheetData result(AccessoryTabType::PASSWORDS, base::string16());
   EXPECT_CALL(mock_manual_filling_controller_,
               RefreshSuggestionsForField(
                   mojom::FocusedFieldType::kFillableTextField, _))
@@ -141,4 +152,47 @@ TEST_F(AddressAccessoryControllerTest, ProvidesEmptySuggestionsMessage) {
             AddressAccessorySheetDataBuilder(addresses_empty_str()).Build());
 }
 
+TEST_F(AddressAccessoryControllerTest, TriggersRefreshWhenDataChanges) {
+  AccessorySheetData result(AccessoryTabType::PASSWORDS, base::string16());
+  EXPECT_CALL(mock_manual_filling_controller_,
+              RefreshSuggestionsForField(
+                  mojom::FocusedFieldType::kFillableTextField, _))
+      .WillRepeatedly(SaveArg<1>(&result));
+
+  // A refresh without data stores an empty sheet and registers an observer.
+  controller()->RefreshSuggestions();
+  ASSERT_EQ(result,
+            AddressAccessorySheetDataBuilder(addresses_empty_str()).Build());
+
+  // When new data is added, a refresh is automatically triggered.
+  AutofillProfile email = test::GetIncompleteProfile2();
+  personal_data_manager()->AddProfile(email);
+  ASSERT_EQ(result, AddressAccessorySheetDataBuilder(base::string16())
+                        .AddUserInfo()
+                        /*name first:*/
+                        .AppendSimpleField(base::string16())
+                        /*name middle:*/
+                        .AppendSimpleField(base::string16())
+                        /*name last:*/
+                        .AppendSimpleField(base::string16())
+                        /*company name:*/
+                        .AppendSimpleField(base::string16())
+                        /*address line1:*/
+                        .AppendSimpleField(base::string16())
+                        /*address line2:*/
+                        .AppendSimpleField(base::string16())
+                        /*address zip:*/
+                        .AppendSimpleField(base::string16())
+                        /*address city:*/
+                        .AppendSimpleField(base::string16())
+                        /*address state:*/
+                        .AppendSimpleField(base::string16())
+                        /*address country:*/
+                        .AppendSimpleField(base::string16())
+                        /*phone number:*/.AppendSimpleField(base::string16())
+                        .AppendSimpleField(
+                            email.GetRawInfo(ServerFieldType::EMAIL_ADDRESS))
+                        .Build());
+}
+
 }  // namespace autofill