Change the behavior of the [X] Save details to Wallet checkbox notification to:

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 ca6c44d7a58bb640b31025297d5c0a8e68f14af2..458492f655216d51757546d7a31e11e4c712deeb 100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -253,11 +253,17 @@ AutofillDialogControllerImpl::AutofillDialogControllerImpl(
       metric_logger_(metric_logger),
       initial_user_state_(AutofillMetrics::DIALOG_USER_STATE_UNKNOWN),
       dialog_type_(dialog_type),
+      is_first_run_(!profile_->GetPrefs()->HasPrefPath(
+          prefs::kAutofillDialogPayWithoutWallet)),
       is_submitting_(false),
       autocheckout_is_running_(false),
       had_autocheckout_error_(false) {
   // TODO(estade): remove duplicates from |form|?
   DCHECK(!callback_.is_null());
+  if (is_first_run_) {
+    profile_->GetPrefs()->SetBoolean(prefs::kAutofillDialogPayWithoutWallet,
+                                     kPayWithoutWalletDefault);
+  }
 }
 
 AutofillDialogControllerImpl::~AutofillDialogControllerImpl() {
@@ -393,7 +399,7 @@ void AutofillDialogControllerImpl::Show() {
   // Otherwise, see if the user could be signed in passively.
   // TODO(aruslan): UMA metrics for sign-in.
   if (account_chooser_model_.WalletIsSelected())
-    StartFetchingWalletItems();
+    GetWalletItems();

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

this matches the other calls we have (e.g. GetFullWallet())

[aruslan, 2013/04/05 03:45:35]

Ack. Here and in GetWalletItems below the codereview shows weird indentation, I
guess it's just a glitch.

 }
 
 void AutofillDialogControllerImpl::Hide() {
@@ -541,43 +547,37 @@ bool AutofillDialogControllerImpl::IsSubmitPausedOn(
   return full_wallet_ && full_wallet_->HasRequiredAction(required_action);
 }
 
-void AutofillDialogControllerImpl::StartFetchingWalletItems() {
+void AutofillDialogControllerImpl::GetWalletItems() {
   // TODO(dbeam): Add Risk capabilites once the UI supports risk challenges.
   GetWalletClient()->GetWalletItems(
-      source_url_,
-      std::vector<wallet::WalletClient::RiskCapability>());
+      source_url_, std::vector<wallet::WalletClient::RiskCapability>());
 }
 
 void AutofillDialogControllerImpl::OnWalletOrSigninUpdate() {
-  if (wallet_items_.get()) {

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

this needed to work without |wallet_items_|

[aruslan, 2013/04/05 03:45:35]

Yup, it's already like this in another CL.
In REQUIRES_RESPONSE case below please break instead of not reached.

-    DCHECK(!signin_helper_.get());
-    switch (SignedInState()) {
-      case SIGNED_IN:
-        // Start fetching the user name if we don't know it yet.
-        if (current_username_.empty()) {
-          signin_helper_.reset(new wallet::WalletSigninHelper(
-              this,
-              profile_->GetRequestContext()));
-          signin_helper_->StartUserNameFetch();
-        }
-        break;
+  switch (SignedInState()) {
+    case SIGNED_IN:
+      // Start fetching the user name if we don't know it yet.
+      if (current_username_.empty()) {
+        signin_helper_.reset(new wallet::WalletSigninHelper(
+            this, profile_->GetRequestContext()));
+        signin_helper_->StartUserNameFetch();
+      }
+      break;
 
-      case REQUIRES_SIGN_IN:
-        // TODO(aruslan): automatic sign-in?
-        break;
+    case REQUIRES_SIGN_IN:
+      // TODO(aruslan): automatic sign-in?
+      break;
 
-      case REQUIRES_PASSIVE_SIGN_IN:
-        // Attempt to passively sign in the user.
-        current_username_.clear();
-        signin_helper_.reset(new wallet::WalletSigninHelper(
-            this,
-            profile_->GetRequestContext()));
-        signin_helper_->StartPassiveSignin();
-        break;
+    case REQUIRES_PASSIVE_SIGN_IN:
+      // Attempt to passively sign in the user.
+      current_username_.clear();
+      signin_helper_.reset(new wallet::WalletSigninHelper(
+          this, profile_->GetRequestContext()));
+      signin_helper_->StartPassiveSignin();
+      break;
 
-      case REQUIRES_RESPONSE:
-        NOTREACHED();
-    }
+    case REQUIRES_RESPONSE:
+      NOTREACHED();
   }
 
   GenerateSuggestionsModels();
@@ -693,6 +693,17 @@ gfx::Image AutofillDialogControllerImpl::AccountChooserImage() {
   return icon;
 }
 
+bool AutofillDialogControllerImpl::GetPayWithoutWallet() const {
+  return profile_->GetPrefs()->GetBoolean(
+      prefs::kAutofillDialogPayWithoutWallet);
+}
+
+void AutofillDialogControllerImpl::SetPayWithoutWallet(
+    bool pay_without_wallet) {
+  profile_->GetPrefs()->SetBoolean(prefs::kAutofillDialogPayWithoutWallet,
+                                   pay_without_wallet);
+}

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

^ this isn't strictly necessary to do in the controller, but I'm worried that
all the views will need to subscribe to pref updates themselves rather than
saying:

  void AutofillDialogControllerImpl::PrefChanged(const std::string& pref) {
    if (pref == prefs::kAutofillDialogPayWithoutWallet && view_) {
      view_->UpdateNotificationArea();
      view_->UpdateAccountChooser();
    }
  }

(which I can wire up / add if we want)

[aruslan, 2013/04/05 03:45:35]

Currently, the account chooser is wired up to trigger AcoountChoiceChanged. I'm
happy to see this gone.

+
 string16 AutofillDialogControllerImpl::LabelForSection(DialogSection section)
     const {
   switch (section) {
@@ -1089,16 +1100,6 @@ void AutofillDialogControllerImpl::ViewClosed() {
         metric);
   }
 
-  // Reset the view so that updates to the pref aren't processed.
-  view_.reset();
-  // On a successful submit, if the user manually selected "pay without wallet",
-  // stop trying to pay with Wallet on future runs of the dialog.
-  bool manually_selected_pay_without_wallet =
-      !account_chooser_model_.WalletIsSelected() &&
-      !account_chooser_model_.had_wallet_error();
-  profile_->GetPrefs()->SetBoolean(prefs::kAutofillDialogPayWithoutWallet,
-                                   manually_selected_pay_without_wallet);
-
   delete this;
 }
 
@@ -1106,27 +1107,44 @@ std::vector<DialogNotification>
     AutofillDialogControllerImpl::CurrentNotifications() const {
   std::vector<DialogNotification> notifications;
 
-  if (account_chooser_model_.WalletIsSelected()) {
-    if (SignedInState() == SIGNED_IN) {
-      // On first run with a complete wallet profile, show a notification
-      // explaining where this data came from.
-      if (IsFirstRun() && HasCompleteWallet()) {
-        notifications.push_back(DialogNotification(
-            DialogNotification::EXPLANATORY_MESSAGE,
-            l10n_util::GetStringUTF16(
-                IDS_AUTOFILL_DIALOG_DETAILS_FROM_WALLET)));
+  if (account_chooser_model_.had_wallet_error()) {
+    // TODO(dbeam): pass along the Wallet error or remove from the translation.
+    // TODO(dbeam): figure out a way to dismiss this error after a while.
+    notifications.push_back(DialogNotification(
+        DialogNotification::WALLET_ERROR,
+        l10n_util::GetStringFUTF16(IDS_AUTOFILL_DIALOG_COMPLETE_WITHOUT_WALLET,
+                                   ASCIIToUTF16("Oops, [Wallet-Error]."))));
+  } else {
+    if (is_first_run_) {
+      if (SignedInState() == SIGNED_IN) {
+        if (HasCompleteWallet()) {
+          // First run, signed in, has a complete Google Wallet.
+          notifications.push_back(DialogNotification(
+              DialogNotification::EXPLANATORY_MESSAGE,
+              l10n_util::GetStringUTF16(
+                  IDS_AUTOFILL_DIALOG_DETAILS_FROM_WALLET)));
+        } else {
+          // First run, signed in, has an incomplete (or no) Google Wallet.
+          notifications.push_back(DialogNotification(
+              DialogNotification::WALLET_USAGE_CONFIRMATION,
+              l10n_util::GetStringUTF16(
+                  IDS_AUTOFILL_DIALOG_SAVE_DETAILS_IN_WALLET)));
+        }
       } else {
+        // First run, not signed in, wallet promo.
         notifications.push_back(DialogNotification(
-            DialogNotification::WALLET_USAGE_CONFIRMATION,
+            DialogNotification::WALLET_SIGNIN_PROMO,
             l10n_util::GetStringUTF16(
-                IDS_AUTOFILL_DIALOG_SAVE_DETAILS_IN_WALLET)));
+                IDS_AUTOFILL_DIALOG_SIGN_IN_AND_SAVE_DETAILS)));
       }
-    } else if (IsFirstRun()) {
-      // If the user is not signed in, show an upsell notification on first run.
+    } else if (SignedInState() == SIGNED_IN && !HasCompleteWallet()) {
+      // After first run, signed in.
       notifications.push_back(DialogNotification(
-          DialogNotification::WALLET_SIGNIN_PROMO,
+          DialogNotification::WALLET_USAGE_CONFIRMATION,
           l10n_util::GetStringUTF16(
-              IDS_AUTOFILL_DIALOG_SIGN_IN_AND_SAVE_DETAILS)));
+              IDS_AUTOFILL_DIALOG_SAVE_DETAILS_IN_WALLET)));
+    } else {
+      // If the user isn't signed in and it's after the first run, no promo.
     }
   }
 
@@ -1155,16 +1173,6 @@ std::vector<DialogNotification>
         l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_AUTOCHECKOUT_ERROR)));
   }
 
-  if (account_chooser_model_.had_wallet_error()) {
-    // TODO(dbeam): pass along the Wallet error or remove from the translation.
-    // TODO(dbeam): figure out a way to dismiss this error after a while.
-    notifications.push_back(DialogNotification(
-        DialogNotification::WALLET_ERROR,
-        l10n_util::GetStringFUTF16(
-            IDS_AUTOFILL_DIALOG_COMPLETE_WITHOUT_WALLET,
-            ASCIIToUTF16("Oops, [Wallet-Error]."))));
-  }
-
   return notifications;
 }
 
@@ -1305,7 +1313,7 @@ void AutofillDialogControllerImpl::Observe(
   if (wallet::IsSignInContinueUrl(load_details->entry->GetVirtualURL())) {
     EndSignInFlow();
     if (account_chooser_model_.WalletIsSelected())
-      StartFetchingWalletItems();
+      GetWalletItems();
   }
 }
 
@@ -1366,8 +1374,7 @@ void AutofillDialogControllerImpl::OnPassiveSigninSuccess(
     const std::string& username) {
   current_username_ = username;
   signin_helper_.reset();
-  wallet_items_.reset();
-  StartFetchingWalletItems();
+  GetWalletItems();
 }
 
 void AutofillDialogControllerImpl::OnUserNameFetchSuccess(
@@ -1478,14 +1485,8 @@ void AutofillDialogControllerImpl::AccountChoiceChanged() {
   if (!view_)
     return;
 
-  // Whenever the user changes the current account, the Wallet data should be
-  // cleared. If the user has chosen a Wallet account, an attempt to fetch
-  // the Wallet data is made to see if the user is still signed in.
-  // This will trigger a passive sign-in if required.
-  // TODO(aruslan): integrate an automatic sign-in.
-  wallet_items_.reset();
-  if (account_chooser_model_.WalletIsSelected())
-    StartFetchingWalletItems();
+  if (account_chooser_model_.WalletIsSelected() && !wallet_items_)

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

^ we could probably re-fetch if we want...

[aruslan, 2013/04/05 03:45:35]

Without the account chooser CL this doesn't matter (but I would refetch anyway).
In the account chooser CL it initiates refetch if we returned to the one that we
were using, otherwise it initiates the sign-in sequence.

+    GetWalletItems();
 
   GenerateSuggestionsModels();
   view_->ModelChanged();
@@ -1556,11 +1557,6 @@ void AutofillDialogControllerImpl::OnWalletSigninError() {
   GetWalletClient()->CancelPendingRequests();
 }
 
-bool AutofillDialogControllerImpl::IsFirstRun() const {
-  PrefService* prefs = profile_->GetPrefs();
-  return !prefs->HasPrefPath(prefs::kAutofillDialogPayWithoutWallet);
-}
-
 void AutofillDialogControllerImpl::GenerateSuggestionsModels() {
   suggested_email_.Reset();
   suggested_cc_.Reset();