[Upstream Feedback] Add loading indicator to local card migration icon

Add existing functionality to local card migration icon. Previously it
has a label fade-in/-out animation. Disable that and replace with the
new animation when experiment is enabled.

Also move loading indicator to PageActionIconView so that child view does
not need to know.

Bug: 964127
Change-Id: I2024c6d9ed23eea261b733dece833a9072faeced
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1828345
Commit-Queue: Siyu An <siyua@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#701691}

chrome/browser/ui/views/autofill/payments/local_card_migration_browsertest.cc

@@ -39,6 +39,7 @@
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_container_view.h"
+#include "chrome/browser/ui/views/page_action/page_action_icon_loading_indicator_view.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_account_icon_container_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
@@ -535,8 +536,11 @@ class LocalCardMigrationBrowserTestForStatusChip
 
   void SetUp() override {
     base::test::ScopedFeatureList scoped_feature_list;
-    scoped_feature_list.InitAndEnableFeature(
-        features::kAutofillEnableToolbarStatusChip);
+    scoped_feature_list.InitWithFeatures(
+        /*enabled_features=*/{features::kAutofillCreditCardUploadFeedback,
+                              features::kAutofillEnableToolbarStatusChip,
+                              features::kAutofillUpstream},
+        /*disabled_features=*/{});
 
     LocalCardMigrationBrowserTest::SetUp();
   }
@@ -1075,6 +1079,44 @@ IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTestForStatusChip,
   EXPECT_TRUE(GetLocalCardMigrationIconView()->GetVisible());
   EXPECT_FALSE(GetLocalCardMigrationOfferBubbleViews());
 }
+
+IN_PROC_BROWSER_TEST_F(LocalCardMigrationBrowserTestForStatusChip,
+                       Feedback_CardSavingAnimation) {
+  SaveLocalCard(kFirstCardNumber);
+  SaveLocalCard(kSecondCardNumber);
+  UseCardAndWaitForMigrationOffer(kFirstCardNumber);
+  // Click the [Continue] button in the bubble.
+  ClickOnOkButton(GetLocalCardMigrationOfferBubbleViews());
+  test_url_loader_factory()->ClearResponses();
+
+  EXPECT_TRUE(GetLocalCardMigrationIconView()->GetVisible());
+  EXPECT_FALSE(GetLocalCardMigrationIconView()
+                   ->loading_indicator_for_testing()
+                   ->IsAnimating());
+
+  // Click the [Save] button in the dialog.
+  ResetEventWaiterForSequence({DialogEvent::SENT_MIGRATE_CARDS_REQUEST});
+  ClickOnOkButton(GetLocalCardMigrationMainDialogView());
+  WaitForObservedEvent();
+
+  // No dialog should be showing, but icon should display throbber animation.
+  EXPECT_EQ(nullptr, GetLocalCardMigrationMainDialogView());
+  EXPECT_TRUE(GetLocalCardMigrationIconView()->GetVisible());
+  EXPECT_TRUE(GetLocalCardMigrationIconView()
+                  ->loading_indicator_for_testing()
+                  ->IsAnimating());
+
+  SetUpMigrateCardsRpcPaymentsAccepts();
+  ResetEventWaiterForSequence({DialogEvent::RECEIVED_MIGRATE_CARDS_RESPONSE});
+  WaitForObservedEvent();
+
+  // Icon animation should stop. Dialog stays hidden.
+  EXPECT_TRUE(GetLocalCardMigrationIconView()->GetVisible());
+  EXPECT_FALSE(GetLocalCardMigrationIconView()
+                   ->loading_indicator_for_testing()
+                   ->IsAnimating());
+}
+
 #endif  // !defined(OS_CHROMEOS)
 
 // TODO(crbug.com/897998):

chrome/browser/ui/views/autofill/payments/local_card_migration_icon_view.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/views/autofill/payments/local_card_migration_bubble_views.h"
 #include "chrome/browser/ui/views/autofill/payments/local_card_migration_dialog_view.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/autofill/core/common/autofill_payments_features.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/paint_vector_icon.h"
@@ -27,7 +28,12 @@ LocalCardMigrationIconView::LocalCardMigrationIconView(
                          delegate) {
   DCHECK(delegate);
   SetID(VIEW_ID_MIGRATE_LOCAL_CREDIT_CARD_BUTTON);
-  SetUpForInOutAnimation();
+  if (base::FeatureList::IsEnabled(
+          features::kAutofillCreditCardUploadFeedback)) {
+    InstallLoadingIndicator();
+  } else {
+    SetUpForInOutAnimation();
+  }
 }
 
 LocalCardMigrationIconView::~LocalCardMigrationIconView() {}
@@ -78,16 +84,32 @@ bool LocalCardMigrationIconView::Update() {
         // Disable the credit card icon so it does not update if user clicks
         // on it.
         SetEnabled(false);
-        AnimateIn(IDS_AUTOFILL_LOCAL_CARD_MIGRATION_ANIMATION_LABEL);
+        if (base::FeatureList::IsEnabled(
+                features::kAutofillCreditCardUploadFeedback)) {
+          SetIsLoading(/*is_loading=*/true);
+        } else {
+          AnimateIn(IDS_AUTOFILL_LOCAL_CARD_MIGRATION_ANIMATION_LABEL);
+        }
         break;
       }
       case LocalCardMigrationFlowStep::MIGRATION_FINISHED: {
-        UnpauseAnimation();
+        if (base::FeatureList::IsEnabled(
+                features::kAutofillCreditCardUploadFeedback)) {
+          SetIsLoading(/*is_loading=*/false);
+        } else {
+          UnpauseAnimation();
+        }
         SetEnabled(true);
         break;
       }
       case LocalCardMigrationFlowStep::MIGRATION_FAILED: {
-        UnpauseAnimation();
+        if (base::FeatureList::IsEnabled(
+                features::kAutofillCreditCardUploadFeedback)) {
+          UpdateIconImage();
+          SetIsLoading(/*is_loading=*/false);
+        } else {
+          UnpauseAnimation();
+        }
         SetEnabled(true);
         break;
       }

chrome/browser/ui/views/autofill/payments/save_card_bubble_views_browsertest.cc

@@ -33,6 +33,7 @@
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_container_view.h"
+#include "chrome/browser/ui/views/page_action/page_action_icon_loading_indicator_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_account_icon_container_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/test/base/in_process_browser_test.h"

chrome/browser/ui/views/autofill/payments/save_card_icon_view.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/autofill/payments/save_card_bubble_views.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/autofill/core/common/autofill_payments_features.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/paint_vector_icon.h"
@@ -26,17 +27,14 @@ SaveCardIconView::SaveCardIconView(CommandUpdater* command_updater,
   DCHECK(delegate);
   SetID(VIEW_ID_SAVE_CREDIT_CARD_BUTTON);
 
+  if (base::FeatureList::IsEnabled(
+          features::kAutofillCreditCardUploadFeedback)) {
+    InstallLoadingIndicator();
+  }
   SetUpForInOutAnimation();
-
-  loading_indicator_ =
-      AddChildView(std::make_unique<PageActionIconLoadingIndicatorView>());
-  loading_indicator_->SetVisible(false);
-  AddObserver(loading_indicator_);
 }
 
-SaveCardIconView::~SaveCardIconView() {
-  RemoveObserver(loading_indicator_);
-}
+SaveCardIconView::~SaveCardIconView() = default;
 
 views::BubbleDialogDelegateView* SaveCardIconView::GetBubble() const {
   SaveCardBubbleController* controller = GetController();
@@ -62,9 +60,9 @@ bool SaveCardIconView::Update() {
 
   if (command_enabled && controller->ShouldShowSavingCardAnimation()) {
     SetEnabled(false);
-    loading_indicator_->ShowAnimation();
+    SetIsLoading(/*is_loading=*/true);
   } else {
-    loading_indicator_->StopAnimation();
+    SetIsLoading(/*is_loading=*/false);
     UpdateIconImage();
     SetEnabled(true);
   }

chrome/browser/ui/views/autofill/payments/save_card_icon_view.h

@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_UI_VIEWS_AUTOFILL_PAYMENTS_SAVE_CARD_ICON_VIEW_H_
 
 #include "base/macros.h"
-#include "chrome/browser/ui/views/page_action/page_action_icon_loading_indicator_view.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
 
 class CommandUpdater;
@@ -29,10 +28,6 @@ class SaveCardIconView : public PageActionIconView {
   bool Update() override;
   base::string16 GetTextForTooltipAndAccessibleName() const override;
 
-  PageActionIconLoadingIndicatorView* loading_indicator_for_testing() {
-    return loading_indicator_;
-  }
-
  protected:
   // PageActionIconView:
   void OnExecuting(PageActionIconView::ExecuteSource execute_source) override;
@@ -45,10 +40,6 @@ class SaveCardIconView : public PageActionIconView {
   // gfx::AnimationDelegate:
   void AnimationEnded(const gfx::Animation* animation) override;
 
-  // The loading indicator. Its animation will be triggered when upload save is
-  // in progress.
-  PageActionIconLoadingIndicatorView* loading_indicator_;
-
   DISALLOW_COPY_AND_ASSIGN(SaveCardIconView);
 };
 

chrome/browser/ui/views/page_action/page_action_icon_loading_indicator_view.cc

@@ -7,19 +7,24 @@
 #include "base/location.h"
 #include "base/time/default_tick_clock.h"
 #include "chrome/browser/themes/theme_properties.h"
+#include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_throbber.h"
 
-PageActionIconLoadingIndicatorView::PageActionIconLoadingIndicatorView() =
-    default;
+PageActionIconLoadingIndicatorView::PageActionIconLoadingIndicatorView(
+    PageActionIconView* parent)
+    : parent_(parent) {
+  parent_->AddObserver(this);
+}
 
-PageActionIconLoadingIndicatorView::~PageActionIconLoadingIndicatorView() =
-    default;
+PageActionIconLoadingIndicatorView::~PageActionIconLoadingIndicatorView() {
+  parent_->RemoveObserver(this);
+}
 
 void PageActionIconLoadingIndicatorView::ShowAnimation() {
-  if (!throbber_start_time_.has_value())
+  if (!throbber_start_time_)
     throbber_start_time_ = base::TimeTicks::Now();
 
   SetVisible(true);
@@ -37,13 +42,15 @@ bool PageActionIconLoadingIndicatorView::IsAnimating() {
 }
 
 void PageActionIconLoadingIndicatorView::OnPaint(gfx::Canvas* canvas) {
+  if (!throbber_start_time_)
+    return;
+
   const SkColor color = GetThemeProvider()->GetColor(
       ThemeProperties::COLOR_TAB_THROBBER_SPINNING);
   constexpr int kThrobberStrokeWidth = 2;
-  gfx::PaintThrobberSpinning(
-      canvas, GetLocalBounds(), color,
-      base::TimeTicks::Now() - throbber_start_time_.value_or(base::TimeTicks()),
-      kThrobberStrokeWidth);
+  gfx::PaintThrobberSpinning(canvas, GetLocalBounds(), color,
+                             base::TimeTicks::Now() - *throbber_start_time_,
+                             kThrobberStrokeWidth);
 }
 
 void PageActionIconLoadingIndicatorView::OnViewBoundsChanged(

chrome/browser/ui/views/page_action/page_action_icon_loading_indicator_view.h

@@ -13,6 +13,8 @@
 #include "ui/views/view.h"
 #include "ui/views/view_observer.h"
 
+class PageActionIconView;
+
 // The view that contains a throbber animation. It is shown when the action
 // related to the page action icon is in progress.
 // TODO(crbug.com/932818): Investigate the possibility of making this a layer
@@ -21,7 +23,7 @@ class PageActionIconLoadingIndicatorView : public views::View,
                                            public views::ViewObserver,
                                            public gfx::AnimationDelegate {
  public:
-  PageActionIconLoadingIndicatorView();
+  explicit PageActionIconLoadingIndicatorView(PageActionIconView* parent);
   ~PageActionIconLoadingIndicatorView() override;
 
   void ShowAnimation();
@@ -38,10 +40,12 @@ class PageActionIconLoadingIndicatorView : public views::View,
   void AnimationProgressed(const gfx::Animation* animation) override;
 
  private:
-  base::Optional<base::TimeTicks> throbber_start_time_ = base::nullopt;
+  base::Optional<base::TimeTicks> throbber_start_time_;
 
   gfx::ThrobAnimation animation_{this};
 
+  PageActionIconView* const parent_;
+
   DISALLOW_COPY_AND_ASSIGN(PageActionIconLoadingIndicatorView);
 };
 

chrome/browser/ui/views/page_action/page_action_icon_view.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/command_updater.h"
 #include "chrome/browser/ui/omnibox/omnibox_theme.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h"
+#include "chrome/browser/ui/views/page_action/page_action_icon_loading_indicator_view.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -62,7 +63,7 @@ PageActionIconView::PageActionIconView(CommandUpdater* command_updater,
   UpdateBorder();
 }
 
-PageActionIconView::~PageActionIconView() {}
+PageActionIconView::~PageActionIconView() = default;
 
 bool PageActionIconView::IsBubbleShowing() const {
   // If the bubble is being destroyed, it's considered showing though it may be
@@ -211,6 +212,23 @@ void PageActionIconView::UpdateIconImage() {
                                           icon_color, GetVectorIconBadge()));
 }
 
+void PageActionIconView::InstallLoadingIndicator() {
+  if (loading_indicator_)
+    return;
+
+  loading_indicator_ =
+      AddChildView(std::make_unique<PageActionIconLoadingIndicatorView>(this));
+  loading_indicator_->SetVisible(false);
+}
+
+void PageActionIconView::SetIsLoading(bool is_loading) {
+  if (!loading_indicator_)
+    return;
+
+  is_loading ? loading_indicator_->ShowAnimation()
+             : loading_indicator_->StopAnimation();
+}
+
 content::WebContents* PageActionIconView::GetWebContents() const {
   return delegate_->GetWebContentsForPageActionIconView();
 }

chrome/browser/ui/views/page_action/page_action_icon_view.h

@@ -20,6 +20,7 @@
 
 class CommandUpdater;
 class OmniboxView;
+class PageActionIconLoadingIndicatorView;
 
 namespace content {
 class WebContents;
@@ -80,6 +81,10 @@ class PageActionIconView : public IconLabelBubbleView {
 
   void ExecuteForTesting();
 
+  PageActionIconLoadingIndicatorView* loading_indicator_for_testing() {
+    return loading_indicator_;
+  }
+
  protected:
   enum ExecuteSource {
     EXECUTE_SOURCE_MOUSE,
@@ -138,6 +143,15 @@ class PageActionIconView : public IconLabelBubbleView {
   // Updates the icon image after some state has changed.
   virtual void UpdateIconImage();
 
+  // Creates and updates the loading indicator.
+  // TODO(crbug.com/964127): Ideally this should be lazily initialized in
+  // SetIsLoading(), but local card migration icon has a weird behavior that
+  // doing so will cause the indicator being invisible. Investigate and fix.
+  void InstallLoadingIndicator();
+
+  // Set if the page action icon is in the loading state.
+  void SetIsLoading(bool is_loading);
+
   // Returns the associated web contents from the delegate.
   content::WebContents* GetWebContents() const;
 
@@ -169,6 +183,9 @@ class PageActionIconView : public IconLabelBubbleView {
   // the web page.
   bool active_ = false;
 
+  // The loading indicator, showing a throbber animation on top of the icon.
+  PageActionIconLoadingIndicatorView* loading_indicator_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(PageActionIconView);
 };