[Payments] Code Refactoring for the Subkey Request

components/payments/core/subkey_requester.cc

+// Copyright 2017 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/payments/core/subkey_requester.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/cancelable_callback.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/time/time.h"
+#include "third_party/libaddressinput/chromium/chrome_address_validator.h"
+#include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_data.h"
+#include "third_party/libaddressinput/src/cpp/include/libaddressinput/source.h"
+#include "third_party/libaddressinput/src/cpp/include/libaddressinput/storage.h"
+namespace payments {
+
+namespace {
+
+using ::autofill::AutofillProfile;

[sebsg, 2017/05/18 17:50:51]

needed?

[Parastoo, 2017/05/18 20:59:45]

Done.

+using ::i18n::addressinput::Source;
+using ::i18n::addressinput::Storage;
+
+class SubKeyRequest : public SubKeyRequester::Request {
+ public:
+  // The |delegate| and |address_validator| need to outlive this Request.
+  SubKeyRequest(const std::string& region_code,
+                int timeout_seconds,
+                SubKeyRequester::Delegate* delegate,
+                autofill::AddressValidator* address_validator)
+      : region_code_(region_code),
+        delegate_(delegate),
+        address_validator_(address_validator),
+        has_responded_(false),
+        on_timeout_(base::Bind(&::payments::SubKeyRequest::OnRulesLoaded,
+                               base::Unretained(this))) {
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, on_timeout_.callback(),
+        base::TimeDelta::FromSeconds(timeout_seconds));
+  }
+
+  ~SubKeyRequest() override {}
+
+  void OnRulesLoaded() override {
+    on_timeout_.Cancel();
+    // Check if the timeout happened before the rules were loaded.
+    if (has_responded_)
+      return;
+    has_responded_ = true;
+
+    delegate_->OnSubKeysReceived(
+        address_validator_->GetRegionSubKeys(region_code_));
+  }
+
+ private:
+  std::string region_code_;
+  SubKeyRequester::Delegate* delegate_;

[sebsg, 2017/05/18 17:50:51]

Please mention the lifetime of these pointer. For example see
https://cs.chromium.org/chromium/src/components/payments/content/payment_requ...

[Parastoo, 2017/05/18 21:05:21]

Done.

+  autofill::AddressValidator* address_validator_;
+
+  bool has_responded_;
+  base::CancelableCallback<void()> on_timeout_;
+
+  DISALLOW_COPY_AND_ASSIGN(SubKeyRequest);
+};
+
+}  // namespace
+
+SubKeyRequester::SubKeyRequester(std::unique_ptr<Source> source,
+                                 std::unique_ptr<Storage> storage)
+    : address_validator_(std::move(source), std::move(storage), this) {}
+
+SubKeyRequester::~SubKeyRequester() {}
+
+void SubKeyRequester::StartRegionSubKeysRequest(const std::string& region_code,
+                                                int timeout_seconds,
+                                                Delegate* requester) {
+  DCHECK(timeout_seconds >= 0);
+
+  std::unique_ptr<SubKeyRequest> request(base::MakeUnique<SubKeyRequest>(
+      region_code, timeout_seconds, requester, &address_validator_));
+
+  if (AreRulesLoadedForRegion(region_code)) {
+    request->OnRulesLoaded();
+  } else {
+    // Setup the variables so that the subkeys request is sent, when the rules
+    // are loaded.
+    pending_subkey_region_code_ = region_code;
+    pending_subkey_request_ = std::move(request);
+  }
+  // Start loading the rules for that region. If the rules were already in the
+  // process of being loaded, this call will do nothing.
+  LoadRulesForRegion(region_code);

[sebsg, 2017/05/18 17:50:51]

Move this in the "else"?

[Parastoo, 2017/05/18 20:59:45]

Done.

+}
+
+bool SubKeyRequester::AreRulesLoadedForRegion(const std::string& region_code) {
+  return address_validator_.AreRulesLoadedForRegion(region_code);
+}
+
+void SubKeyRequester::LoadRulesForRegion(const std::string& region_code) {
+  address_validator_.LoadRules(region_code);
+}
+
+void SubKeyRequester::OnAddressValidationRulesLoaded(
+    const std::string& region_code,
+    bool success) {
+  // if |success| == false, AddressValidator::GetRegionSubKeys will return an
+  // empty list of subkeys. => No need to check for |success|.
+  // Check if there is any subkey request for that region code.
+  if (!pending_subkey_region_code_.compare(region_code))
+    pending_subkey_request_->OnRulesLoaded();
+  pending_subkey_region_code_.clear();

[sebsg, 2017/05/18 17:50:51]

I'm curious about this. Do you know if my address_validator can impact this?
Let's say you start to load "US" and I start to load "CA". When "CA" gets loaded
will this function be called?

If yes, we should not reset your request and etc.
If no, we should DCHECK !pending_subkey_region_code_.compare(region_code)

[Parastoo, 2017/05/18 20:59:45]

No. 

For further reference: You (AddressNormalizer) and I (SubKeyRequester) are both
the children of the abstract class autofill::LoadRuleListener. When calling the
constructor of the autofill::AddressValidator, we each pass different type of
objects as load_rule_listener (I pass a SubKeyRequester object, you pass an
AddressNormalizer one.) So the AddressValidator will know which one is which,
here: 
(https://codesearch.chromium.org/chromium/src/third_party/libaddressinput/chro...).


I think we should not use DCHECK, because there is a case when the rules are
loaded after the timeout, or the request is cancelled when the loading has
already started. In these cases, the rules will be loaded, and this function is
called, and we should simply discard the rules.

+  pending_subkey_request_.reset();
+}
+
+void SubKeyRequester::CancelPendingGetSubKeys() {
+  pending_subkey_region_code_.clear();
+  pending_subkey_request_.reset();
+}
+
+}  // namespace payments