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 c3f3d7682eb072d861b1e10c5efe6e5518db382a..147f942918bf628536e6861c2ac91c91a3b0ff05 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"
@@ -1418,13 +1419,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.
+  return risk_data_.empty() ? "risky business" : risk_data_;

[Ilya Sherman, 2013/05/03 07:38:31]

Why can risk_data_ be empty?  We shouldn't issue requests that require risk data
until it's loaded.

[Dan Beam, 2013/05/03 09:24:47]

Failed encoding, not loaded yet (there's nothing to say that risk loads first).

[Ilya Sherman, 2013/05/07 00:59:30]

We should structure the code so that it's guaranteed that risk data is loaded by
the time anyone needs to call GetRiskData().  Likewise, we should structure the
code so that it's not possible for the encoding to fail.

If either of the above goes wrong, then we don't have risk data to offer the
server, which essentially means we're a misbehaving client.  It seems pretty
appropriate for the server to reject requests from misbehaving clients.

 }
 
 void AutofillDialogControllerImpl::OnDidAcceptLegalDocuments() {
-  // TODO(dbeam): Don't send risk params until legal documents are accepted:
-  // http://crbug.com/173505
+  legal_docs_current_ = true;
+  LoadRiskFingerprintData();
 }
 
 void AutofillDialogControllerImpl::OnDidAuthenticateInstrument(bool success) {
@@ -1503,6 +1504,11 @@ void AutofillDialogControllerImpl::OnDidGetWalletItems(
 
   // TODO(dbeam): verify items support kCartCurrency? http://crbug.com/232952
   wallet_items_ = wallet_items.Pass();
+
+  legal_docs_current_ = wallet_items_->legal_documents().empty();
+  if (legal_docs_current_)
+    LoadRiskFingerprintData();
+
   OnWalletOrSigninUpdate();
 }
 
@@ -1702,6 +1708,38 @@ bool AutofillDialogControllerImpl::IsFirstRun() const {
   return is_first_run_;
 }
 
+void AutofillDialogControllerImpl::LoadRiskFingerprintData() {
+  if (!legal_docs_current_)
+    return;

[Ilya Sherman, 2013/05/03 07:38:31]

Can this be a DCHECK instead?

[Dan Beam, 2013/05/03 09:24:47]

A DCHECK() is not a strong assurance in release builds or when half the devs
don't build with them on ;).

[Ilya Sherman, 2013/05/07 00:59:30]

DCHECKs are used to document expected state.  Unless there are expected code
paths that could reach here with |legal_docs_current_| being false, please make
this a DCHECK.

I don't really understand why Evan builds with DCHECKs off, but that doesn't
mean that our code should be different from how most other Chrome code is
structured.

[Dan Beam, 2013/05/07 04:02:50]

Done.

+
+  uint64 obfuscated_gaia_id = 0;
+  bool success = base::StringToUint64(wallet_items_->obfuscated_gaia_id(),
+                                      &obfuscated_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(
+      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::SerializeFingerprint(
+    risk::Fingerprint* fingerprint, std::string* data) {
+  fingerprint->SerializeToString(data);
+}
+
 void AutofillDialogControllerImpl::DisableWallet() {
   signin_helper_.reset();
   account_chooser_model_.SetHadWalletError();
@@ -2023,38 +2061,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, dialog_type_,
-      g_browser_process->GetApplicationLocale(),
-      base::Bind(&AutofillDialogControllerImpl::OnDidLoadRiskFingerprintData,
-                 weak_ptr_factory_.GetWeakPtr()));
-}
-
 void AutofillDialogControllerImpl::OnDidLoadRiskFingerprintData(
     scoped_ptr<risk::Fingerprint> fingerprint) {
-  NOTIMPLEMENTED();
+  if (!legal_docs_current_)
+    return;
+
+  std::string proto_data;
+  SerializeFingerprint(fingerprint.get(), &proto_data);
+
+  if (!base::Base64Encode(proto_data, &risk_data_))

[Dan Beam, 2013/05/03 07:29:09]

btw, I have no idea how this could fail, but it doesn't hurt... I guess....

[Ilya Sherman, 2013/05/03 07:38:31]

Yeah, I'm also uncertain.  I think a DCHECK might be reasonable.

[Dan Beam, 2013/05/03 09:24:47]

Maybe for this... I just don't understand why this returns a bool...

+    risk_data_.clear();
 }
 
 bool AutofillDialogControllerImpl::IsManuallyEditingSection(