Relands: Telemetry: hooks "chrome.gpuBenchmarking.smoothScrollBy" with java on android.

Relands: Telemetry: hooks "chrome.gpuBenchmarking.smoothScrollBy" with java on android.

In order to provide more realistic benchmarks, instead of shortcutting in native via mousewheels,
let Telemetry cross all the boundaries and synthesize MotionEvents all the way up in java.
(relands after r173752 reverted https://codereview.chromium.org/11415227/)

BUG=166521, 168599
TEST=tools/perf/run_multipage_benchmarks --browser=android-content-shell scrolling_benchmark tools/perf/page_sets/top_25.json

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199112

[bulach, 2013-01-09 16:08:53]

this was reverted, so I'm trying to reland...

the first patchset 1 is just a git revert, so you may want to review only the
diffs between those two patchsets.

The notable changes are:
1. in render_widget_host_impl.cc, there's a very simple queue for
"in_process_event_types" that gets pushed on "event send" and popped on ACK..
works fine for mouse wheel, but not for touch events, as there are all sorts of
gestures being syntehsizes / ACKd... so rather than writing
yet-another-logic-layer for keeping a consistent event stream, it's simpler to
just keep Ticking the gesture.
please let me know your thoughts if this makes sense!

2. in SmoothScroller.java / .cc, I changed the logic a bit so it has a clearer
statema chine for DOWN -> MOVING -> UP, and it no longer sends both DOWN + MOVE
or MOVE + UP at the same tick.

once you guys are happy, I'll add avi for content/ OWNERS.

thanks!

[Sami, 2013-01-10 14:36:44]

The new state machine logic looks good to me. Like we talked, I'm less sure
about the in_process_event_types_ fix so let's try to understand it better
first.

https://codereview.chromium.org/11793003/diff/10002/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11793003/diff/10002/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:38:
private final byte STATE_MOVING = 2;
Nit: any particular reason to skip 1?-)

[bulach, 2013-01-10 19:09:48]

many many thanks, sami!!!

I added far more details in crbug.com/166521, but to summarize:
when popping the ACK'd event, we need to skip other "categories" that may be in
the queue already, since the ACK can be triggered by other internal queues that
contain their on "categories" (i.e., touch_event_queue_)....

another look please?

https://codereview.chromium.org/11793003/diff/10002/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11793003/diff/10002/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:38:
private final byte STATE_MOVING = 2;
On 2013/01/10 14:36:45, Sami wrote:
> Nit: any particular reason to skip 1?-)

haha, done. :)

[Sami, 2013-01-10 19:45:34]

I like this solution. Just one question...

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1700: i !=
in_process_event_types_.end(); ++ i) {
Nit: no space between ++ and i.

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1715: ack_type ==
WebInputEvent::MouseMove ||
Why not use WebInputEvent::isMouseEventType? Because there's no separate queue
for mouse events?

[nduca, 2013-01-10 20:34:33]

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1697: bool found =
false;
Should we pull this edit out to its own cl? This seems like it should get
reviewed by the input-ack gods.

[bulach, 2013-01-11 10:42:07]

thanks for the ideas!
let's wait for https://codereview.chromium.org/11858007/

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1697: bool found =
false;
On 2013/01/10 20:34:33, nduca wrote:
> Should we pull this edit out to its own cl? This seems like it should get
> reviewed by the input-ack gods.

good idea! I split it into:
https://codereview.chromium.org/11858007/

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1700: i !=
in_process_event_types_.end(); ++ i) {
On 2013/01/10 19:45:34, Sami wrote:
> Nit: no space between ++ and i.

Done.

https://codereview.chromium.org/11793003/diff/8037/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1715: ack_type ==
WebInputEvent::MouseMove ||
On 2013/01/10 19:45:34, Sami wrote:
> Why not use WebInputEvent::isMouseEventType? Because there's no separate queue
> for mouse events?

Done.

[bulach, 2013-04-30 18:48:26]

bring our your dead! this is not a dead patch... :)

anyways: looks like the pre-requisite has finally stuck, now we need to hook it
up with java.. ptal.

sadrul: sorry, I haven't got a chance to deeply look into your patch, but one
thing you may want is to look into the "GetScrollDelta" here, I moved up to
https://codereview.chromium.org/11793003/diff/18001/content/port/browser/smoo...
so that all implementations can reuse that.

thanks!

[nduca, 2013-05-02 05:46:52]

high level lgtm. marcus, if there's anything you want me to look at in detail,
let me know. Otherwise, my thought is we should just land and then experiment
with a few traces taken with this running to sanity check that they match the
real world pretty well.

[Sami, 2013-05-02 14:41:11]

Sorry for the delay, this still lgtm.

[sadrul, 2013-05-02 14:47:41]

renderer_host/ LGTM

[bulach, 2013-05-02 14:54:46]

+jam: need OWNERS for content/, content/browser and content/port.

thanks everyone!

[jam, 2013-05-02 15:56:16]

https://codereview.chromium.org/11793003/diff/18001/content/port/browser/smoo...
File content/port/browser/smooth_scroll_gesture.h (right):

https://codereview.chromium.org/11793003/diff/18001/content/port/browser/smoo...
content/port/browser/smooth_scroll_gesture.h:32: double
GetScrollDelta(base::TimeTicks now, base::TimeDelta desired_interval);
content/port also follows the same API rules as content/public

see http://www.chromium.org/developers/content-module/content-api

in particular, see the first and last rule in that page

[bulach, 2013-05-03 10:45:24]

ahn, thanks for the pointer jam!

moved the functionality into its own class in
content/browser/renderer_host/smooth_scroll_calculator.h, not changing any
public API this time. ptal.

[jam, 2013-05-03 18:06:41]

content_browser.gypi lgtm, for the rest just have the existing owners that
reviewed this before look at the new file

[bulach, 2013-05-07 07:51:02]

thanks jam!

sadrul / nduca: quick look at
content/browser/renderer_host/smooth_scroll_calculator.* please?
delta from patchset #4 is that rather than moving that function up to
content/port, I moved "sideways" into its own class in
content/browser/renderer_host.

[sadrul, 2013-05-08 14:54:35]

SLGTM. Just nits:

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h
(right):

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h:37:
#endif // CONTENT_BROWSER_RENDERER_HOST_BASIC_MOUSE_WHEEL_SMOOTH_SCROLL_GESTURE_
two spaces before // (here and in the rest of the places in this patch)

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/smooth_scroll_calculator.h (right):

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/smooth_scroll_calculator.h:5: #ifndef
CONTENT_BROWSER_SMOOTH_SCROLL_CALCULATOR_H_
+RENDERER_HOST_

[bulach, 2013-05-08 15:19:25]

thanks! nits addressed, CQing.

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h
(right):

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h:37:
#endif // CONTENT_BROWSER_RENDERER_HOST_BASIC_MOUSE_WHEEL_SMOOTH_SCROLL_GESTURE_
On 2013/05/08 14:54:35, sadrul wrote:
> two spaces before // (here and in the rest of the places in this patch)

Done.

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/smooth_scroll_calculator.h (right):

https://codereview.chromium.org/11793003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/smooth_scroll_calculator.h:5: #ifndef
CONTENT_BROWSER_SMOOTH_SCROLL_CALCULATOR_H_
On 2013/05/08 14:54:35, sadrul wrote:
> +RENDERER_HOST_

Done.

[commit-bot: I haz the power, 2013-05-08 15:19:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11793003/42001

[nduca, 2013-05-08 15:44:39]

thanks marcus!

[commit-bot: I haz the power, 2013-05-09 04:59:17]

Change committed as 199112