Register fling scheduler on GSB instead of GFS.

Registering the FlingScheduler after processing a GFS causes a frame of
jank at the beginning of the fling. This is because the first OnAnimationStep
call after observer's registration might be missing and/or incorrectly have
the timestamp of the last begin frame before the observer's registration
rather than the first begin frame after the registration.

To avoid this issue, in this cl we register the observer on GSB instead of GFS
so that the potential missing/incorrect OnAnimationStep call passes by the time
that the GFS is processed. We unregister the observer on GSE instead of when
the fling stops since not all GSBs are followed by GFS events.

Bug: 882907
Test: GestureEventQueueWithCompositorEventQueueTest.FlingSchedulerOpbserverRegisteredOnGSB
Change-Id: I149469fce31c9146c7282bee5cba7152ee65b1ea
Reviewed-on: https://chromium-review.googlesource.com/c/1313270Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604707}

content/browser/renderer_host/input/fling_controller.cc

@@ -197,7 +197,7 @@ void FlingController::ProcessGestureFlingStart(
 }
 
 void FlingController::ScheduleFlingProgress() {
-  scheduler_client_->ScheduleFlingProgress(weak_ptr_factory_.GetWeakPtr());
+  scheduler_client_->ScheduleFlingProgress();
 }
 
 void FlingController::ProcessGestureFlingCancel(
@@ -417,7 +417,7 @@ void FlingController::CancelCurrentFling() {
   }
 
   if (had_active_fling) {
-    scheduler_client_->DidStopFlingingOnBrowser(weak_ptr_factory_.GetWeakPtr());
+    scheduler_client_->DidStopFlingingOnBrowser();
     TRACE_EVENT_ASYNC_END0("input", kFlingTraceName, this);
   }
 }
@@ -449,6 +449,15 @@ bool FlingController::UpdateCurrentFlingState(
   return true;
 }
 
+void FlingController::RegisterFlingSchedulerObserver() {
+  scheduler_client_->RegisterFlingSchedulerObserver(
+      weak_ptr_factory_.GetWeakPtr());
+}
+
+void FlingController::UnregisterFlingSchedulerObserver() {
+  scheduler_client_->UnregisterFlingSchedulerObserver();
+}
+
 bool FlingController::FlingCancellationIsDeferred() const {
   return fling_booster_ && fling_booster_->fling_cancellation_is_deferred();
 }

content/browser/renderer_host/input/fling_controller.h

@@ -39,12 +39,15 @@ class CONTENT_EXPORT FlingControllerSchedulerClient {
  public:
   virtual ~FlingControllerSchedulerClient() {}
 
-  virtual void ScheduleFlingProgress(
-      base::WeakPtr<FlingController> fling_controller) = 0;
+  virtual void ScheduleFlingProgress() = 0;
 
-  virtual void DidStopFlingingOnBrowser(
+  virtual void RegisterFlingSchedulerObserver(
       base::WeakPtr<FlingController> fling_controller) = 0;
 
+  virtual void UnregisterFlingSchedulerObserver() = 0;
+
+  virtual void DidStopFlingingOnBrowser() = 0;
+
   virtual bool NeedsBeginFrameForFlingProgress() = 0;
 };
 
@@ -100,6 +103,19 @@ class CONTENT_EXPORT FlingController {
 
   gfx::Vector2dF CurrentFlingVelocity() const;
 
+  // Registering the FlingScheduler after processing a GFS causes a frame of
+  // jank at the beginning of the fling (see https://crbug.com/882907). This is
+  // because the first OnAnimationStep call after observer's registration might
+  // be missing and/or incorrectly have the timestamp of the last begin frame
+  // before the observer's registration rather than the first begin frame after
+  // the registration (https://crbug.com/848796). To avoid this issue we
+  // register the observer on GSB instead of GFS so that the potential
+  // missing/incorrect OnAnimationStep call passes by the time that the GFS is
+  // processed. We unregister the observer on GSE instead of when the fling
+  // stops since not all GSBs are followed by GFS events.
+  void RegisterFlingSchedulerObserver();
+  void UnregisterFlingSchedulerObserver();
+
   // Returns the |TouchpadTapSuppressionController| instance.
   TouchpadTapSuppressionController* GetTouchpadTapSuppressionController();
 

content/browser/renderer_host/input/fling_controller_unittest.cc

@@ -69,15 +69,16 @@ class FlingControllerTest : public FlingControllerEventSenderClient,
   }
 
   // FlingControllerSchedulerClient
-  void ScheduleFlingProgress(
-      base::WeakPtr<FlingController> fling_controller) override {
+  void ScheduleFlingProgress() override {
     DCHECK(!scheduled_next_fling_progress_);
     scheduled_next_fling_progress_ = true;
   }
-  void DidStopFlingingOnBrowser(
-      base::WeakPtr<FlingController> fling_controller) override {
+  void DidStopFlingingOnBrowser() override {
     notified_client_after_fling_stop_ = true;
   }
+  void RegisterFlingSchedulerObserver(
+      base::WeakPtr<FlingController> fling_controller) override {}
+  void UnregisterFlingSchedulerObserver() override {}
   bool NeedsBeginFrameForFlingProgress() override {
     return needs_begin_frame_for_fling_progress_;
   }

content/browser/renderer_host/input/fling_scheduler.cc

@@ -24,31 +24,39 @@ FlingScheduler::~FlingScheduler() {
     observed_compositor_->RemoveAnimationObserver(this);
 }
 
-void FlingScheduler::ScheduleFlingProgress(
-    base::WeakPtr<FlingController> fling_controller) {
-  DCHECK(fling_controller);
-  fling_controller_ = fling_controller;
+void FlingScheduler::ScheduleFlingProgress() {
   // Don't do anything if a ui::Compositor is already being observed.
   if (observed_compositor_)
     return;
+
+  host_->SetNeedsBeginFrameForFlingProgress();
+}
+
+void FlingScheduler::RegisterFlingSchedulerObserver(
+    base::WeakPtr<FlingController> fling_controller) {
+  DCHECK(fling_controller);
+  fling_controller_ = fling_controller;
+  DCHECK(!observed_compositor_);
+
   ui::Compositor* compositor = GetCompositor();
   // If a ui::Compositor can't be obtained, ask the host for BeginFrames.
-  if (!compositor) {
-    host_->SetNeedsBeginFrameForFlingProgress();
+  if (!compositor)
     return;
-  }
+
   compositor->AddAnimationObserver(this);
   observed_compositor_ = compositor;
 }
 
-void FlingScheduler::DidStopFlingingOnBrowser(
-    base::WeakPtr<FlingController> fling_controller) {
-  DCHECK(fling_controller);
+void FlingScheduler::UnregisterFlingSchedulerObserver() {
   if (observed_compositor_) {
     observed_compositor_->RemoveAnimationObserver(this);
     observed_compositor_ = nullptr;
   }
   fling_controller_ = nullptr;
+}
+
+void FlingScheduler::DidStopFlingingOnBrowser() {
+  UnregisterFlingSchedulerObserver();
   host_->DidStopFlinging();
 }
 

content/browser/renderer_host/input/fling_scheduler.h

@@ -24,10 +24,11 @@ class CONTENT_EXPORT FlingScheduler : public FlingSchedulerBase,
   ~FlingScheduler() override;
 
   // FlingControllerSchedulerClient
-  void ScheduleFlingProgress(
-      base::WeakPtr<FlingController> fling_controller) override;
-  void DidStopFlingingOnBrowser(
+  void ScheduleFlingProgress() override;
+  void RegisterFlingSchedulerObserver(
       base::WeakPtr<FlingController> fling_controller) override;
+  void UnregisterFlingSchedulerObserver() override;
+  void DidStopFlingingOnBrowser() override;
   bool NeedsBeginFrameForFlingProgress() override;
 
   // FlingSchedulerBase

content/browser/renderer_host/input/fling_scheduler_android.cc

@@ -20,33 +20,41 @@ FlingSchedulerAndroid::~FlingSchedulerAndroid() {
     observed_window_->RemoveObserver(this);
 }
 
-void FlingSchedulerAndroid::ScheduleFlingProgress(
-    base::WeakPtr<FlingController> fling_controller) {
-  DCHECK(fling_controller);
-  fling_controller_ = fling_controller;
+void FlingSchedulerAndroid::ScheduleFlingProgress() {
   if (!observed_window_) {
-    ui::WindowAndroid* window = GetRootWindow();
     // If the root window does not have a Compositor (happens on Android
     // WebView), we'll never receive an OnAnimate call. In this case fall back
     // to BeginFrames coming from the host.
-    if (!window || !window->GetCompositor()) {
-      host_->SetNeedsBeginFrameForFlingProgress();
-      return;
-    }
-    window->AddObserver(this);
-    observed_window_ = window;
+    host_->SetNeedsBeginFrameForFlingProgress();
+    return;
   }
   observed_window_->SetNeedsAnimate();
 }
 
-void FlingSchedulerAndroid::DidStopFlingingOnBrowser(
+void FlingSchedulerAndroid::RegisterFlingSchedulerObserver(
     base::WeakPtr<FlingController> fling_controller) {
   DCHECK(fling_controller);
+  fling_controller_ = fling_controller;
+
+  DCHECK(!observed_window_);
+  ui::WindowAndroid* window = GetRootWindow();
+  if (!window || !window->GetCompositor())
+    return;
+
+  window->AddObserver(this);
+  observed_window_ = window;
+}
+
+void FlingSchedulerAndroid::UnregisterFlingSchedulerObserver() {
   if (observed_window_) {
     observed_window_->RemoveObserver(this);
     observed_window_ = nullptr;
   }
   fling_controller_ = nullptr;
+}
+
+void FlingSchedulerAndroid::DidStopFlingingOnBrowser() {
+  UnregisterFlingSchedulerObserver();
   host_->DidStopFlinging();
 }
 

content/browser/renderer_host/input/fling_scheduler_android.h

@@ -21,10 +21,11 @@ class CONTENT_EXPORT FlingSchedulerAndroid : public FlingSchedulerBase,
   ~FlingSchedulerAndroid() override;
 
   // FlingControllerSchedulerClient
-  void ScheduleFlingProgress(
-      base::WeakPtr<FlingController> fling_controller) override;
-  void DidStopFlingingOnBrowser(
+  void ScheduleFlingProgress() override;
+  void RegisterFlingSchedulerObserver(
       base::WeakPtr<FlingController> fling_controller) override;
+  void UnregisterFlingSchedulerObserver() override;
+  void DidStopFlingingOnBrowser() override;
   bool NeedsBeginFrameForFlingProgress() override;
 
   // FlingSchedulerBase

content/browser/renderer_host/input/fling_scheduler_unittest.cc

@@ -18,15 +18,13 @@ class FakeFlingScheduler : public FlingScheduler {
  public:
   FakeFlingScheduler(RenderWidgetHostImpl* host) : FlingScheduler(host) {}
 
-  void ScheduleFlingProgress(
-      base::WeakPtr<FlingController> fling_controller) override {
-    FlingScheduler::ScheduleFlingProgress(fling_controller);
+  void ScheduleFlingProgress() override {
+    FlingScheduler::ScheduleFlingProgress();
     fling_in_progress_ = true;
   }
 
-  void DidStopFlingingOnBrowser(
-      base::WeakPtr<FlingController> fling_controller) override {
-    FlingScheduler::DidStopFlingingOnBrowser(fling_controller);
+  void DidStopFlingingOnBrowser() override {
+    FlingScheduler::DidStopFlingingOnBrowser();
     fling_in_progress_ = false;
   }
 
@@ -56,6 +54,7 @@ class FlingSchedulerTest : public testing::Test,
     fling_scheduler_ = std::make_unique<FakeFlingScheduler>(widget_host_);
     fling_controller_ = std::make_unique<FlingController>(
         this, fling_scheduler_.get(), FlingController::Config());
+    fling_controller_->RegisterFlingSchedulerObserver();
   }
 
   void TearDown() override {

content/browser/renderer_host/input/gesture_event_queue.cc

@@ -148,6 +148,12 @@ void GestureEventQueue::QueueAndForwardIfNecessary(
     DCHECK_NE(gesture_event.event.GetType(),
               WebInputEvent::kGestureFlingCancel);
     coalesced_gesture_events_.push_back(gesture_event);
+    if (gesture_event.event.GetType() == WebInputEvent::kGestureScrollBegin) {
+      fling_controller_.RegisterFlingSchedulerObserver();
+    } else if (gesture_event.event.GetType() ==
+               WebInputEvent::kGestureScrollEnd) {
+      fling_controller_.UnregisterFlingSchedulerObserver();
+    }
     client_->SendGestureEventImmediately(gesture_event);
     return;
   }
@@ -160,6 +166,10 @@ void GestureEventQueue::QueueAndForwardIfNecessary(
     case WebInputEvent::kGestureScrollBegin:
       if (OnScrollBegin(gesture_event))
         return;
+      fling_controller_.RegisterFlingSchedulerObserver();
+      break;
+    case WebGestureEvent::kGestureScrollEnd:
+      fling_controller_.UnregisterFlingSchedulerObserver();
       break;
     default:
       break;

content/browser/renderer_host/input/gesture_event_queue_unittest.cc

@@ -103,10 +103,15 @@ class GestureEventQueueTest : public testing::Test,
       const GestureEventWithLatencyInfo& gesture_event) override {}
 
   // FlingControllerSchedulerClient
-  void ScheduleFlingProgress(
-      base::WeakPtr<FlingController> fling_controller) override {}
-  void DidStopFlingingOnBrowser(
-      base::WeakPtr<FlingController> fling_controller) override {}
+  void ScheduleFlingProgress() override {}
+  void DidStopFlingingOnBrowser() override {}
+  void RegisterFlingSchedulerObserver(
+      base::WeakPtr<FlingController> fling_controller) override {
+    fling_scheduler_observer_registered_ = true;
+  }
+  void UnregisterFlingSchedulerObserver() override {
+    fling_scheduler_observer_registered_ = false;
+  }
   bool NeedsBeginFrameForFlingProgress() override { return false; }
 
  protected:
@@ -188,6 +193,10 @@ class GestureEventQueueTest : public testing::Test,
         SyntheticWebGestureEventBuilder::Build(type, sourceDevice)));
   }
 
+  bool fling_scheduler_observer_registered() const {
+    return fling_scheduler_observer_registered_;
+  }
+
   unsigned GestureEventQueueSize() {
     return queue()->coalesced_gesture_events_.size();
   }
@@ -234,6 +243,7 @@ class GestureEventQueueTest : public testing::Test,
   WebGestureEvent last_acked_event_;
   std::unique_ptr<InputEventAckState> sync_ack_result_;
   std::unique_ptr<WebGestureEvent> sync_followup_event_;
+  bool fling_scheduler_observer_registered_ = false;
   base::test::ScopedFeatureList feature_list_;
 };
 
@@ -1243,4 +1253,17 @@ TEST_F(GestureEventQueueWithCompositorEventQueueTest,
   EXPECT_EQ(0U, GetAndResetSentGestureEventCount());
 }
 
+// Checks that the fling scheduler observer is regsitered/unregistered on
+// GSB/GSE respectively.
+TEST_F(GestureEventQueueWithCompositorEventQueueTest,
+       FlingSchedulerOpbserverRegisteredOnGSB) {
+  EXPECT_FALSE(fling_scheduler_observer_registered());
+  SimulateGestureEvent(WebInputEvent::kGestureScrollBegin,
+                       blink::kWebGestureDeviceTouchscreen);
+  EXPECT_TRUE(fling_scheduler_observer_registered());
+  SimulateGestureEvent(WebInputEvent::kGestureScrollEnd,
+                       blink::kWebGestureDeviceTouchscreen);
+  EXPECT_FALSE(fling_scheduler_observer_registered());
+}
+
 }  // namespace content

content/browser/renderer_host/input/mock_input_router_client.h

@@ -67,10 +67,11 @@ class MockInputRouterClient : public InputRouterClient,
   }
 
   // FlingControllerSchedulerClient
-  void ScheduleFlingProgress(
-      base::WeakPtr<FlingController> fling_controller) override {}
-  void DidStopFlingingOnBrowser(
+  void ScheduleFlingProgress() override {}
+  void DidStopFlingingOnBrowser() override {}
+  void RegisterFlingSchedulerObserver(
       base::WeakPtr<FlingController> fling_controller) override {}
+  void UnregisterFlingSchedulerObserver() override {}
   bool NeedsBeginFrameForFlingProgress() override;
 
  private: