Replace old with new synthetic gesture framework.

Replace old with new synthetic gesture framework.

- Remove current implementation of synthetic gestures.
- Rename temporarily named classes and files of new synthetic gesture framework
  (e.g. SyntheticGestureNew -> SyntheticGesture).
- Link up new implementation in browser and renderer.
- Hook up synthetic gesture controller to input flushing in RWHI.
- Update JavaScript front-end and Telemetry.

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

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

[Dominik Grewe, 2013-11-18 16:07:13]

PTAL. The patch is quite big, but the majority of changes are to do with
removing the old framework and renaming classes/files with the New/_new suffix.

The files containing the interesting changes are:

synthetic_gesture_target_base.cc
render_widget_host_impl.h/cc
input_messages.h
gpu_benchmarking_extensions.cc
render_widget.h/cc
scroll.js/py

[jdduke (slow), 2013-11-18 16:32:55]

Looking good. I think my main question is if the gesture controller would fit
better in the ImmediateInputRouter.  However, as it doesn't interact strongly
with the other router types or interfaces, I think it could live either place
just fine, so I'll leave that up to you.

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:25: :
host_(host) {

Could you add a DCHECK(host) here?

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:402: if
(synthetic_gesture_controller_)

So, I don't have a strong opinion whether the controller lives here or in the
router.  It seems like it would fit nicely in the router, you would just add a
InputRouterClient method that creates the target.

The advantage there is that the router handles all InputHostMsg types, and you
can isolate testing somewhat better than if it lives on the RWHI. Up to you.

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:1720: LOG(WARNING) <<

Let's remove the warning and just early return.

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:1726:
view_->CreateSyntheticGestureTarget().Pass()));

Hmm, I don't think you need the Pass(), do you? It should already be an
rvalue... I could be wrong though, I'm not intimately familiar with the
differences between scoped_ptr and unique_ptr.

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.h:28: #include
"content/browser/renderer_host/input/synthetic_gesture_controller.h"

You can forward declare SyntheticGestureController, and include this in the .cc.

[Dominik Grewe, 2013-11-18 17:48:05]

I think I'd rather keep the gesture controller in RWHI for now. As you said,
interaction with the router is limited and I don't think different routers would
treat the gesture controller differently, would they?

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:25: :
host_(host) {
On 2013/11/18 16:32:55, jdduke wrote:
> 
> Could you add a DCHECK(host) here?

Done.

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:1720: LOG(WARNING) <<
On 2013/11/18 16:32:55, jdduke wrote:
> 
> Let's remove the warning and just early return.

Done.

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:1726:
view_->CreateSyntheticGestureTarget().Pass()));
On 2013/11/18 16:32:55, jdduke wrote:
> 
> Hmm, I don't think you need the Pass(), do you? It should already be an
> rvalue... I could be wrong though, I'm not intimately familiar with the
> differences between scoped_ptr and unique_ptr.

Yeah, you're right.

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.h:28: #include
"content/browser/renderer_host/input/synthetic_gesture_controller.h"
On 2013/11/18 16:32:55, jdduke wrote:
> 
> You can forward declare SyntheticGestureController, and include this in the
.cc.

scoped_ptr<> doesn't like incomplete types. If I don't include the file here the
compiler complains.

[jdduke (slow), 2013-11-18 17:55:23]

On 2013/11/18 17:48:05, Dominik Grewe wrote:
> I think I'd rather keep the gesture controller in RWHI for now. As you said,
> interaction with the router is limited and I don't think different routers
would
> treat the gesture controller differently, would they?

That's fine.  The original plan was to have multiple router implementations, but
that's since been abandoned, and we'll be renaming the current router to
"InputRouterImpl" to prevent any further confusion.

[jdduke (slow), 2013-11-18 17:58:44]

content/browser/renderer_host/input lgtm.

https://codereview.chromium.org/62443007/diff/80001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/62443007/diff/80001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1719: if (!view_)

Do you think it's worth early returning at the beginning of the function if
|view_| is null? Would we ever want to queue a gesture if the |view_| has become
null? Not sure here.

[Dominik Grewe, 2013-11-18 18:09:40]

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/62443007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.h:28: #include
"content/browser/renderer_host/input/synthetic_gesture_controller.h"
On 2013/11/18 17:48:05, Dominik Grewe wrote:
> On 2013/11/18 16:32:55, jdduke wrote:
> > 
> > You can forward declare SyntheticGestureController, and include this in the
> .cc.
> 
> scoped_ptr<> doesn't like incomplete types. If I don't include the file here
the
> compiler complains.

I was wrong. This is now fixed in ps3.

https://codereview.chromium.org/62443007/diff/80001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/62443007/diff/80001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1719: if (!view_)
On 2013/11/18 17:58:44, jdduke wrote:
> 
> Do you think it's worth early returning at the beginning of the function if
> |view_| is null? Would we ever want to queue a gesture if the |view_| has
become
> null? Not sure here.

We shouldn't have a synthetic gesture controller if |view_| is null. Though I'm
not entirely sure if that's guaranteed to be the case.

[Dominik Grewe, 2013-11-18 18:27:34]

Adding more owners. Please look at the following files. Thanks!

bulach: content/public/android/* & content/browser/android/* & tools/telemetry/*
piman: content/* & content/renderer/gpu/* content/port/browser/*
kenrb: content/common/*_messages.h
sadrul: content/browser/renderer_host/*

As I said in an earlier comment: While the patch is quite large, it contains a
lot of renaming and removed files.

[sadrul, 2013-11-19 10:02:22]

lgtm

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:316: view_ =
RenderWidgetHostViewPort::FromRWHV(view);
Should you reset synthetic_gesture_controller_ here too?

[Dominik Grewe, 2013-11-19 11:32:41]

Thanks, Sadrul!

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:316: view_ =
RenderWidgetHostViewPort::FromRWHV(view);
On 2013/11/19 10:02:23, sadrul wrote:
> Should you reset synthetic_gesture_controller_ here too?

Yes, that's a good point. We don't want a gesture controller that uses an
out-of-date view. I'm not sure if it's strictly necessary, i.e. if the view will
ever be set after a gesture has been queued and before RendererExited is called,
but I'll reset it just to be sure.

[kenrb, 2013-11-19 15:43:07]

lgtm with a caveat. If my observation about type casting is correct then you
could either address it here or I could file a bug, whatever you prefer.

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture.cc (right):

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture.cc:34: return
CreateGesture<SyntheticPinchGesture,
I realize this isn't specific to your patch, but I have a question about how
SyntheticGestureParams get cast to their subtypes. They self-identify which type
they are, but I can't see any way by which we guarantee that have the correct
size. I'm concerned that a renderer process passing these messages could supply
too little data, have it's params cast to a gesture type, and then use this as a
memory leak primitive.

[Dominik Grewe, 2013-11-19 17:10:27]

Thanks for the feedback, Ken! I'm happy to address this issue in this CL if it's
not a major change. But I don't see a way to fix it (see my comment below).

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture.cc (right):

https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture.cc:34: return
CreateGesture<SyntheticPinchGesture,
On 2013/11/19 15:43:08, kenrb wrote:
> I realize this isn't specific to your patch, but I have a question about how
> SyntheticGestureParams get cast to their subtypes. They self-identify which
type
> they are, but I can't see any way by which we guarantee that have the correct
> size. I'm concerned that a renderer process passing these messages could
supply
> too little data, have it's params cast to a gesture type, and then use this as
a
> memory leak primitive.

How could we check the size of the data? The message we send contains a
SyntheticGesturePacket which points to the SyntheticGestureParams. So we'd have
to check the size of that object pointed to by the packet, rather than the
message itself, right? How can we do that at runtime?

[jdduke (slow), 2013-11-19 18:48:11]

On 2013/11/19 17:10:27, Dominik Grewe wrote:
> Thanks for the feedback, Ken! I'm happy to address this issue in this CL if
it's
> not a major change. But I don't see a way to fix it (see my comment below).
> 
>
https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
> File content/browser/renderer_host/input/synthetic_gesture.cc (right):
> 
>
https://codereview.chromium.org/62443007/diff/160001/content/browser/renderer...
> content/browser/renderer_host/input/synthetic_gesture.cc:34: return
> CreateGesture<SyntheticPinchGesture,
> On 2013/11/19 15:43:08, kenrb wrote:
> > I realize this isn't specific to your patch, but I have a question about how
> > SyntheticGestureParams get cast to their subtypes. They self-identify which
> type
> > they are, but I can't see any way by which we guarantee that have the
correct
> > size. I'm concerned that a renderer process passing these messages could
> supply
> > too little data, have it's params cast to a gesture type, and then use this
as
> a
> > memory leak primitive.
> 
> How could we check the size of the data? The message we send contains a
> SyntheticGesturePacket which points to the SyntheticGestureParams. So we'd
have
> to check the size of that object pointed to by the packet, rather than the
> message itself, right? How can we do that at runtime?

The browser should only be using SyntheticGestureParam derivates created via the
deserialization code in input_param_traits.cc.  There we validate the supplied
data from the renderer, and create the appropriate type, after which point there
shouldn't be a way for the renderer to fiddle with the type.  If we were just
casting a raw buffer then I think we'd have a problem, as you say.

[kenrb, 2013-11-19 20:10:05]

On 2013/11/19 18:48:11, jdduke wrote:
> The browser should only be using SyntheticGestureParam derivates created via
the
> deserialization code in input_param_traits.cc.  There we validate the supplied
> data from the renderer, and create the appropriate type, after which point
there
> shouldn't be a way for the renderer to fiddle with the type.  If we were just
> casting a raw buffer then I think we'd have a problem, as you say.

Thanks, I missed the template specializations there. I didn't see a place where
it was checking the event subtype before deserializing, but you are right that
the SyntheticGesturePacket Read does it.

No further concerns with this code.

[piman, 2013-11-19 20:45:17]

LGTM + couple of nits.

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
File
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
(right):

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:30:
MockSyntheticGesture(bool *finished, int num_steps)
nit: star on the left (as it was)

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:83: //
Ignoring the result for now.
nit: is that a TODO / is there a bug? Otherwise, just remove the comment which
doesn't add anything.

[Dominik Grewe, 2013-11-20 10:16:31]

Thanks for the feedback, piman and kenrb. And thanks for the clarification
jdduke!

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
File
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
(right):

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:30:
MockSyntheticGesture(bool *finished, int num_steps)
On 2013/11/19 20:45:18, piman wrote:
> nit: star on the left (as it was)

Done.

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/62443007/diff/330001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:83: //
Ignoring the result for now.
On 2013/11/19 20:45:18, piman wrote:
> nit: is that a TODO / is there a bug? Otherwise, just remove the comment which
> doesn't add anything.

We currently don't plan to use the result but may decide to do so in future. I'm
removing the comment for now.

[bulach, 2013-11-20 12:06:16]

good stuff!!! overall lgtm, but some important comments below about running
telemetry against older versions... this needs to be addressed in this patch,
but hopefully it'll not be too complicated.

https://codereview.chromium.org/62443007/diff/400001/content/renderer/gpu/gpu...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/62443007/diff/400001/content/renderer/gpu/gpu...
content/renderer/gpu/gpu_benchmarking_extension.cc:277: "   
function(pixels_to_scroll, opt_callback, opt_mouse_event_x,"
nit: perhaps s/opt_mouse_event/opt_event/ ?

https://codereview.chromium.org/62443007/diff/400001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/62443007/diff/400001/content/renderer/render_...
content/renderer/render_widget.cc:2007: scoped_ptr<SyntheticGestureParams>
gesture_params,
does this need to be a ptr? I think such params struct are normally just passed
by value..

https://codereview.chromium.org/62443007/diff/400001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/scroll.js (right):

https://codereview.chromium.org/62443007/diff/400001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/scroll.js:24: this.gesture_source_type =
chrome.gpuBenchmarking.DEFAULT_INPUT;
hmm, we run this same file against the "reference" build and older versions of
chrome...
chrome.gpuBenchmarking.DEFAULT_INPUT and the new function params may not be
available, so you need to polyfill it..
make sure there's a TODO and a bug to remove in say ~6 weeks time once this
patch rolls into stable..

https://codereview.chromium.org/62443007/diff/400001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/scroll.py (right):

https://codereview.chromium.org/62443007/diff/400001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/scroll.py:51: gesture_source_type =
'chrome.gpuBenchmarking.DEFAULT_INPUT'
as above, this needs to be guarded..
easy way to check is by running the benchmark against --browser=system..

[Dominik Grewe, 2013-11-20 13:20:35]

Thanks Marcus! New patch uploaded.

https://codereview.chromium.org/62443007/diff/400001/content/renderer/gpu/gpu...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/62443007/diff/400001/content/renderer/gpu/gpu...
content/renderer/gpu/gpu_benchmarking_extension.cc:277: "   
function(pixels_to_scroll, opt_callback, opt_mouse_event_x,"
On 2013/11/20 12:06:17, bulach wrote:
> nit: perhaps s/opt_mouse_event/opt_event/ ?

Good point! We're not necessarily using mouse input. How about opt_start_x/y to
make it clearer?

https://codereview.chromium.org/62443007/diff/400001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/62443007/diff/400001/content/renderer/render_...
content/renderer/render_widget.cc:2007: scoped_ptr<SyntheticGestureParams>
gesture_params,
On 2013/11/20 12:06:17, bulach wrote:
> does this need to be a ptr? I think such params struct are normally just
passed
> by value..

SyntheticGestureParams is just an abstract type. We want to be able to pass
arbitrary subtypes, that's why we're using a pointer here.

https://codereview.chromium.org/62443007/diff/400001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/scroll.js (right):

https://codereview.chromium.org/62443007/diff/400001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/scroll.js:24: this.gesture_source_type =
chrome.gpuBenchmarking.DEFAULT_INPUT;
On 2013/11/20 12:06:17, bulach wrote:
> hmm, we run this same file against the "reference" build and older versions of
> chrome...
> chrome.gpuBenchmarking.DEFAULT_INPUT and the new function params may not be
> available, so you need to polyfill it..
> make sure there's a TODO and a bug to remove in say ~6 weeks time once this
> patch rolls into stable..

So it turns out we don't really need to do this. JS is "flexible" enough to cope
with undefined variables and additional parameters.

[bulach, 2013-11-20 13:22:55]

lgtm, thanks for checking the new telemetry will work on old browsers!

[commit-bot: I haz the power, 2013-11-20 13:23:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/62443007/530001

[commit-bot: I haz the power, 2013-11-20 16:20:22]

Change committed as 236254

[Dominik Grewe, 2013-11-21 17:45:29]

This patch got reverted yesterday because of some bot failures. It turned out,
however, that the failures were due to some other CL (see comments in
https://codereview.chromium.org/79143002/)

I'll re-commit this patch.

[commit-bot: I haz the power, 2013-11-21 17:45:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/62443007/530001

[commit-bot: I haz the power, 2013-11-21 17:47:39]

Change committed as 236545