Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(470)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 12221040: Interactive autofill: Handle Online Wallet being unavailable. (Closed)

Created:
7 years, 10 months ago by Dan Beam
Modified:
7 years, 10 months ago
CC:
chromium-reviews, Raman Kakilate, tfarina, benquan, dhollowa+watch_chromium.org, ahutter, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman
Visibility:
Public.

Description

Interactive autofill: Handle Online Wallet being unavailable. Adds to WalletClient: - bool HasRequestInProgress(): whether a request is in flight. Adds to AutofillDialogController: - bool CanPayWithWallet(): whether not we've had a wallet error. - string16 AccountChooserText(): the text that the account chooser displays. - bool AccountChooserEnabled(): whether the account chooser should be underlined/clickable. Adds to AutofillDialogControllerImpl: - an implementation of newly added AutofillDialogController methods. - a way to safely refresh wallet items from the server. Adds to AutofillDialogViews: - NotificationArea arrow points at a views::View* above it - UpdateAccountChooser() accounts for wallet being unavailable. - padding between |notification_area_| and |account_chooser_link_|. BUG=163504, 168669 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182020

Patch Set 1 #

Patch Set 2 : log message, fake translation #

Patch Set 3 : . #

Patch Set 4 : todo #

Patch Set 5 : x #

Total comments: 31

Patch Set 6 : simpler #

Patch Set 7 : . #

Patch Set 8 : . #

Total comments: 6

Patch Set 9 : w00t #

Patch Set 10 : includes #

Patch Set 11 : TODO(again) #

Patch Set 12 : . #

Total comments: 5

Patch Set 13 : . #

Patch Set 14 : . #

Patch Set 15 : . #

Total comments: 9

Patch Set 16 : isherman@ review #

Total comments: 6

Patch Set 17 : . #

Patch Set 18 : . #

Patch Set 19 : rebase #

Patch Set 20 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+186 lines, -51 lines) Patch
M chrome/app/generated_resources.grd View 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/autofill/wallet/wallet_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/autofill/wallet/wallet_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +5 lines, -1 line 0 comments Download
M chrome/browser/autofill/wallet/wallet_client_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +18 lines, -0 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_dialog_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +8 lines, -1 line 0 comments Download
M chrome/browser/ui/autofill/autofill_dialog_controller_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +19 lines, -1 line 0 comments Download
M chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 9 chunks +83 lines, -25 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_dialog_views.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +12 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_dialog_views.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 8 chunks +31 lines, -20 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
Dan Beam
estade@: review and I can get sky@ to do a chrome/browser/ui/views/OWNERS stamp? sg?
7 years, 10 months ago (2013-02-06 06:12:29 UTC) #1
Evan Stade
https://codereview.chromium.org/12221040/diff/3004/chrome/browser/ui/autofill/autofill_dialog_controller.h File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): https://codereview.chromium.org/12221040/diff/3004/chrome/browser/ui/autofill/autofill_dialog_controller.h#newcode48 chrome/browser/ui/autofill/autofill_dialog_controller.h:48: virtual string16 SignedInUser() const = 0; notice how these ...
7 years, 10 months ago (2013-02-06 21:57:48 UTC) #2
Dan Beam
so tell me if you like what I've done re: removing some strings from AutofillDialogController ...
7 years, 10 months ago (2013-02-07 01:22:40 UTC) #3
Dan Beam
(I'm attempting to keep all logic in the controller)
7 years, 10 months ago (2013-02-07 01:23:21 UTC) #4
Dan Beam
FYI: screenshots - http://imgur.com/a/hzcFg
7 years, 10 months ago (2013-02-07 01:32:50 UTC) #5
Evan Stade
https://codereview.chromium.org/12221040/diff/3004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/12221040/diff/3004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode574 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:574: if (had_wallet_error_) { On 2013/02/07 01:22:40, Dan Beam wrote: ...
7 years, 10 months ago (2013-02-07 01:54:41 UTC) #6
Dan Beam
https://codereview.chromium.org/12221040/diff/3004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/12221040/diff/3004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode574 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:574: if (had_wallet_error_) { On 2013/02/07 01:54:42, Evan Stade wrote: ...
7 years, 10 months ago (2013-02-07 03:26:53 UTC) #7
Dan Beam
estade@: reverted the code you wanted me to
7 years, 10 months ago (2013-02-07 03:52:38 UTC) #8
Evan Stade
https://codereview.chromium.org/12221040/diff/2007/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/12221040/diff/2007/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode182 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:182: // TODO(dbeam): |arrow_centering_anchor_| is too big (not just text ...
7 years, 10 months ago (2013-02-07 06:13:21 UTC) #9
Dan Beam
https://codereview.chromium.org/12221040/diff/2007/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/12221040/diff/2007/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode182 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:182: // TODO(dbeam): |arrow_centering_anchor_| is too big (not just text ...
7 years, 10 months ago (2013-02-07 18:22:36 UTC) #10
Evan Stade
lgtm, over to sky
7 years, 10 months ago (2013-02-07 18:47:24 UTC) #11
Dan Beam
+sky@ for chrome/browser/ui/views +isherman@ for chrome/browser/autofill
7 years, 10 months ago (2013-02-07 19:03:29 UTC) #12
Ilya Sherman
LGTM with nits: https://codereview.chromium.org/12221040/diff/5004/chrome/browser/autofill/wallet/wallet_client.cc File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/12221040/diff/5004/chrome/browser/autofill/wallet/wallet_client.cc#newcode255 chrome/browser/autofill/wallet/wallet_client.cc:255: DCHECK(!IsRequestInProgress()) << "Tried to fetch two ...
7 years, 10 months ago (2013-02-11 06:07:01 UTC) #13
sky
https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode129 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:129: arrow_centering_anchor_(arrow_centering_anchor) { Can you instead get what you need ...
7 years, 10 months ago (2013-02-11 21:07:33 UTC) #14
Dan Beam
https://codereview.chromium.org/12221040/diff/5004/chrome/browser/autofill/wallet/wallet_client.cc File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/12221040/diff/5004/chrome/browser/autofill/wallet/wallet_client.cc#newcode255 chrome/browser/autofill/wallet/wallet_client.cc:255: DCHECK(!IsRequestInProgress()) << "Tried to fetch two things at once!"; ...
7 years, 10 months ago (2013-02-11 21:53:54 UTC) #15
Ilya Sherman
https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode801 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:801: return; Make this a DCHECK then?
7 years, 10 months ago (2013-02-11 22:04:40 UTC) #16
sky
https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode129 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:129: arrow_centering_anchor_(arrow_centering_anchor) { On 2013/02/11 21:53:54, Dan Beam wrote: > ...
7 years, 10 months ago (2013-02-11 22:05:55 UTC) #17
Dan Beam
https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode801 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:801: return; On 2013/02/11 22:04:41, Ilya Sherman wrote: > Make ...
7 years, 10 months ago (2013-02-11 22:28:46 UTC) #18
Ilya Sherman
lg, thanks
7 years, 10 months ago (2013-02-11 22:36:18 UTC) #19
Dan Beam
https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/12221040/diff/14004/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode129 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:129: arrow_centering_anchor_(arrow_centering_anchor) { On 2013/02/11 22:05:55, sky wrote: > On ...
7 years, 10 months ago (2013-02-11 23:11:42 UTC) #20
sky
LGTM
7 years, 10 months ago (2013-02-12 00:56:31 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/12221040/5018
7 years, 10 months ago (2013-02-12 01:09:53 UTC) #22
commit-bot: I haz the power
7 years, 10 months ago (2013-02-12 01:09:56 UTC) #23 
Failed to apply patch for
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
  Hunk #2 succeeded at 223 (offset -2 lines).
  Hunk #3 succeeded at 261 (offset -2 lines).
  Hunk #4 succeeded at 288 (offset -2 lines).
  Hunk #5 succeeded at 579 (offset -2 lines).
  Hunk #6 succeeded at 668 (offset -2 lines).
  Hunk #7 FAILED at 706.
  Hunk #8 succeeded at 792 (offset 3 lines).
  1 out of 8 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc.rej

Patch:       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
0f687368e28d1f41b877715f4ed5d7108d737e58..62035e2d3240d4455198e589fbcfb5ab9363edeb
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -134,6 +134,8 @@ AutofillDialogControllerImpl::AutofillDialogControllerImpl(
       ssl_status_(ssl_status),
       callback_(callback),
       wallet_client_(profile_->GetRequestContext()),
+      refresh_wallet_items_queued_(false),
+      had_wallet_error_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(suggested_email_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(suggested_cc_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(suggested_billing_(this)),
@@ -223,7 +225,8 @@ void AutofillDialogControllerImpl::Show() {
   view_->Show();
 
   // Request sugar info after the view is showing to simplify code for now.
-  wallet_client_.GetWalletItems(this);
+  if (CanPayWithWallet())
+    ScheduleRefreshWalletItems();
 }
 
 void AutofillDialogControllerImpl::Hide() {
@@ -260,14 +263,18 @@ string16 AutofillDialogControllerImpl::ConfirmButtonText()
const {
   return l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_SUBMIT_BUTTON);
 }
 
-string16 AutofillDialogControllerImpl::SignInText() const {
+string16 AutofillDialogControllerImpl::CancelSignInText() const {
   // TODO(abodenha): real strings and l10n.
-  return string16(ASCIIToUTF16("Sign in to use Google Wallet"));
+  return ASCIIToUTF16("Don't sign in.");
 }
 
-string16 AutofillDialogControllerImpl::CancelSignInText() const {
-  // TODO(abodenha): real strings and l10n.
-  return string16(ASCIIToUTF16("Don't sign in."));
+string16 AutofillDialogControllerImpl::SaveLocallyText() const {
+  return l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_SAVE_LOCALLY_CHECKBOX);
+}
+
+string16 AutofillDialogControllerImpl::ProgressBarText() const {
+  return l10n_util::GetStringUTF16(
+      IDS_AUTOFILL_DIALOG_AUTOCHECKOUT_PROGRESS_BAR);
 }
 
 DialogSignedInState AutofillDialogControllerImpl::SignedInState() const {
@@ -283,13 +290,26 @@ DialogSignedInState
AutofillDialogControllerImpl::SignedInState() const {
   return SIGNED_IN;
 }
 
-string16 AutofillDialogControllerImpl::SaveLocallyText() const {
-  return l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_SAVE_LOCALLY_CHECKBOX);
+bool AutofillDialogControllerImpl::CanPayWithWallet() const {
+  return !had_wallet_error_;
 }
 
-string16 AutofillDialogControllerImpl::ProgressBarText() const {
-  return l10n_util::GetStringUTF16(
-      IDS_AUTOFILL_DIALOG_AUTOCHECKOUT_PROGRESS_BAR);
+string16 AutofillDialogControllerImpl::AccountChooserText() const {
+  if (!CanPayWithWallet())
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PAY_WITHOUT_WALLET);
+
+  // TODO(dbeam): real strings and l10n.
+  DialogSignedInState state = SignedInState();
+  return state != SIGNED_IN ? ASCIIToUTF16("Sign in to use Google Wallet") :
+                              ASCIIToUTF16("user@example.com");
+}
+
+bool AutofillDialogControllerImpl::AccountChooserEnabled() const {
+  if (!CanPayWithWallet())
+    return false;
+
+  DialogSignedInState state = SignedInState();
+  return state != REQUIRES_RESPONSE && state != SIGNED_IN;
 }
 
 const DetailInputs& AutofillDialogControllerImpl::RequestedFieldsForSection(
@@ -561,14 +581,22 @@ DialogNotification
AutofillDialogControllerImpl::CurrentNotification() const {
             IDS_AUTOFILL_DIALOG_SITE_WARNING,
UTF8ToUTF16(source_url_.host())));
   }
 
+  if (!CanPayWithWallet()) {
+    // TODO(dbeam): pass along the Wallet error or remove from the translation.
+    return DialogNotification(
+        DialogNotification::WALLET_ERROR,
+        l10n_util::GetStringFUTF16(
+            IDS_AUTOFILL_DIALOG_COMPLETE_WITHOUT_WALLET,
+            ASCIIToUTF16("[Wallet-Error].")));
+  }
+
   return DialogNotification();
 }
 
 void AutofillDialogControllerImpl::StartSignInFlow() {
   DCHECK(registrar_.IsEmpty());
 
-  content::Source<content::NavigationController> source(
-      &view_->ShowSignIn());
+  content::Source<content::NavigationController> source(&view_->ShowSignIn());
   registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED, source);
 }
 
@@ -642,10 +670,8 @@ void AutofillDialogControllerImpl::Observe(
       content::Details<content::LoadCommittedDetails>(details).ptr();
   if (wallet::IsSignInContinueUrl(load_details->entry->GetVirtualURL())) {
     EndSignInFlow();
-    // TODO(dbeam): the fetcher can't handle being called multiple times.
-    // Address this soon as we will be re-fetching wallet items after every
-    // required action is resolved.
-    wallet_client_.GetWalletItems(this);
+    if (CanPayWithWallet())
+      ScheduleRefreshWalletItems();
   }
 }
 
@@ -680,48 +706,55 @@ void
AutofillDialogControllerImpl::OnDidEscrowSensitiveInformation(
 void AutofillDialogControllerImpl::OnDidGetFullWallet(
     scoped_ptr<wallet::FullWallet> full_wallet) {
   NOTIMPLEMENTED();
+  WalletRequestCompleted(true);
 }
 
 void AutofillDialogControllerImpl::OnDidGetWalletItems(
     scoped_ptr<wallet::WalletItems> wallet_items) {
+  bool items_changed = !wallet_items_ || *wallet_items != *wallet_items_;
   wallet_items_ = wallet_items.Pass();
-  view_->UpdateAccountChooser();
-  view_->UpdateNotificationArea();
+  WalletRequestCompleted(true);
+
+  if (items_changed) {
+    view_->UpdateAccountChooser();
+    view_->UpdateNotificationArea();
+  }
 }
 
 void AutofillDialogControllerImpl::OnDidSaveAddress(
     const std::string& address_id) {
   NOTIMPLEMENTED() << " address_id=" << address_id;
+  WalletRequestCompleted(true);
 }
 
 void AutofillDialogControllerImpl::OnDidSaveInstrument(
     const std::string& instrument_id) {
   NOTIMPLEMENTED() << " instrument_id=" << instrument_id;
+  WalletRequestCompleted(true);
 }
 
 void AutofillDialogControllerImpl::OnDidSaveInstrumentAndAddress(
     const std::string& instrument_id, const std::string& address_id) {
   NOTIMPLEMENTED() << " instrument_id=" << instrument_id
                    << ", address_id=" << address_id;
+  WalletRequestCompleted(true);
 }
 
 void AutofillDialogControllerImpl::OnDidSendAutocheckoutStatus() {
   NOTIMPLEMENTED();
+  WalletRequestCompleted(true);
 }
 
 void AutofillDialogControllerImpl::OnWalletError() {
-  NOTIMPLEMENTED();
-  wallet_items_.reset();
+  WalletRequestCompleted(false);
 }
 
 void AutofillDialogControllerImpl::OnMalformedResponse() {
-  NOTIMPLEMENTED();
-  wallet_items_.reset();
+  WalletRequestCompleted(false);
 }
 
 void AutofillDialogControllerImpl::OnNetworkError(int response_code) {
-  NOTIMPLEMENTED() << " response_code=" << response_code;
-  wallet_items_.reset();
+  WalletRequestCompleted(false);
 }
 

////////////////////////////////////////////////////////////////////////////////
@@ -763,6 +796,31 @@ bool AutofillDialogControllerImpl::HasRequiredAction(
   return std::find(actions.begin(), actions.end(), action) != actions.end();
 }
 
+void AutofillDialogControllerImpl::ScheduleRefreshWalletItems() {
+  DCHECK(CanPayWithWallet());
+
+  if (wallet_client_.HasRequestInProgress()) {
+    refresh_wallet_items_queued_ = true;
+    return;
+  }
+
+  wallet_client_.GetWalletItems(this);
+  refresh_wallet_items_queued_ = false;
+}
+
+void AutofillDialogControllerImpl::WalletRequestCompleted(bool success) {
+  if (!success) {
+    had_wallet_error_ = true;
+    wallet_items_.reset();
+    view_->UpdateAccountChooser();
+    view_->UpdateNotificationArea();
+    return;
+  }
+
+  if (refresh_wallet_items_queued_)
+    ScheduleRefreshWalletItems();
+}
+
 void AutofillDialogControllerImpl::GenerateSuggestionsModels() {
   PersonalDataManager* manager = GetManager();
   const std::vector<CreditCard*>& cards = manager->credit_cards();

Powered by Google App Engine
This is Rietveld 408576698