From bd95d685e771dd103c842f675e3b391c40f27cbe Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 27 Sep 2019 11:27:26 -0700 Subject: [PATCH] More fixes for comments --- shell/common/animator.cc | 2 +- shell/common/animator.h | 18 ++++++++-- shell/common/engine.cc | 2 +- shell/common/engine.h | 12 +++---- shell/common/pointer_data_dispatcher.cc | 3 ++ shell/common/pointer_data_dispatcher.h | 25 +++++++------- shell/common/shell_test.cc | 45 +++++++++++++------------ shell/common/vsync_waiter.cc | 4 +-- shell/common/vsync_waiter.h | 4 +-- 9 files changed, 66 insertions(+), 49 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 7e3f63eff91aa..b0ef6fca098f2 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -244,7 +244,7 @@ void Animator::AwaitVSync() { delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_); } -void Animator::ScheduleSecondaryVsyncCallback(std::function callback) { +void Animator::ScheduleSecondaryVsyncCallback(fml::closure callback) { waiter_->ScheduleSecondaryCallback(std::move(callback)); } diff --git a/shell/common/animator.h b/shell/common/animator.h index 8d1fbedc4af00..dddc70b145b46 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -52,10 +52,22 @@ class Animator final { void Render(std::unique_ptr layer_tree); - /// Add a secondary callback for the next vsync. + //-------------------------------------------------------------------------- + /// @brief Schedule a secondary callback to be executed right after the + /// main `VsyncWaiter::AsyncWaitForVsync` callback (which is added + /// by `Animator::RequestFrame`). /// - /// See also |PointerDataDispatcher::ScheduleSecondaryVsyncCallback|. - void ScheduleSecondaryVsyncCallback(std::function callback); + /// Like the callback in `AsyncWaitForVsync`, this callback is + /// only scheduled to be called once, and it's supposed to be + /// called in the UI thread. If there is no AsyncWaitForVsync + /// callback (`Animator::RequestFrame` is not called), this + /// secondary callback will still be executed at vsync. + /// + /// This callback is used to provide the vsync signal needed by + /// `SmoothPointerDataDispatcher`. + /// + /// @see `PointerDataDispatcher::ScheduleSecondaryVsyncCallback`. + void ScheduleSecondaryVsyncCallback(fml::closure callback); void Start(); diff --git a/shell/common/engine.cc b/shell/common/engine.cc index ce2b7a259da4c..f106c662dafff 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -474,7 +474,7 @@ void Engine::DoDispatchPacket(std::unique_ptr packet, } } -void Engine::ScheduleSecondaryVsyncCallback(std::function callback) { +void Engine::ScheduleSecondaryVsyncCallback(fml::closure callback) { animator_->ScheduleSecondaryVsyncCallback(std::move(callback)); } diff --git a/shell/common/engine.h b/shell/common/engine.h index ae1ab081ee5b2..2e4fd41964bb2 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -236,11 +236,11 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { /// tasks that require access to components /// that cannot be safely accessed by the /// engine. This is the shell. - /// @param dispatcher_maker The `std::function` provided by - /// `PlatformView` for engine to create the - /// pointer data dispatcher. Similar to other - /// engine resources, this dispatcher_maker and - /// its returned dispatcher is only safe to be + /// @param dispatcher_maker The callback provided by `PlatformView` for + /// engine to create the pointer data + /// dispatcher. Similar to other engine + /// resources, this dispatcher_maker and its + /// returned dispatcher is only safe to be /// called from the UI thread. /// @param vm An instance of the running Dart VM. /// @param[in] isolate_snapshot The snapshot used to create the root @@ -714,7 +714,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { uint64_t trace_flow_id) override; // |PointerDataDispatcher::Delegate| - void ScheduleSecondaryVsyncCallback(std::function callback) override; + void ScheduleSecondaryVsyncCallback(fml::closure callback) override; private: Engine::Delegate& delegate_; diff --git a/shell/common/pointer_data_dispatcher.cc b/shell/common/pointer_data_dispatcher.cc index 508fc0c09c113..8341ecec10ab2 100644 --- a/shell/common/pointer_data_dispatcher.cc +++ b/shell/common/pointer_data_dispatcher.cc @@ -8,6 +8,9 @@ namespace flutter { PointerDataDispatcher::~PointerDataDispatcher() = default; DefaultPointerDataDispatcher::~DefaultPointerDataDispatcher() = default; + +SmoothPointerDataDispatcher::SmoothPointerDataDispatcher(Delegate& delegate) + : DefaultPointerDataDispatcher(delegate), weak_factory_(this) {} SmoothPointerDataDispatcher::~SmoothPointerDataDispatcher() = default; void DefaultPointerDataDispatcher::DispatchPacket( diff --git a/shell/common/pointer_data_dispatcher.h b/shell/common/pointer_data_dispatcher.h index 06db4d501be35..d67a5b6fe7ff3 100644 --- a/shell/common/pointer_data_dispatcher.h +++ b/shell/common/pointer_data_dispatcher.h @@ -47,21 +47,20 @@ class PointerDataDispatcher { virtual void DoDispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) = 0; - //---------------------------------------------------------------------------- + //-------------------------------------------------------------------------- /// @brief Schedule a secondary callback to be executed right after the /// main `VsyncWaiter::AsyncWaitForVsync` callback (which is added /// by `Animator::RequestFrame`). /// /// Like the callback in `AsyncWaitForVsync`, this callback is - /// only scheduled to be called once, and it's supposed to be - /// called in the UI thread. If there is no AsyncWaitForVsync - /// callback (`Animator::RequestFrame` is not called), this - /// secondary callback will still be executed at vsync. + /// only scheduled to be called once, and it will be called in the + /// UI thread. If there is no AsyncWaitForVsync callback + /// (`Animator::RequestFrame` is not called), this secondary + /// callback will still be executed at vsync. /// /// This callback is used to provide the vsync signal needed by /// `SmoothPointerDataDispatcher`. - virtual void ScheduleSecondaryVsyncCallback( - std::function callback) = 0; + virtual void ScheduleSecondaryVsyncCallback(fml::closure callback) = 0; }; //---------------------------------------------------------------------------- @@ -97,10 +96,11 @@ class DefaultPointerDataDispatcher : public PointerDataDispatcher { }; //------------------------------------------------------------------------------ -/// The dispatcher that filters out irregular input events delivery to provide -/// a smooth scroll on iPhone X/Xs. -/// -/// This fixes https://github.com/flutter/flutter/issues/31086. +/// A dispatcher that may temporarily store and defer the last received +/// PointerDataPacket if multiple packets are received in one VSYNC. The +/// deferred packet will be sent in the next vsync in order to smooth out the +/// events. This filters out irregular input events delivery to provide a smooth +/// scroll on iPhone X/Xs. /// /// It works as follows: /// @@ -135,8 +135,7 @@ class DefaultPointerDataDispatcher : public PointerDataDispatcher { /// See also input_events_unittests.cc where we test all our claims above. class SmoothPointerDataDispatcher : public DefaultPointerDataDispatcher { public: - SmoothPointerDataDispatcher(Delegate& delegate) - : DefaultPointerDataDispatcher(delegate), weak_factory_(this) {} + SmoothPointerDataDispatcher(Delegate& delegate); // |PointerDataDispatcer| void DispatchPacket(std::unique_ptr packet, diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 470c63557f52b..12c6348b72e41 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #define FML_USED_ON_EMBEDDER #include "flutter/shell/common/shell_test.h" @@ -102,15 +103,13 @@ void ShellTest::RunEngine(Shell* shell, RunConfiguration configuration) { } void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) { - std::promise finished; + std::promise restarted; fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetUITaskRunner(), - [shell, &finished, &configuration]() { - bool restarted = shell->engine_->Restart(std::move(configuration)); - ASSERT_TRUE(restarted); - finished.set_value(true); + [shell, &restarted, &configuration]() { + restarted.set_value(shell->engine_->Restart(std::move(configuration))); }); - finished.get_future().wait(); + ASSERT_TRUE(restarted.get_future().get()); } void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { @@ -277,21 +276,25 @@ std::future ShellTestVsyncClock::NextVSync() { void ShellTestVsyncWaiter::AwaitVSync() { FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); auto vsync_future = clock_.NextVSync(); - vsync_future.wait(); - - // Post the `FireCallback` to the Platform thread so earlier Platform tasks - // (specifically, the `VSyncFlush` call) will be finished before - // `FireCallback` is executed. This is only needed for our unit tests. - // - // Without this, the repeated VSYNC signals in `VSyncFlush` may start both the - // current frame in the UI thread and the next frame in the secondary - // callback (both of them are waiting for VSYNCs). That breaks the unit test's - // assumption that each frame's VSYNC must be issued by different `VSyncFlush` - // call (which reset the `will_draw_new_frame` bit). - // - // For example, HandlesActualIphoneXsInputEvents will fail without this. - task_runners_.GetPlatformTaskRunner()->PostTask( - [this]() { FireCallback(fml::TimePoint::Now(), fml::TimePoint::Now()); }); + + std::async([&vsync_future, this]() { + vsync_future.wait(); + + // Post the `FireCallback` to the Platform thread so earlier Platform tasks + // (specifically, the `VSyncFlush` call) will be finished before + // `FireCallback` is executed. This is only needed for our unit tests. + // + // Without this, the repeated VSYNC signals in `VSyncFlush` may start both + // the current frame in the UI thread and the next frame in the secondary + // callback (both of them are waiting for VSYNCs). That breaks the unit + // test's assumption that each frame's VSYNC must be issued by different + // `VSyncFlush` call (which resets the `will_draw_new_frame` bit). + // + // For example, HandlesActualIphoneXsInputEvents will fail without this. + task_runners_.GetPlatformTaskRunner()->PostTask([this]() { + FireCallback(fml::TimePoint::Now(), fml::TimePoint::Now()); + }); + }); } ShellTestPlatformView::ShellTestPlatformView(PlatformView::Delegate& delegate, diff --git a/shell/common/vsync_waiter.cc b/shell/common/vsync_waiter.cc index fb31330b9388d..c191a13490006 100644 --- a/shell/common/vsync_waiter.cc +++ b/shell/common/vsync_waiter.cc @@ -55,7 +55,7 @@ void VsyncWaiter::AsyncWaitForVsync(Callback callback) { AwaitVSync(); } -void VsyncWaiter::ScheduleSecondaryCallback(std::function callback) { +void VsyncWaiter::ScheduleSecondaryCallback(fml::closure callback) { FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); if (!callback) { @@ -85,7 +85,7 @@ void VsyncWaiter::ScheduleSecondaryCallback(std::function callback) { void VsyncWaiter::FireCallback(fml::TimePoint frame_start_time, fml::TimePoint frame_target_time) { Callback callback; - std::function secondary_callback; + fml::closure secondary_callback; { std::scoped_lock lock(callback_mutex_); diff --git a/shell/common/vsync_waiter.h b/shell/common/vsync_waiter.h index 7dd1dedfba800..8e476f0d63157 100644 --- a/shell/common/vsync_waiter.h +++ b/shell/common/vsync_waiter.h @@ -29,7 +29,7 @@ class VsyncWaiter : public std::enable_shared_from_this { /// Add a secondary callback for the next vsync. /// /// See also |PointerDataDispatcher::ScheduleSecondaryVsyncCallback|. - void ScheduleSecondaryCallback(std::function callback); + void ScheduleSecondaryCallback(fml::closure callback); static constexpr float kUnknownRefreshRateFPS = 0.0; @@ -61,7 +61,7 @@ class VsyncWaiter : public std::enable_shared_from_this { Callback callback_ FML_GUARDED_BY(callback_mutex_); std::mutex secondary_callback_mutex_; - std::function secondary_callback_ FML_GUARDED_BY(callback_mutex_); + fml::closure secondary_callback_ FML_GUARDED_BY(callback_mutex_); FML_DISALLOW_COPY_AND_ASSIGN(VsyncWaiter); };