Reland "Views: Don't show throbber for very brief resource loads in tabs

post initial load."

This time, land behind a command line flag (the same one that controls
the stop/reload icon toggles). Also, it was noted that this change could
make certain actions feel sluggish, such as pressing "Save Changes" in
monorail, because the throbber feedback was no longer immediate. To
address this, only suppress switches to the throbber that occur right
after the throbber has stopped showing. That way, as long as you press
"Save Changes" a few seconds after the page has finished loading, the
throbber feedback will be immediate, but we'll still get the suppression
of flicker at the end of a page load (visible, for example, on yahoo.com,
cyclingnews.com, or when navigating to an extension/app/etc. in the
Chrome Web Store).

Bug: 734104
Change-Id: I5a59fc16785e3a5f30b40a72f03c5c6a67e66337
Reviewed-on: https://chromium-review.googlesource.com/567800Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485985}

chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc

@@ -313,16 +313,8 @@ void BrowserTabStripController::ShowContextMenuForTab(
 }
 
 void BrowserTabStripController::UpdateLoadingAnimations() {
-  // Don't use the model count here as it's possible for this to be invoked
-  // before we've applied an update from the model (Browser::TabInsertedAt may
-  // be processed before us and invokes this).
-  for (int i = 0, tab_count = tabstrip_->tab_count(); i < tab_count; ++i) {
-    if (model_->ContainsIndex(i)) {
-      Tab* tab = tabstrip_->tab_at(i);
-      WebContents* contents = model_->GetWebContentsAt(i);
-      tab->UpdateLoadingAnimation(TabContentsNetworkState(contents));
-    }
-  }
+  for (int i = 0, tab_count = tabstrip_->tab_count(); i < tab_count; ++i)
+    tabstrip_->tab_at(i)->StepLoadingAnimation();
 }
 
 int BrowserTabStripController::HasAvailableDragActions() const {

chrome/browser/ui/views/tabs/tab.cc

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "build/build_config.h"
 #include "cc/paint/paint_flags.h"
 #include "cc/paint/paint_recorder.h"
@@ -97,6 +98,11 @@ const double kSelectedTabThrobScale = 0.95 - kSelectedTabOpacity;
 
 const char kTabCloseButtonName[] = "TabCloseButton";
 
+bool ShouldShowThrobber(TabRendererData::NetworkState state) {
+  return state != TabRendererData::NETWORK_STATE_NONE &&
+         state != TabRendererData::NETWORK_STATE_ERROR;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Drawing and utility functions
 
@@ -361,11 +367,10 @@ class Tab::ThrobberView : public views::View {
   // Resets the times tracking when the throbber changes state.
   void ResetStartTimes();
 
- private:
   // views::View:
-  bool CanProcessEventsWithinSubtree() const override;
   void OnPaint(gfx::Canvas* canvas) override;
 
+ private:
   Tab* owner_;  // Weak. Owns |this|.
 
   // The point in time when the tab icon was first painted in the waiting state.
@@ -380,7 +385,9 @@ class Tab::ThrobberView : public views::View {
   DISALLOW_COPY_AND_ASSIGN(ThrobberView);
 };
 
-Tab::ThrobberView::ThrobberView(Tab* owner) : owner_(owner) {}
+Tab::ThrobberView::ThrobberView(Tab* owner) : owner_(owner) {
+  set_can_process_events_within_subtree(false);
+}
 
 void Tab::ThrobberView::ResetStartTimes() {
   waiting_start_time_ = base::TimeTicks();
@@ -388,15 +395,9 @@ void Tab::ThrobberView::ResetStartTimes() {
   waiting_state_ = gfx::ThrobberWaitingState();
 }
 
-bool Tab::ThrobberView::CanProcessEventsWithinSubtree() const {
-  return false;
-}
-
 void Tab::ThrobberView::OnPaint(gfx::Canvas* canvas) {
   const TabRendererData::NetworkState state = owner_->data().network_state;
-  if (state == TabRendererData::NETWORK_STATE_NONE ||
-      state == TabRendererData::NETWORK_STATE_ERROR)
-    return;
+  CHECK(ShouldShowThrobber(state));
 
   const ui::ThemeProvider* tp = GetThemeProvider();
   const gfx::Rect bounds = GetLocalBounds();
@@ -534,8 +535,8 @@ void Tab::SetData(const TabRendererData& data) {
     return;
 
   TabRendererData old(data_);
-  UpdateLoadingAnimation(data.network_state);
   data_ = data;
+  UpdateThrobber(old);
 
   base::string16 title = data_.title;
   if (title.empty()) {
@@ -569,17 +570,11 @@ void Tab::SetData(const TabRendererData& data) {
   SchedulePaint();
 }
 
-void Tab::UpdateLoadingAnimation(TabRendererData::NetworkState state) {
-  if (state == data_.network_state &&
-      (state == TabRendererData::NETWORK_STATE_NONE ||
-       state == TabRendererData::NETWORK_STATE_ERROR)) {
-    // If the network state is none or is a network error and hasn't changed,
-    // do nothing. Otherwise we need to advance the animation frame.
+void Tab::StepLoadingAnimation() {
+  if (!throbber_->visible())
     return;
-  }
 
-  data_.network_state = state;
-  AdvanceLoadingAnimation();
+  RefreshThrobber();
 }
 
 void Tab::StartPulse() {
@@ -1273,10 +1268,8 @@ void Tab::PaintIcon(gfx::Canvas* canvas) {
     return;
 
   // Throbber will do its own painting.
-  if (data().network_state != TabRendererData::NETWORK_STATE_NONE &&
-      data().network_state != TabRendererData::NETWORK_STATE_ERROR) {
+  if (throbber_->visible())
     return;
-  }
   // Ensure that |favicon_| is created.
   if (favicon_.isNull()) {
     ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
@@ -1306,16 +1299,47 @@ void Tab::PaintIcon(gfx::Canvas* canvas) {
   }
 }
 
-void Tab::AdvanceLoadingAnimation() {
-  const TabRendererData::NetworkState state = data().network_state;
-  if (state == TabRendererData::NETWORK_STATE_NONE ||
-      state == TabRendererData::NETWORK_STATE_ERROR) {
+void Tab::UpdateThrobber(const TabRendererData& old) {
+  const bool should_show = ShouldShowThrobber(data_.network_state);
+  const bool is_showing = throbber_->visible();
+
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDelayReloadStopButtonChange)) {
+    // Minimize flip-flops between showing the throbber and the favicon. Delay
+    // the switch from favicon to throbber if the switch would occur just after
+    // the a throbbing session finishes. The heuristic for "throbbing session
+    // finishing" is that the old and new URLs match and that the throbber has
+    // been shown recently. See crbug.com/734104
+    constexpr auto kSuppressChangeDuration = base::TimeDelta::FromSeconds(3);
+    if (!is_showing && should_show && old.url == data_.url &&
+        (base::TimeTicks::Now() - last_throbber_show_time_) <
+            kSuppressChangeDuration) {
+      if (!delayed_throbber_show_timer_.IsRunning()) {
+        delayed_throbber_show_timer_.Start(FROM_HERE, kSuppressChangeDuration,
+                                           this, &Tab::RefreshThrobber);
+      }
+      return;
+    }
+
+    delayed_throbber_show_timer_.Stop();
+  }
+
+  if (!is_showing && !should_show)
+    return;
+
+  RefreshThrobber();
+}
+
+void Tab::RefreshThrobber() {
+  if (!ShouldShowThrobber(data().network_state)) {
     throbber_->ResetStartTimes();
     throbber_->SetVisible(false);
     ScheduleIconPaint();
     return;
   }
 
+  last_throbber_show_time_ = base::TimeTicks::Now();
+
   // Since the throbber can animate for a long time, paint to a separate layer
   // when possible to reduce repaint overhead.
   const bool paint_to_layer = controller_->CanPaintThrobberToLayer();

chrome/browser/ui/views/tabs/tab.h

@@ -12,6 +12,8 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "cc/paint/paint_record.h"
 #include "chrome/browser/ui/views/tabs/tab_renderer_data.h"
 #include "ui/base/layout.h"
@@ -94,8 +96,8 @@ class Tab : public gfx::AnimationDelegate,
   void SetData(const TabRendererData& data);
   const TabRendererData& data() const { return data_; }
 
-  // Sets the network state.
-  void UpdateLoadingAnimation(TabRendererData::NetworkState state);
+  // Redraws the loading animation if one is visible. Otherwise, no-op.
+  void StepLoadingAnimation();
 
   // Starts/Stops a pulse animation.
   void StartPulse();
@@ -259,8 +261,11 @@ class Tab : public gfx::AnimationDelegate,
   // Paints the favicon, mirrored for RTL if needed.
   void PaintIcon(gfx::Canvas* canvas);
 
-  // Invoked if data_.network_state changes, or the network_state is not none.
-  void AdvanceLoadingAnimation();
+  // Updates the throbber.
+  void UpdateThrobber(const TabRendererData& old);
+
+  // Sets the throbber visibility according to the state in |data_|.
+  void RefreshThrobber();
 
   // Returns the number of favicon-size elements that can fit in the tab's
   // current size.
@@ -363,6 +368,15 @@ class Tab : public gfx::AnimationDelegate,
   // and thus may be null.
   gfx::ImageSkia favicon_;
 
+  // This timer allows us to delay updating the visibility of the loading
+  // indicator so that state changes of a very brief duration aren't visually
+  // apparent to the user.
+  base::OneShotTimer delayed_throbber_show_timer_;
+
+  // The last time the throbber was visible to the user. See notes in
+  // UpdateThrobber().
+  base::TimeTicks last_throbber_show_time_;
+
   class BackgroundCache {
    public:
     BackgroundCache();

chrome/browser/ui/views/tabs/tab_unittest.cc

@@ -6,12 +6,14 @@
 
 #include <stddef.h>
 
+#include "base/command_line.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/tabs/tab_utils.h"
 #include "chrome/browser/ui/views/tabs/alert_indicator_button.h"
 #include "chrome/browser/ui/views/tabs/tab_controller.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/grit/theme_resources.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/models/list_selection_model.h"
@@ -233,6 +235,17 @@ class TabTest : public views::ViewsTestBase {
     }
   }
 
+  void FastForwardThrobberStateTimer(Tab* tab) {
+    ASSERT_TRUE(tab->delayed_throbber_show_timer_.IsRunning());
+    auto closure = tab->delayed_throbber_show_timer_.user_task();
+    tab->delayed_throbber_show_timer_.Stop();
+    closure.Run();
+  }
+
+  void WindBackLastThrobberShowTime(Tab* tab) {
+    tab->last_throbber_show_time_ = base::TimeTicks();
+  }
+
  protected:
   void InitWidget(Widget* widget) {
     Widget::InitParams params(CreateParams(Widget::InitParams::TYPE_WINDOW));
@@ -442,47 +455,153 @@ TEST_F(TabTest, LayeredThrobber) {
   tab.SetBoundsRect(gfx::Rect(Tab::GetStandardSize()));
 
   views::View* throbber = GetThrobberView(tab);
+  TabRendererData data;
+  data.url = GURL("http://example.com");
   EXPECT_FALSE(throbber->visible());
   EXPECT_EQ(TabRendererData::NETWORK_STATE_NONE, tab.data().network_state);
   EXPECT_EQ(throbber->bounds(), GetFaviconBounds(tab));
 
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_NONE);
+  // Simulate a "normal" tab load: should paint to a layer.
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  EXPECT_TRUE(tab_controller.CanPaintThrobberToLayer());
+  EXPECT_TRUE(throbber->visible());
+  EXPECT_TRUE(throbber->layer());
+  data.network_state = TabRendererData::NETWORK_STATE_LOADING;
+  tab.SetData(data);
+  EXPECT_TRUE(throbber->visible());
+  EXPECT_TRUE(throbber->layer());
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
+  EXPECT_FALSE(throbber->visible());
+
+  // After loading is done, simulate another resource starting to load.
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  EXPECT_TRUE(throbber->visible());
+
+  // Reset.
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
+  EXPECT_FALSE(throbber->visible());
+
+  // Simulate a drag started and stopped during a load: layer painting stops
+  // temporarily.
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  EXPECT_TRUE(throbber->visible());
+  EXPECT_TRUE(throbber->layer());
+  tab_controller.set_paint_throbber_to_layer(false);
+  tab.StepLoadingAnimation();
+  EXPECT_TRUE(throbber->visible());
+  EXPECT_FALSE(throbber->layer());
+  tab_controller.set_paint_throbber_to_layer(true);
+  tab.StepLoadingAnimation();
+  EXPECT_TRUE(throbber->visible());
+  EXPECT_TRUE(throbber->layer());
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
+  EXPECT_FALSE(throbber->visible());
+
+  // Simulate a tab load starting and stopping during tab dragging (or with
+  // stacked tabs): no layer painting.
+  tab_controller.set_paint_throbber_to_layer(false);
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  EXPECT_TRUE(throbber->visible());
+  EXPECT_FALSE(throbber->layer());
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
   EXPECT_FALSE(throbber->visible());
+}
+
+// As above, but tests what happens when we're heuristically delaying the switch
+// between favicon and throbber.
+TEST_F(TabTest, LayeredThrobber2) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDelayReloadStopButtonChange);
+
+  Widget widget;
+  InitWidget(&widget);
+
+  FakeTabController tab_controller;
+  Tab tab(&tab_controller, nullptr);
+  widget.GetContentsView()->AddChildView(&tab);
+  tab.SetBoundsRect(gfx::Rect(Tab::GetStandardSize()));
+
+  views::View* throbber = GetThrobberView(tab);
+  TabRendererData data;
+  data.url = GURL("http://example.com");
+  EXPECT_FALSE(throbber->visible());
+  EXPECT_EQ(TabRendererData::NETWORK_STATE_NONE, tab.data().network_state);
+  EXPECT_EQ(throbber->bounds(), GetFaviconBounds(tab));
 
   // Simulate a "normal" tab load: should paint to a layer.
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_WAITING);
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
   EXPECT_TRUE(tab_controller.CanPaintThrobberToLayer());
   EXPECT_TRUE(throbber->visible());
   EXPECT_TRUE(throbber->layer());
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_LOADING);
+  data.network_state = TabRendererData::NETWORK_STATE_LOADING;
+  tab.SetData(data);
   EXPECT_TRUE(throbber->visible());
   EXPECT_TRUE(throbber->layer());
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_NONE);
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
+  EXPECT_FALSE(throbber->visible());
+
+  // After loading is done, simulate another resource starting to load. The
+  // throbber shouldn't immediately become visible again.
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  EXPECT_FALSE(throbber->visible());
+  ASSERT_NO_FATAL_FAILURE(FastForwardThrobberStateTimer(&tab));
+  EXPECT_TRUE(throbber->visible());
+
+  // On the other hand, if a resource starts to load after the throbber has been
+  // absent for a while, jump straight to showing it.
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
+  EXPECT_FALSE(throbber->visible());
+  WindBackLastThrobberShowTime(&tab);
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  EXPECT_TRUE(throbber->visible());
+
+  // Reset.
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
   EXPECT_FALSE(throbber->visible());
 
   // Simulate a drag started and stopped during a load: layer painting stops
   // temporarily.
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_WAITING);
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  ASSERT_NO_FATAL_FAILURE(FastForwardThrobberStateTimer(&tab));
   EXPECT_TRUE(throbber->visible());
   EXPECT_TRUE(throbber->layer());
   tab_controller.set_paint_throbber_to_layer(false);
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_WAITING);
+  tab.StepLoadingAnimation();
   EXPECT_TRUE(throbber->visible());
   EXPECT_FALSE(throbber->layer());
   tab_controller.set_paint_throbber_to_layer(true);
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_WAITING);
+  tab.StepLoadingAnimation();
   EXPECT_TRUE(throbber->visible());
   EXPECT_TRUE(throbber->layer());
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_NONE);
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
   EXPECT_FALSE(throbber->visible());
 
   // Simulate a tab load starting and stopping during tab dragging (or with
   // stacked tabs): no layer painting.
   tab_controller.set_paint_throbber_to_layer(false);
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_WAITING);
+  data.network_state = TabRendererData::NETWORK_STATE_WAITING;
+  tab.SetData(data);
+  ASSERT_NO_FATAL_FAILURE(FastForwardThrobberStateTimer(&tab));
   EXPECT_TRUE(throbber->visible());
   EXPECT_FALSE(throbber->layer());
-  tab.UpdateLoadingAnimation(TabRendererData::NETWORK_STATE_NONE);
+  data.network_state = TabRendererData::NETWORK_STATE_NONE;
+  tab.SetData(data);
   EXPECT_FALSE(throbber->visible());
 }