Load and send Wallet Risk params after user has agreed

chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc

Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index 59bf9eaa35bd0d0af5ae678e6afe085c11bf0612..a07d5cfb80f6bcbacbd6fc79f1bd42fdfd01f7b7 100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <string>
 
+#include "base/base64.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
@@ -1500,13 +1501,13 @@ DialogType AutofillDialogControllerImpl::GetDialogType() const {
 }
 
 std::string AutofillDialogControllerImpl::GetRiskData() const {
-  // TODO(dbeam): Implement this.
-  return "risky business";
+  // TODO(dbeam): remove the server restriction that this must not be empty.

[Ilya Sherman, 2013/05/22 22:34:24]

Could you file a tracking bug for the specific changes that need to be made? 
I'm assuming it's correct for the requirement to remain for GetFullWallet()
calls, right?

[Ilya Sherman, 2013/05/22 23:53:59]

^^^

[Dan Beam, 2013/05/23 00:30:40]

omitting risk params works now, I've updated the code in later patchsets

+  return risk_data_.empty() ? "risky business" : risk_data_;
 }
 
 void AutofillDialogControllerImpl::OnDidAcceptLegalDocuments() {
-  // TODO(dbeam): Don't send risk params until legal documents are accepted:
-  // http://crbug.com/173505
+  accepted_legal_documents_ = true;
+  LoadRiskFingerprintData();

[Ilya Sherman, 2013/05/22 22:34:24]

Is this method guaranteed to only be called after 'Submit' is pressed?  If so,
could you add a DCHECK to verify that invariant?

[Dan Beam, 2013/05/22 23:25:20]

Done.

 }
 
 void AutofillDialogControllerImpl::OnDidAuthenticateInstrument(bool success) {
@@ -1581,6 +1582,7 @@ void AutofillDialogControllerImpl::OnDidGetWalletItems(
     scoped_ptr<wallet::WalletItems> wallet_items) {
   legal_documents_text_.clear();
   legal_document_link_ranges_.clear();
+  accepted_legal_documents_ = false;
 
   // TODO(dbeam): verify items support kCartCurrency? http://crbug.com/232952
   wallet_items_ = wallet_items.Pass();
@@ -1594,8 +1596,7 @@ void AutofillDialogControllerImpl::OnDidSaveAddress(
 
   if (required_actions.empty()) {
     active_address_id_ = address_id;
-    if (!active_instrument_id_.empty())
-      GetFullWallet();
+    GetFullWalletIfReady();
   } else {
     HandleSaveOrUpdateRequiredActions(required_actions);
   }
@@ -1608,8 +1609,7 @@ void AutofillDialogControllerImpl::OnDidSaveInstrument(
 
   if (required_actions.empty()) {
     active_instrument_id_ = instrument_id;
-    if (!active_address_id_.empty())
-      GetFullWallet();
+    GetFullWalletIfReady();
   } else {
     HandleSaveOrUpdateRequiredActions(required_actions);
   }
@@ -1620,7 +1620,8 @@ void AutofillDialogControllerImpl::OnDidSaveInstrumentAndAddress(
     const std::string& address_id,
     const std::vector<wallet::RequiredAction>& required_actions) {
   OnDidSaveInstrument(instrument_id, required_actions);
-  OnDidSaveAddress(address_id, required_actions);
+  if (is_submitting_)
+    OnDidSaveAddress(address_id, required_actions);

[Ilya Sherman, 2013/05/22 22:34:24]

Why is the address treated differently than the payment instrument?

[Dan Beam, 2013/05/22 23:25:20]

OnDidSaveInstrument() can set is_submitting_ to false, which makes the DCHECK()
at the top of OnDidSaveAddress() fail.

[Ilya Sherman, 2013/05/22 23:53:59]

Ah, ok.  Please add a comment to this effect so that this looks less confusing /
surprising.

[Dan Beam, 2013/05/23 00:30:40]

Done.

 }
 
 void AutofillDialogControllerImpl::OnDidUpdateAddress(
@@ -1731,6 +1732,7 @@ AutofillDialogControllerImpl::AutofillDialogControllerImpl(
       weak_ptr_factory_(this),
       is_first_run_(!profile_->GetPrefs()->HasPrefPath(
           ::prefs::kAutofillDialogPayWithoutWallet)),
+      accepted_legal_documents_(false),
       is_submitting_(false),
       wallet_server_validation_error_(false),
       autocheckout_state_(AUTOCHECKOUT_NOT_STARTED),
@@ -1760,6 +1762,41 @@ bool AutofillDialogControllerImpl::IsFirstRun() const {
   return is_first_run_;
 }
 
+void AutofillDialogControllerImpl::LoadRiskFingerprintData() {
+  DCHECK(LegalDocumentsAreCurrent());
+
+  // Clear potential stale data to ensure |GetFullWalletIfReady()| triggers only
+  // when a new fingerprint is loaded.
+  risk_data_.clear();
+
+  uint64 obfuscated_gaia_id = 0;
+  bool success = base::StringToUint64(wallet_items_->obfuscated_gaia_id(),
+                                      &obfuscated_gaia_id);
+  DCHECK(success);
+
+  gfx::Rect window_bounds;
+#if !defined(OS_ANDROID)
+  window_bounds = GetBaseWindowForWebContents(web_contents())->GetBounds();
+#else
+  // TODO(dbeam): figure out the correct browser window size to pass along for
+  // android.
+#endif
+
+  PrefService* user_prefs = profile_->GetPrefs();
+  std::string charset = user_prefs->GetString(::prefs::kDefaultCharset);
+  std::string accept_languages =
+      user_prefs->GetString(::prefs::kAcceptLanguages);
+  base::Time install_time = base::Time::FromTimeT(
+      g_browser_process->local_state()->GetInt64(::prefs::kInstallDate));
+
+  risk::GetFingerprint(
+      obfuscated_gaia_id, window_bounds, *web_contents(),
+      chrome::VersionInfo().Version(), charset, accept_languages, install_time,
+      dialog_type_, g_browser_process->GetApplicationLocale(),
+      base::Bind(&AutofillDialogControllerImpl::OnDidLoadRiskFingerprintData,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 void AutofillDialogControllerImpl::OpenTabWithUrl(const GURL& url) {
 #if !defined(OS_ANDROID)
   chrome::NavigateParams params(
@@ -2101,38 +2138,16 @@ void AutofillDialogControllerImpl::HidePopup() {
   input_showing_popup_ = NULL;
 }
 
-void AutofillDialogControllerImpl::LoadRiskFingerprintData() {
-  // TODO(dbeam): Add a CHECK or otherwise strong guarantee that the ToS have
-  // been accepted prior to calling into this method. Also, ensure that the UI
-  // contains a clear indication to the user as to what data will be collected.
-  // Until then, this code should not be called. http://crbug.com/173505
-
-  int64 gaia_id = 0;
-  bool success =
-      base::StringToInt64(wallet_items_->obfuscated_gaia_id(), &gaia_id);
-  DCHECK(success);
-
-  gfx::Rect window_bounds =
-      GetBaseWindowForWebContents(web_contents())->GetBounds();
-
-  PrefService* user_prefs = profile_->GetPrefs();
-  std::string charset = user_prefs->GetString(::prefs::kDefaultCharset);
-  std::string accept_languages =
-      user_prefs->GetString(::prefs::kAcceptLanguages);
-  base::Time install_time = base::Time::FromTimeT(
-      g_browser_process->local_state()->GetInt64(::prefs::kInstallDate));
-
-  risk::GetFingerprint(
-      gaia_id, window_bounds, *web_contents(), chrome::VersionInfo().Version(),
-      charset, accept_languages, install_time, GetDialogType(),
-      g_browser_process->GetApplicationLocale(),
-      base::Bind(&AutofillDialogControllerImpl::OnDidLoadRiskFingerprintData,
-                 weak_ptr_factory_.GetWeakPtr()));
-}
-
 void AutofillDialogControllerImpl::OnDidLoadRiskFingerprintData(
     scoped_ptr<risk::Fingerprint> fingerprint) {
-  NOTIMPLEMENTED();
+  DCHECK(LegalDocumentsAreCurrent());
+
+  std::string proto_data;
+  fingerprint->SerializeToString(&proto_data);
+  bool success = base::Base64Encode(proto_data, &risk_data_);
+  DCHECK(success);
+
+  GetFullWalletIfReady();
 }
 
 bool AutofillDialogControllerImpl::IsManuallyEditingSection(
@@ -2240,6 +2255,11 @@ void AutofillDialogControllerImpl::SetIsSubmitting(bool submitting) {
   }
 }
 
+bool AutofillDialogControllerImpl::LegalDocumentsAreCurrent() const {
+  return accepted_legal_documents_ ||
+      (wallet_items_ && wallet_items_->legal_documents().empty());
+}
+
 void AutofillDialogControllerImpl::SubmitWithWallet() {
   // TODO(dbeam): disallow interacting with the dialog while submitting.
   // http://crbug.com/230932
@@ -2257,6 +2277,9 @@ void AutofillDialogControllerImpl::SubmitWithWallet() {
       wallet_items_->google_transaction_id(),
       source_url_);
 
+  if (LegalDocumentsAreCurrent())
+    LoadRiskFingerprintData();
+
   SuggestionsMenuModel* billing =
       SuggestionsMenuModelForSection(SECTION_CC_BILLING);
   int instrument_index = -1;
@@ -2280,10 +2303,8 @@ void AutofillDialogControllerImpl::SubmitWithWallet() {
     DCHECK(!active_address_id_.empty());
   }
 
-  if (!active_instrument_id_.empty() && !active_address_id_.empty()) {
-    GetFullWallet();
+  if (!active_instrument_id_.empty() && !active_address_id_.empty())

[Ilya Sherman, 2013/05/22 22:34:24]

This block could use a comment now.  Seems like it might be cleaner to decompose
methods for saving the instrument and address if needed, though.

[Dan Beam, 2013/05/22 23:25:20]

Done.

     return;
-  }
 
   scoped_ptr<wallet::Instrument> inputted_instrument =
       CreateTransientInstrument();
@@ -2390,11 +2411,7 @@ scoped_ptr<wallet::Address>AutofillDialogControllerImpl::
 }
 
 void AutofillDialogControllerImpl::GetFullWallet() {
-  DCHECK(is_submitting_);
-  DCHECK(IsPayingWithWallet());
-  DCHECK(wallet_items_);
-  DCHECK(!active_instrument_id_.empty());
-  DCHECK(!active_address_id_.empty());

[Ilya Sherman, 2013/05/22 22:34:24]

Why did you remove these DCHECKs?

[Dan Beam, 2013/05/22 23:25:20]

I made this function GetFullWalletIfReady(), but decided to split later. Added
back.

+  DCHECK(is_submitting_ && IsPayingWithWallet());

[Ilya Sherman, 2013/05/22 22:34:24]

nit: Better to have these as separate DCHECKs, so that a failure tells you which
check failed.

[Dan Beam, 2013/05/22 23:25:20]

Done.

 
   std::vector<wallet::WalletClient::RiskCapability> capabilities;
   capabilities.push_back(wallet::WalletClient::VERIFY_CVC);
@@ -2408,6 +2425,15 @@ void AutofillDialogControllerImpl::GetFullWallet() {
       capabilities));
 }
 
+void AutofillDialogControllerImpl::GetFullWalletIfReady() {
+  DCHECK(is_submitting_ && IsPayingWithWallet());
+
+  if (!active_instrument_id_.empty() && !active_address_id_.empty() &&
+      !risk_data_.empty()) {
+    GetFullWallet();
+  }
+}
+
 void AutofillDialogControllerImpl::HandleSaveOrUpdateRequiredActions(
     const std::vector<wallet::RequiredAction>& required_actions) {
   DCHECK(!required_actions.empty());