chromeos: minor cleanup of TabScrubber

Biggest non-style/c++11 change is to convert from NotificationRegistrar to
BrowserListObserver.

This should not result in any functional change.

BUG=889097,268984
TEST=covered by tests

Change-Id: I8ea1c89f28c66f312fbcfda925591f96852d2173
Reviewed-on: https://chromium-review.googlesource.com/c/1298123Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602421}

chrome/browser/ui/ash/tab_scrubber.cc

@@ -10,26 +10,22 @@
 
 #include "ash/shell.h"
 #include "base/metrics/histogram_macros.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/frame/immersive_mode_controller.h"
 #include "chrome/browser/ui/views/tabs/glow_hover_controller.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/browser/ui/views/tabs/tab_style.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
-#include "ui/aura/window.h"
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/gesture_detection/gesture_configuration.h"
 
 namespace {
 
-const int64_t kActivationDelayMS = 200;
-
 inline float Clamp(float value, float low, float high) {
   return std::min(high, std::max(value, low));
 }
@@ -78,29 +74,16 @@ bool TabScrubber::IsActivationPending() {
   return activate_timer_.IsRunning();
 }
 
-TabScrubber::TabScrubber()
-    : scrubbing_(false),
-      browser_(nullptr),
-      tab_strip_(nullptr),
-      swipe_x_(-1),
-      swipe_y_(-1),
-      swipe_direction_(LEFT),
-      highlighted_tab_(-1),
-      activation_delay_(kActivationDelayMS),
-      use_default_activation_delay_(true),
-      weak_ptr_factory_(this) {
+TabScrubber::TabScrubber() {
   // TODO(mash): Add window server API to observe swipe gestures. Observing
   // gestures on browser windows is not sufficient, as this feature works when
   // the cursor is over the shelf, desktop, etc. https://crbug.com/796366
   ash::Shell::Get()->AddPreTargetHandler(this);
-  registrar_.Add(this, chrome::NOTIFICATION_BROWSER_CLOSED,
-                 content::NotificationService::AllSources());
+  BrowserList::AddObserver(this);
 }
 
 TabScrubber::~TabScrubber() {
-  // Note: The weak_ptr_factory_ should invalidate  its weak pointers before
-  // any other members are destroyed.
-  weak_ptr_factory_.InvalidateWeakPtrs();
+  BrowserList::RemoveObserver(this);
 }
 
 void TabScrubber::OnScrollEvent(ui::ScrollEvent* event) {
@@ -122,8 +105,7 @@ void TabScrubber::OnScrollEvent(ui::ScrollEvent* event) {
     return;
   }
 
-  BrowserView* browser_view = BrowserView::GetBrowserViewForNativeWindow(
-      browser->window()->GetNativeWindow());
+  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
   TabStrip* tab_strip = browser_view->tabstrip();
 
   if (tab_strip->IsAnimating()) {
@@ -138,7 +120,7 @@ void TabScrubber::OnScrollEvent(ui::ScrollEvent* event) {
   // left, positive means right.
   float x_offset = event->x_offset();
   if (!scrubbing_) {
-    BeginScrub(browser, browser_view, x_offset);
+    BeginScrub(browser_view, x_offset);
   } else if (highlighted_tab_ == -1) {
     // Has the direction of the swipe changed while scrubbing?
     Direction direction = (x_offset < 0) ? LEFT : RIGHT;
@@ -174,18 +156,17 @@ void TabScrubber::OnScrollEvent(ui::ScrollEvent* event) {
   }
 }
 
-void TabScrubber::Observe(int type,
-                          const content::NotificationSource& source,
-                          const content::NotificationDetails& details) {
-  if (content::Source<Browser>(source).ptr() == browser_) {
-    activate_timer_.Stop();
-    swipe_x_ = -1;
-    swipe_y_ = -1;
-    scrubbing_ = false;
-    highlighted_tab_ = -1;
-    browser_ = nullptr;
-    tab_strip_ = nullptr;
-  }
+void TabScrubber::OnBrowserRemoved(Browser* browser) {
+  if (browser != browser_)
+    return;
+
+  activate_timer_.Stop();
+  swipe_x_ = -1;
+  swipe_y_ = -1;
+  scrubbing_ = false;
+  highlighted_tab_ = -1;
+  browser_ = nullptr;
+  tab_strip_ = nullptr;
 }
 
 void TabScrubber::OnTabAdded(int index) {
@@ -229,15 +210,13 @@ Browser* TabScrubber::GetActiveBrowser() {
   return browser;
 }
 
-void TabScrubber::BeginScrub(Browser* browser,
-                             BrowserView* browser_view,
-                             float x_offset) {
-  DCHECK(browser);
+void TabScrubber::BeginScrub(BrowserView* browser_view, float x_offset) {
   DCHECK(browser_view);
+  DCHECK(browser_view->browser());
 
   tab_strip_ = browser_view->tabstrip();
   scrubbing_ = true;
-  browser_ = browser;
+  browser_ = browser_view->browser();
 
   Direction direction = (x_offset < 0) ? LEFT : RIGHT;
   ScrubDirectionChanged(direction);
@@ -256,8 +235,7 @@ void TabScrubber::FinishScrub(bool activate) {
   activate_timer_.Stop();
 
   if (browser_ && browser_->window()) {
-    BrowserView* browser_view = BrowserView::GetBrowserViewForNativeWindow(
-        browser_->window()->GetNativeWindow());
+    BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_);
     TabStrip* tab_strip = browser_view->tabstrip();
     if (activate && highlighted_tab_ != -1) {
       Tab* tab = tab_strip->tab_at(highlighted_tab_);
@@ -279,16 +257,11 @@ void TabScrubber::FinishScrub(bool activate) {
 }
 
 void TabScrubber::ScheduleFinishScrubIfNeeded() {
-  int delay = use_default_activation_delay_
-                  ? ui::GestureConfiguration::GetInstance()
-                        ->tab_scrub_activation_delay_in_ms()
-                  : activation_delay_;
-
-  if (delay >= 0) {
-    activate_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(delay),
-                          base::Bind(&TabScrubber::FinishScrub,
-                                     weak_ptr_factory_.GetWeakPtr(), true));
-  }
+  const base::TimeDelta delay = base::TimeDelta::FromMilliseconds(
+      use_default_activation_delay_ ? 200 : 0);
+  activate_timer_.Start(FROM_HERE, delay,
+                        base::BindRepeating(&TabScrubber::FinishScrub,
+                                            base::Unretained(this), true));
 }
 
 void TabScrubber::ScrubDirectionChanged(Direction direction) {

chrome/browser/ui/ash/tab_scrubber.h

@@ -9,13 +9,13 @@
 
 #include "base/macros.h"
 #include "base/timer/timer.h"
-#include "chrome/browser/ui/views/frame/immersive_mode_controller.h"
+#include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_observer.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "ui/events/event_handler.h"
 
 class Browser;
+class BrowserView;
+class ImmersiveRevealedLock;
 class Tab;
 class TabStrip;
 
@@ -25,7 +25,7 @@ class Point;
 
 // Class to enable quick tab switching via horizontal 3 finger swipes.
 class TabScrubber : public ui::EventHandler,
-                    public content::NotificationObserver,
+                    public BrowserListObserver,
                     public TabStripObserver {
  public:
   enum Direction { LEFT, RIGHT };
@@ -39,25 +39,20 @@ class TabScrubber : public ui::EventHandler,
                                   int index,
                                   TabScrubber::Direction direction);
 
-  void set_activation_delay(int activation_delay) {
-    activation_delay_ = activation_delay;
-    use_default_activation_delay_ = false;
-  }
-  int activation_delay() const { return activation_delay_; }
   int highlighted_tab() const { return highlighted_tab_; }
   bool IsActivationPending();
 
  private:
+  friend class TabScrubberTest;
+
   TabScrubber();
   ~TabScrubber() override;
 
   // ui::EventHandler overrides:
   void OnScrollEvent(ui::ScrollEvent* event) override;
 
-  // content::NotificationObserver overrides:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+  // BrowserListObserver overrides:
+  void OnBrowserRemoved(Browser* browser) override;
 
   // TabStripObserver overrides.
   void OnTabAdded(int index) override;
@@ -66,7 +61,7 @@ class TabScrubber : public ui::EventHandler,
 
   Browser* GetActiveBrowser();
 
-  void BeginScrub(Browser* browser, BrowserView* browser_view, float x_offset);
+  void BeginScrub(BrowserView* browser_view, float x_offset);
   void FinishScrub(bool activate);
 
   void ScheduleFinishScrubIfNeeded();
@@ -81,32 +76,28 @@ class TabScrubber : public ui::EventHandler,
   void UpdateHighlightedTab(Tab* new_tab, int new_index);
 
   // Are we currently scrubbing?.
-  bool scrubbing_;
-  // The last browser we used for scrubbing, NULL if |scrubbing_| is
-  // false and there is no pending work.
-  Browser* browser_;
+  bool scrubbing_ = false;
+  // The last browser we used for scrubbing, NULL if |scrubbing_| is false and
+  // there is no pending work.
+  Browser* browser_ = nullptr;
   // The TabStrip of the active browser we're scrubbing.
-  TabStrip* tab_strip_;
-  // The current accumulated x and y positions of a swipe, in
-  // the coordinates of the TabStrip of |browser_|
-  float swipe_x_;
-  float swipe_y_;
+  TabStrip* tab_strip_ = nullptr;
+  // The current accumulated x and y positions of a swipe, in the coordinates
+  // of the TabStrip of |browser_|.
+  float swipe_x_ = -1;
+  int swipe_y_ = -1;
   // The direction the current swipe is headed.
-  Direction swipe_direction_;
+  Direction swipe_direction_ = LEFT;
   // The index of the tab that is currently highlighted.
-  int highlighted_tab_;
+  int highlighted_tab_ = -1;
   // Timer to control a delayed activation of the |highlighted_tab_|.
   base::RetainingOneShotTimer activate_timer_;
-  // Time to wait in ms before newly selected tab becomes active.
-  int activation_delay_;
-  // Set if activation_delay had been explicitly set.
-  bool use_default_activation_delay_;
+  // True if the default activation delay should be used with |activate_timer_|.
+  // A value of false means the |activate_timer_| gets a zero delay.
+  bool use_default_activation_delay_ = true;
   // Forces the tabs to be revealed if we are in immersive fullscreen.
   std::unique_ptr<ImmersiveRevealedLock> immersive_reveal_lock_;
 
-  content::NotificationRegistrar registrar_;
-  base::WeakPtrFactory<TabScrubber> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(TabScrubber);
 };
 

chrome/browser/ui/ash/tab_scrubber_browsertest.cc

@@ -76,6 +76,8 @@ class ImmersiveRevealEndedWaiter : public ImmersiveModeController::Observer {
   DISALLOW_COPY_AND_ASSIGN(ImmersiveRevealEndedWaiter);
 };
 
+}  // namespace
+
 class TabScrubberTest : public InProcessBrowserTest,
                         public TabStripModelObserver {
  public:
@@ -86,7 +88,7 @@ class TabScrubberTest : public InProcessBrowserTest,
   }
 
   void SetUpOnMainThread() override {
-    TabScrubber::GetInstance()->set_activation_delay(0);
+    TabScrubber::GetInstance()->use_default_activation_delay_ = false;
 
     // Disable external monitor scaling of coordinates.
     ash::Shell* shell = ash::Shell::Get();
@@ -267,8 +269,6 @@ class TabScrubberTest : public InProcessBrowserTest,
   DISALLOW_COPY_AND_ASSIGN(TabScrubberTest);
 };
 
-}  // namespace
-
 // Swipe a single tab in each direction.
 IN_PROC_BROWSER_TEST_F(TabScrubberTest, Single) {
   AddTabs(browser(), 1);

ui/aura/test/aura_test_base.cc

@@ -75,7 +75,6 @@ void AuraTestBase::SetUp() {
   gesture_config->set_semi_long_press_time_in_ms(400);
   gesture_config->set_show_press_delay_in_ms(5);
   gesture_config->set_swipe_enabled(true);
-  gesture_config->set_tab_scrub_activation_delay_in_ms(200);
   gesture_config->set_two_finger_tap_enabled(true);
   gesture_config->set_velocity_tracker_strategy(
       ui::VelocityTracker::Strategy::LSQ2_RESTRICTED);

ui/events/gesture_detection/gesture_configuration.cc

@@ -67,7 +67,6 @@ GestureConfiguration::GestureConfiguration()
       // 2 * max_touch_move_in_pixels_for_click_.
       span_slop_(30),
       swipe_enabled_(false),
-      tab_scrub_activation_delay_in_ms_(200),
       two_finger_tap_enabled_(false),
       velocity_tracker_strategy_(VelocityTracker::Strategy::STRATEGY_DEFAULT) {
 }

ui/events/gesture_detection/gesture_configuration.h

@@ -161,14 +161,6 @@ class GESTURE_DETECTION_EXPORT GestureConfiguration {
   float span_slop() const { return span_slop_; }
   bool swipe_enabled() const { return swipe_enabled_; }
   void set_swipe_enabled(bool val) { swipe_enabled_ = val; }
-
-  // TODO(davemoore): Move into chrome/browser/ui.
-  int tab_scrub_activation_delay_in_ms() const {
-    return tab_scrub_activation_delay_in_ms_;
-  }
-  void set_tab_scrub_activation_delay_in_ms(int val) {
-    tab_scrub_activation_delay_in_ms_ = val;
-  }
   bool two_finger_tap_enabled() const { return two_finger_tap_enabled_; }
   void set_two_finger_tap_enabled(bool val) { two_finger_tap_enabled_ = val; }
   VelocityTracker::Strategy velocity_tracker_strategy() const {
@@ -254,9 +246,6 @@ class GESTURE_DETECTION_EXPORT GestureConfiguration {
   int show_press_delay_in_ms_;
   float span_slop_;
   bool swipe_enabled_;
-
-  // TODO(davemoore): Move into chrome/browser/ui.
-  int tab_scrub_activation_delay_in_ms_;
   bool two_finger_tap_enabled_;
   VelocityTracker::Strategy velocity_tracker_strategy_;