[Payments] Code Refactoring for the Subkey Request

chrome/browser/autofill/android/personal_data_manager_android.cc

Index: chrome/browser/autofill/android/personal_data_manager_android.cc
diff --git a/chrome/browser/autofill/android/personal_data_manager_android.cc b/chrome/browser/autofill/android/personal_data_manager_android.cc
index 34b2b9c3bbba87a64a87bb8d3ec31ba6023225e9..8ce9967f8457e3f7529e0b20db942d2f769035a1 100644
--- a/chrome/browser/autofill/android/personal_data_manager_android.cc
+++ b/chrome/browser/autofill/android/personal_data_manager_android.cc
@@ -307,38 +307,23 @@ class AndroidAddressNormalizerDelegate
   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 {}
-
- 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_));
-    }
+  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:
   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
@@ -353,12 +338,11 @@ PersonalDataManagerAndroid::PersonalDataManagerAndroid(JNIEnv* env, jobject obj)
                   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);
 }
 
@@ -752,7 +736,8 @@ 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(
@@ -788,51 +773,29 @@ jboolean PersonalDataManagerAndroid::HasCreditCards(
   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(
@@ -855,11 +818,6 @@ ScopedJavaLocalRef<jobjectArray> PersonalDataManagerAndroid::GetCreditCardGUIDs(
   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,