Skip to content

Commit

Permalink
[ui] Handle late ACKed touch events more properly
Browse files Browse the repository at this point in the history
This CL adds an extra function named
`OnGestureProviderAuraWillBeDestroyed()` to `GestureProviderAuraClient`
so that `GestureProviderAuraClient` can response to destruction of
a `GestureProviderAura` instance.

See the comment 27 under this issue for more details.

Bug: 1292264
Change-Id: I53502e896d3a36f9610ca48c11b07422e5b4ce03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3489646
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Sean Topping <seantopping@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984964}
  • Loading branch information
Andrew Xu authored and Chromium LUCI CQ committed Mar 24, 2022
1 parent 00c6a1e commit d2fdb99
Show file tree
Hide file tree
Showing 6 changed files with 104 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,10 @@ FullscreenMagnificationController::FullscreenMagnificationController(
this, gesture_provider_client_.get());
}

FullscreenMagnificationController::~FullscreenMagnificationController() {}
FullscreenMagnificationController::~FullscreenMagnificationController() {
// Destroy `gesture_provider_` before `gesture_provider_client_`.
gesture_provider_.reset();
}

void FullscreenMagnificationController::SetEnabled(bool enabled) {
if (is_enabled_ == enabled)
Expand Down
70 changes: 70 additions & 0 deletions ui/aura/gestures/gesture_recognizer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "ui/events/event_utils.h"
#include "ui/events/gesture_detection/gesture_configuration.h"
#include "ui/events/gesture_detection/gesture_provider.h"
#include "ui/events/gestures/gesture_recognizer_impl.h"
#include "ui/events/gestures/gesture_types.h"
#include "ui/events/test/event_generator.h"
#include "ui/events/test/events_test_utils.h"
Expand Down Expand Up @@ -1748,6 +1749,75 @@ TEST_F(GestureRecognizerTest, GestureTapFollowedByScroll) {
EXPECT_TRUE(delegate->fling());
}

// Verifies that destroying a gesture provider aura instance before a touch
// event is ACKed works as expected (see https://crbug.com/1292264).
TEST_F(GestureRecognizerTest, DestroyGestureProviderAuraBeforeAck) {
TimedEvents tes;
const int kTouchId = 4;
std::unique_ptr<GestureEventConsumeDelegate> delegate(
new GestureEventConsumeDelegate());
std::unique_ptr<aura::Window> window1(CreateTestWindowWithDelegate(
delegate.get(), /*id=*/-2345, /*bounds=*/gfx::Rect(0, 0, 50, 50),
/*parent=*/root_window()));

// Touch press then release on `window1`.
constexpr gfx::Point touch_location(/*x=*/10, /*y=*/20);
ui::TouchEvent press(
ui::ET_TOUCH_PRESSED, touch_location, /*time_stamp=*/tes.Now(),
ui::PointerDetails(ui::EventPointerType::kTouch, kTouchId));
delegate->Reset();
DispatchEventUsingWindowDispatcher(&press);
EXPECT_TRUE(delegate->tap_down());
delegate->Reset();
ui::TouchEvent release(
ui::ET_TOUCH_RELEASED, touch_location,
/*time_stamp=*/press.time_stamp() + base::Milliseconds(50),
ui::PointerDetails(ui::EventPointerType::kTouch, kTouchId));
DispatchEventUsingWindowDispatcher(&release);
EXPECT_FALSE(delegate->tap_down());

// Verify that the gesture provider for `window1` is created.
auto* gesture_recognizer = static_cast<ui::GestureRecognizerImpl*>(
aura::Env::GetInstance()->gesture_recognizer());
const auto& consumer_provider_mappings =
gesture_recognizer->consumer_gesture_provider_;
EXPECT_NE(consumer_provider_mappings.cend(),
consumer_provider_mappings.find(window1.get()));

// Create a second window for handling touch events.
std::unique_ptr<QueueTouchEventDelegate> delegate2(
new QueueTouchEventDelegate(host()->dispatcher()));
const int kTouchId2 = 4;
std::unique_ptr<aura::Window> window2(CreateTestWindowWithDelegate(
delegate2.get(), /*id=*/-1234, /*bounds=*/gfx::Rect(100, 100, 500, 500),
root_window()));
delegate2->set_window(window2.get());

// Send a press event on `window2`. Verify that the gesture provider for
// `window2` is created.
ui::TouchEvent press2(
ui::ET_TOUCH_PRESSED, /*location=*/gfx::Point(200, 200),
/*time_stamp=*/tes.Now(),
ui::PointerDetails(ui::EventPointerType::kTouch, kTouchId2));
DispatchEventUsingWindowDispatcher(&press2);
EXPECT_NE(consumer_provider_mappings.cend(),
consumer_provider_mappings.find(window2.get()));

// Verify that `press2` is associated with a gesture provider raw pointer.
const auto& event_provider_mappings =
gesture_recognizer->event_to_gesture_provider_;
EXPECT_NE(event_provider_mappings.cend(),
event_provider_mappings.find(press2.unique_event_id()));

// Before ACKing `press2`, replacing the gesture provider of `window2` with a
// new value through event transferal.
aura::Env::GetInstance()->gesture_recognizer()->TransferEventsTo(
window1.get(), window2.get(), ui::TransferTouchesBehavior::kCancel);

// ACK the press event.
delegate2->ReceivedAck();
}

TEST_F(GestureRecognizerTest, AsynchronousGestureRecognition) {
std::unique_ptr<QueueTouchEventDelegate> queued_delegate(
new QueueTouchEventDelegate(host()->dispatcher()));
Expand Down
6 changes: 4 additions & 2 deletions ui/events/gestures/gesture_provider_aura.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ GestureProviderAura::GestureProviderAura(GestureConsumer* consumer,
kDoubleTapPlatformSupport);
}

GestureProviderAura::~GestureProviderAura() {}
GestureProviderAura::~GestureProviderAura() {
client_->OnGestureProviderAuraWillBeDestroyed(this);
}

bool GestureProviderAura::OnTouchEvent(TouchEvent* event) {
if (!pointer_state_.OnTouch(*event))
Expand Down Expand Up @@ -114,4 +116,4 @@ void GestureProviderAura::OnTouchEnter(int pointer_id, float x, float y) {
false /* is_source_touch_event_set_blocking */);
}

} // namespace content
} // namespace ui
4 changes: 4 additions & 0 deletions ui/events/gestures/gesture_provider_aura.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ class EVENTS_EXPORT GestureProviderAuraClient {
virtual ~GestureProviderAuraClient() {}
virtual void OnGestureEvent(GestureConsumer* consumer,
GestureEvent* event) = 0;

// Called when `gesture_provider` will be destroyed.
virtual void OnGestureProviderAuraWillBeDestroyed(
GestureProviderAura* gesture_provider) {}
};

// Provides gesture detection and dispatch given a sequence of touch events
Expand Down
12 changes: 12 additions & 0 deletions ui/events/gestures/gesture_recognizer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,18 @@ void GestureRecognizerImpl::OnGestureEvent(GestureConsumer* raw_input_consumer,
DispatchGestureEvent(raw_input_consumer, event);
}

void GestureRecognizerImpl::OnGestureProviderAuraWillBeDestroyed(
GestureProviderAura* gesture_provider) {
// Clean `event_to_gesture_provider_` by removing invalid raw pointers.
for (auto iter = event_to_gesture_provider_.begin();
iter != event_to_gesture_provider_.end();) {
if (iter->second == gesture_provider)
iter = event_to_gesture_provider_.erase(iter);
else
++iter;
}
}

GestureEventHelper* GestureRecognizerImpl::FindDispatchHelperForConsumer(
GestureConsumer* consumer) {
for (GestureEventHelper* helper : helpers_) {
Expand Down
10 changes: 10 additions & 0 deletions ui/events/gestures/gesture_recognizer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
#include "ui/events/types/event_type.h"
#include "ui/gfx/geometry/point.h"

namespace aura::test {
FORWARD_DECLARE_TEST(GestureRecognizerTest,
DestroyGestureProviderAuraBeforeAck);
} // namespace aura::test

namespace ui {
class GestureConsumer;
class GestureEvent;
Expand Down Expand Up @@ -76,6 +81,9 @@ class EVENTS_EXPORT GestureRecognizerImpl : public GestureRecognizer,
GestureConsumer* consumer) override;

private:
FRIEND_TEST_ALL_PREFIXES(aura::test::GestureRecognizerTest,
DestroyGestureProviderAuraBeforeAck);

// Sets up the target consumer for gestures based on the touch-event.
void SetupTargets(const TouchEvent& event, GestureConsumer* consumer);

Expand All @@ -99,6 +107,8 @@ class EVENTS_EXPORT GestureRecognizerImpl : public GestureRecognizer,
// Overridden from GestureProviderAuraClient
void OnGestureEvent(GestureConsumer* raw_input_consumer,
GestureEvent* event) override;
void OnGestureProviderAuraWillBeDestroyed(
GestureProviderAura* gesture_provider) override;

// Convenience method to find the GestureEventHelper that can dispatch events
// to a specific |consumer|.
Expand Down

0 comments on commit d2fdb99

Please sign in to comment.