[Payments] Code Refactoring for the Subkey Request

chrome/browser/autofill/android/personal_data_manager_android.cc

 // 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.
 
 #include "chrome/browser/autofill/android/personal_data_manager_android.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <memory>
 #include <utility>
@@ skipping 289 matching lines @@
   }
 
  private:
   ~AndroidAddressNormalizerDelegate() override {}
 
   ScopedJavaGlobalRef<jobject> jdelegate_;
 
   DISALLOW_COPY_AND_ASSIGN(AndroidAddressNormalizerDelegate);
 };
 
-class AndroidSubKeyRequesterDelegate
-    : public PersonalDataManagerAndroid::SubKeyRequestDelegate,
-      public base::SupportsWeakPtr<AndroidSubKeyRequesterDelegate> {
+class AndroidSubKeyReceiver {

[David Trainor- moved to gerrit, 2017/05/23 21:03:59]

Don't even need this now :).  Remove the class and just use something like:

void HandleSubKeysReceived(ScopedJavaGlobalRef<jobject> delegate, const
std::vector<std::string>& sub_keys) {
  JNIEnv* env = base::android::AttachCurrentThread();
  Java_GetSubKeysRequestDelegate_onSubKeysReceived(env, delegate,
base::android::ToJavaArrayOfStrings(env, sub_keys));
}


...

base::OnceCallback<void(const std::vector<std::string>&)> cb =
base::BindOnce(&HandleSubKeysReceived, ScopedJavaGlobalRef<jobject>(jdelegate));

...

[Parastoo, 2017/05/24 14:57:20]

Then we would need to have the definition of the new function in
SubKeyRequester, which would require the subkey_requester.h to understand
ScopedJavaGlobalRef, which is not possible because there is no jni file for that
file. (also not very modular.)
+ The SubKeyRequester may be later used in non-android applications, which means
that we should deal with the ScopedJavaGlobalRef, .. in this file.

[Parastoo, 2017/05/24 14:59:15]

Correcting a typo: we should NOT deal with the ScopedJavaGlobalRef, .. in this
file.*

[David Trainor- moved to gerrit, 2017/05/24 15:29:37]

Look at "Partial Binding Of Parameters" at
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md.

base::Bind solves this for you by allowing you to partially bind the parameters
(yay!).  That means you can pass in the ScopedJavaGlobalRef<> in this file and
SubKeyRequester will only ever see or know about a callback that takes an
std::vector<std::string>.  That's exactly the behavior you want.

Notice that in your current code, SubKeyRequester doesn't need to know about the
AndroidSubKeyReceiver you're passing in to the callback?  Same kind of concept.

From the code above, this will partially bind the jobject so the callback you're
left with only takes an std::vector<std::string> (see the callback parameters
and how they match your signature below):

base::OnceCallback<void(const std::vector<std::string>&)> cb =
base::BindOnce(&HandleSubKeysReceived, ScopedJavaGlobalRef<jobject>(jdelegate));

[Parastoo, 2017/05/24 18:53:57]

Cool! Thank you.
Done.

  public:
-  AndroidSubKeyRequesterDelegate(
-      JNIEnv* env,
-      const base::android::JavaParamRef<jobject>& jdelegate,
-      const std::string& region_code,
-      base::WeakPtr<PersonalDataManagerAndroid> personal_data_manager_android) {
+  AndroidSubKeyReceiver(JNIEnv* env,
+                        const base::android::JavaParamRef<jobject>& jdelegate) {
     jdelegate_.Reset(env, jdelegate);
-    region_code_ = region_code;
-    personal_data_manager_android_ = personal_data_manager_android;
   }
 
-  ~AndroidSubKeyRequesterDelegate() override {}
+  void OnSubKeysReceived(std::vector<std::string> sub_keys) {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    Java_GetSubKeysRequestDelegate_onSubKeysReceived(
+        env, jdelegate_, base::android::ToJavaArrayOfStrings(env, sub_keys));
+  }
 
  private:
-  // PersonalDataManagerAndroid::SubKeyRequestDelegate:
-  void OnRulesSuccessfullyLoaded() override {
-    if (personal_data_manager_android_) {
-      JNIEnv* env = base::android::AttachCurrentThread();
-      Java_GetSubKeysRequestDelegate_onSubKeysReceived(
-          env, jdelegate_,
-          personal_data_manager_android_->GetSubKeys(env, region_code_));
-    }
-  }
+  ScopedJavaGlobalRef<jobject> jdelegate_;
 
-  ScopedJavaGlobalRef<jobject> jdelegate_;
-  std::string region_code_;
-  base::WeakPtr<PersonalDataManagerAndroid> personal_data_manager_android_;
-
-  DISALLOW_COPY_AND_ASSIGN(AndroidSubKeyRequesterDelegate);
+  DISALLOW_COPY_AND_ASSIGN(AndroidSubKeyReceiver);
 };
 
 }  // namespace
 
 PersonalDataManagerAndroid::PersonalDataManagerAndroid(JNIEnv* env, jobject obj)
     : weak_java_obj_(env, obj),
       personal_data_manager_(PersonalDataManagerFactory::GetForProfile(
           ProfileManager::GetActiveUserProfile())),
       address_normalizer_(
           std::unique_ptr<::i18n::addressinput::Source>(
               new autofill::ChromeMetadataSource(
                   I18N_ADDRESS_VALIDATION_DATA_URL,
                   personal_data_manager_->GetURLRequestContextGetter())),
           ValidationRulesStorageFactory::CreateStorage()),
-      address_validator_(
+      subkey_requester_(
           base::MakeUnique<autofill::ChromeMetadataSource>(
               I18N_ADDRESS_VALIDATION_DATA_URL,
               personal_data_manager_->GetURLRequestContextGetter()),
-          ValidationRulesStorageFactory::CreateStorage(),
-          this) {
+          ValidationRulesStorageFactory::CreateStorage()) {
   personal_data_manager_->AddObserver(this);
 }
 
 PersonalDataManagerAndroid::~PersonalDataManagerAndroid() {
   personal_data_manager_->RemoveObserver(this);
 }
 
 jboolean PersonalDataManagerAndroid::IsDataLoaded(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& unused_obj) const {
@@ skipping 373 matching lines @@
     const base::android::JavaParamRef<jobject>& unused_obj,
     const base::android::JavaParamRef<jstring>& jregion_code) {
   address_normalizer_.LoadRulesForRegion(
       ConvertJavaStringToUTF8(env, jregion_code));
 }
 
 void PersonalDataManagerAndroid::LoadRulesForSubKeys(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& unused_obj,
     const base::android::JavaParamRef<jstring>& jregion_code) {
-  address_validator_.LoadRules(ConvertJavaStringToUTF8(env, jregion_code));
+  subkey_requester_.LoadRulesForRegion(
+      ConvertJavaStringToUTF8(env, jregion_code));
 }
 
 void PersonalDataManagerAndroid::StartAddressNormalization(
     JNIEnv* env,
     const JavaParamRef<jobject>& unused_obj,
     const JavaParamRef<jobject>& jprofile,
     const JavaParamRef<jstring>& jregion_code,
     jint jtimeout_seconds,
     const JavaParamRef<jobject>& jdelegate) {
   const std::string region_code = ConvertJavaStringToUTF8(env, jregion_code);
@@ skipping 15 matching lines @@
     const base::android::JavaParamRef<jobject>& unused_obj) {
   return !personal_data_manager_->GetProfiles().empty();
 }
 
 jboolean PersonalDataManagerAndroid::HasCreditCards(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& unused_obj) {
   return !personal_data_manager_->GetCreditCards().empty();
 }
 
-base::android::ScopedJavaLocalRef<jobjectArray>
-PersonalDataManagerAndroid::GetSubKeys(JNIEnv* env,
-                                       const std::string& region_code) {
-  std::vector<std::string> sub_keys =
-      address_validator_.GetRegionSubKeys(region_code);
-  return base::android::ToJavaArrayOfStrings(env, sub_keys);
-}
-
-void PersonalDataManagerAndroid::OnAddressValidationRulesLoaded(
-    const std::string& region_code,
-    bool success) {
-  // if |success| == false, AddressValidator::GetRegionSubKeys will return an
-  // empty list of sub-keys. => No need to check for |success|.
-  // Check if there is any sub-key request for that region code.
-  if (!pending_subkey_region_code_.compare(region_code))
-    pending_subkey_request_->OnRulesSuccessfullyLoaded();
-  pending_subkey_region_code_.clear();
-  pending_subkey_request_.reset();
-}
-
 void PersonalDataManagerAndroid::StartRegionSubKeysRequest(
     JNIEnv* env,
     const JavaParamRef<jobject>& unused_obj,
     const JavaParamRef<jstring>& jregion_code,
+    jint jtimeout_seconds,
     const JavaParamRef<jobject>& jdelegate) {
   const std::string region_code = ConvertJavaStringToUTF8(env, jregion_code);
-  std::unique_ptr<SubKeyRequestDelegate> requester =
-      base::MakeUnique<AndroidSubKeyRequesterDelegate>(
-          env, jdelegate, region_code, AsWeakPtr());
 
-  if (AreRulesLoadedForRegion(region_code)) {
-    requester->OnRulesSuccessfullyLoaded();
-  } else {
-    // Setup the variables so that the sub-keys request is sent, when the rules
-    // are loaded.
-    pending_subkey_region_code_ = region_code;
-    pending_subkey_request_ = std::move(requester);
-  }
+  std::unique_ptr<AndroidSubKeyReceiver> android_subkey_receiver_(
+      base::MakeUnique<AndroidSubKeyReceiver>(env, jdelegate));
+
+  base::OnceCallback<void(std::vector<std::string>)> cb =
+      base::BindOnce(&AndroidSubKeyReceiver::OnSubKeysReceived,
+                     std::move(android_subkey_receiver_));
+
+  subkey_requester_.StartRegionSubKeysRequest(region_code, jtimeout_seconds,
+                                              std::move(cb));
 }
 
 void PersonalDataManagerAndroid::CancelPendingGetSubKeys(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& unused_obj) {
-  pending_subkey_region_code_.clear();
-  pending_subkey_request_.reset();
+  subkey_requester_.CancelPendingGetSubKeys();
 }
 
 ScopedJavaLocalRef<jobjectArray> PersonalDataManagerAndroid::GetProfileGUIDs(
     JNIEnv* env,
     const std::vector<AutofillProfile*>& profiles) {
   std::vector<base::string16> guids;
   for (AutofillProfile* profile : profiles)
     guids.push_back(base::UTF8ToUTF16(profile->guid()));
 
   return base::android::ToJavaArrayOfStrings(env, guids);
 }
 
 ScopedJavaLocalRef<jobjectArray> PersonalDataManagerAndroid::GetCreditCardGUIDs(
     JNIEnv* env,
     const std::vector<CreditCard*>& credit_cards) {
   std::vector<base::string16> guids;
   for (CreditCard* credit_card : credit_cards)
     guids.push_back(base::UTF8ToUTF16(credit_card->guid()));
 
   return base::android::ToJavaArrayOfStrings(env, guids);
 }
 
-bool PersonalDataManagerAndroid::AreRulesLoadedForRegion(
-    const std::string& region_code) {
-  return address_validator_.AreRulesLoadedForRegion(region_code);
-}
-
 ScopedJavaLocalRef<jobjectArray> PersonalDataManagerAndroid::GetProfileLabels(
     JNIEnv* env,
     bool address_only,
     bool include_name_in_label,
     bool include_organization_in_label,
     bool include_country_in_label,
     std::vector<AutofillProfile*> profiles) {
   std::unique_ptr<std::vector<ServerFieldType>> suggested_fields;
   size_t minimal_fields_shown = 2;
   if (address_only) {
@@ skipping 97 matching lines @@
                base::android::ConvertJavaStringToUTF16(env, jcountry_name)));
 }
 
 static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   PersonalDataManagerAndroid* personal_data_manager_android =
       new PersonalDataManagerAndroid(env, obj);
   return reinterpret_cast<intptr_t>(personal_data_manager_android);
 }
 
 }  // namespace autofill