Commit d9a56afc authored by Daniel Libby's avatar Daniel Libby Committed by Commit Bot

Add explicit flag for compositor scrollbar injected gestures

The original change to enable scrollbar latency for the composited
scrollbars incorrectly used an existing member to try and determine
whether a GestureScrollUpdate was the first one in an injected sequence
or not. is_first_gesture_scroll_update_ was incorrect because it is only
updated when input is actually dispatched to InputHandlerProxy, and the
flag is cleared for all GSUs before the location where it was being
read.

This bug was missed because of incorrect tests. The
VerifyRecordedSamplesForHistogram method doesn't actually assert or
expect anything - the return value must be inspected.

As part of fixing up the tests, I made a few other changes to get them
passing consistently across all platforms:
- turn on main thread scrollbar injection feature (in case it's ever
  turned off we don't want the tests to start failing)
- enable mock scrollbars
- disable smooth scrolling
- don't run scrollbar tests on Android

The composited scrollbar button test is disabled due to a bug in how
the mock theme reports its button sizes, which throws off the region
detection in ScrollbarLayerImplBase::IdentifyScrollbarPart (filed
crbug.com/974063 for this issue).

Change-Id: Ie1a762a5f6ecc264d22f0256db68f141fc76b950

Bug: 954007
Change-Id: Ib258e08e083e79da90ba2e4e4216e4879cf00cf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1652741
Commit-Queue: Daniel Libby <dlibby@microsoft.com>
Reviewed-by: default avatarDavid Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669086}
parent 0781a31a
......@@ -174,6 +174,7 @@ InputHandlerProxy::InputHandlerProxy(cc::InputHandler* input_handler,
current_overscroll_params_(nullptr),
has_ongoing_compositor_scroll_or_pinch_(false),
is_first_gesture_scroll_update_(false),
last_injected_gesture_was_begin_(false),
tick_clock_(base::DefaultTickClock::GetInstance()),
snap_fling_controller_(std::make_unique<cc::SnapFlingController>(this)),
compositor_touch_action_enabled_(
......@@ -311,15 +312,10 @@ void InputHandlerProxy::DispatchSingleInputEvent(
current_overscroll_params_.reset();
blink::WebGestureEvent::Type type = event_with_callback->event().GetType();
if (type == blink::WebGestureEvent::kGestureScrollUpdate) {
EnsureScrollUpdateLatencyComponent(
&monitored_latency_info, event_with_callback->event().TimeStamp());
}
InputHandlerProxy::EventDisposition disposition = RouteToTypeSpecificHandler(
event_with_callback->event(), original_latency_info);
blink::WebGestureEvent::Type type = event_with_callback->event().GetType();
switch (type) {
case blink::WebGestureEvent::kGestureScrollBegin:
is_first_gesture_scroll_update_ = true;
......@@ -365,32 +361,6 @@ void InputHandlerProxy::DispatchSingleInputEvent(
std::move(current_overscroll_params_));
}
// Scroll updates injected from within the renderer process will not have a
// scroll update component, since those are added to the latency info
// in the browser process before being dispatched to the renderer.
void InputHandlerProxy::EnsureScrollUpdateLatencyComponent(
LatencyInfo* monitored_latency_info,
base::TimeTicks original_timestamp) {
// Currently we only expect LatencyInfo's of type SCROLLBAR to get into
// this state, but there are a few exceptions (i.e. GestureScrollUpdates
// that are coalesced with GesturePinchUpdates via
// CompositorThreadEventQueue::Queue/CoalesceScrollAndPinch).
// TODO(dlibby): Update that codepath and turn this into a DCHECK when there
// is a missing scroll update component.
if (monitored_latency_info->source_event_type() !=
ui::SourceEventType::SCROLLBAR)
return;
// Add a scroll update component to the latency info if one doesn't exist,
// based on whether or not this is the first scroll update we've seen in a
// gesture sequence.
monitored_latency_info->AddLatencyNumberWithTimestamp(
(is_first_gesture_scroll_update_)
? ui::INPUT_EVENT_LATENCY_FIRST_SCROLL_UPDATE_ORIGINAL_COMPONENT
: ui::INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT,
original_timestamp, 1);
}
void InputHandlerProxy::DispatchQueuedInputEvents() {
// Calling |NowTicks()| is expensive so we only want to do it once.
base::TimeTicks now = tick_clock_->NowTicks();
......@@ -436,6 +406,24 @@ void InputHandlerProxy::InjectScrollbarGestureScroll(
DCHECK(!scrollbar_latency_info.FindLatency(
ui::INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_IMPL_COMPONENT, nullptr));
if (type == WebInputEvent::Type::kGestureScrollBegin) {
last_injected_gesture_was_begin_ = true;
} else {
if (type == WebInputEvent::Type::kGestureScrollUpdate) {
// For injected GSUs, add a scroll update component to the latency info
// so that it is properly classified as a scroll. If the last injected
// gesture was a GSB, then this GSU is the first scroll update - mark
// the LatencyInfo as such.
scrollbar_latency_info.AddLatencyNumberWithTimestamp(
(last_injected_gesture_was_begin_)
? ui::INPUT_EVENT_LATENCY_FIRST_SCROLL_UPDATE_ORIGINAL_COMPONENT
: ui::INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT,
original_timestamp, 1);
}
last_injected_gesture_was_begin_ = false;
}
std::unique_ptr<EventWithCallback> gesture_event_with_callback_update =
std::make_unique<EventWithCallback>(
std::move(web_scoped_gesture_event), scrollbar_latency_info,
......
......@@ -187,12 +187,6 @@ class InputHandlerProxy : public cc::InputHandlerClient,
bool* is_touching_scrolling_layer,
cc::TouchAction* white_listed_touch_action);
// Scroll updates injected from within the renderer process will not have a
// scroll update component, since those are added to the latency info
// in the browser process before being dispatched to the renderer.
void EnsureScrollUpdateLatencyComponent(LatencyInfo* monitored_latency_info,
base::TimeTicks original_timestamp);
InputHandlerProxyClient* client_;
cc::InputHandler* input_handler_;
......@@ -235,6 +229,13 @@ class InputHandlerProxy : public cc::InputHandlerClient,
bool has_ongoing_compositor_scroll_or_pinch_;
bool is_first_gesture_scroll_update_;
// Whether the last injected scroll gesture was a GestureScrollBegin. Used to
// determine which GestureScrollUpdate is the first in a gesture sequence for
// latency classification. This is separate from
// |is_first_gesture_scroll_update_| and is used to determine which type of
// latency component should be added for injected GestureScrollUpdates.
bool last_injected_gesture_was_begin_;
const base::TickClock* tick_clock_;
std::unique_ptr<cc::SnapFlingController> snap_fling_controller_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment