[DevTools] Touch emulation in content.

[DevTools] Touch emulation in content.

When renderer requests touch emulation using mouse, host creates
a TouchEmulator object and passes mouse, mouse wheel and keyboard events
to emulator before sending them to renderer.

Emulator completely blocks mouse-related events, and instead generates
touch events. Those touch events are handled over to gesture provider
after being acked by renderer. Resulting gestures are sent to the renderer.

When shift is pressed, scroll gestures are converted to pinch gestures, so
user can scale the page. This is required because multitouch is not yet
supported by emulator.

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

[dgozman, 2014-04-04 15:58:22]

Could you please take a look?

This currently works on linux aura and mac.
Blink-side patch: https://codereview.chromium.org/145003002/.

To support pinch on mac, manual handling was added to blink. We can remove that
once event handling is enabled in compositor.

Thank you,
Dmitry

[jdduke (slow), 2014-04-04 16:11:27]

Awesome, this will be a great addition.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:38:
InitCursorFromResource(&pinch_cursor_, IDR_DEVTOOLS_PINCH_CURSOR_ICON);
I noticed that the icons would occasionally revert to their previous appearance,
I don't recall the exact circumstances but it may have been when the cursor
moved out of and back into the window bounds?

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:62: // TODO(dgozman):
Should we also send TouchCancel?
You should be able to accomplish the pinch/scroll cancellation by calling
|CancelTouch()|, but you might want to DCHECK that this terminates the
pinch/scroll.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.h (right):

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.h:25: // TODO(dgozman): maybe
pass latency info together with events.
A new LatencyInfo will be generated for the touch, and for now it's important
that the touch type is used when the LatencyInfo is initialized.

However, we may want to pass the touch's LatencyInfo into |HandleTouchEventAck|,
and use that to populate the gesture LatencyInfo... miletus@, what's the best
course of action here?

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.h:30: // Returns |true| if
the event ack was consumed.
Also note here that the consumed ack should not propagate any further (that may
sound implicit, but it can't hurt).

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.h:44: // Whether we should
convert scroll into pinches.
scroll -> scrolls

[jdduke (slow), 2014-04-04 16:21:11]

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:107: bool
TouchEmulator::HandleKeyboardEvent(const NativeWebKeyboardEvent& event) {
Let's use a "blink::WebKeyboardEvent" type here to make testing simpler.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.h (right):

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.h:21: explicit
TouchEmulator(RenderWidgetHostImpl* host);
Other than the cursor creation and the RenderWidgetHostImpl dependency, this is
very close to being quite easy to unit test.  I'd feel a lot more comfortable if
we had a few very basic tests where we feed in some mouse events and check the
output, toggle the shift modifier, etc...  Doesn't have to be anything too
complicated, more as a sanity check that can be fleshed out.

Perhaps add a TouchEmulatorClient with the RWHI dispatch methods that are
currently used?  If the WebCursor creation from resource is a problem, we could
just provide it as an argument in the constructor?

[dgozman, 2014-04-07 16:54:55]

PTAL

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:38:
InitCursorFromResource(&pinch_cursor_, IDR_DEVTOOLS_PINCH_CURSOR_ICON);
On 2014/04/04 16:11:28, jdduke wrote:
> I noticed that the icons would occasionally revert to their previous
appearance,
> I don't recall the exact circumstances but it may have been when the cursor
> moved out of and back into the window bounds? 

I've fixed that in blink, where sometimes cursor was changed while it should
not.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:62: // TODO(dgozman):
Should we also send TouchCancel?
On 2014/04/04 16:11:28, jdduke wrote:
> You should be able to accomplish the pinch/scroll cancellation by calling
> |CancelTouch()|, but you might want to DCHECK that this terminates the
> pinch/scroll.

Looks like canceling touch does not initiate a synchronous callback chain. Thus,
emulator is already gone when we get any gesture end event.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:107: bool
TouchEmulator::HandleKeyboardEvent(const NativeWebKeyboardEvent& event) {
On 2014/04/04 16:21:11, jdduke wrote:
> Let's use a "blink::WebKeyboardEvent" type here to make testing simpler.

Done.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.h (right):

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.h:21: explicit
TouchEmulator(RenderWidgetHostImpl* host);
On 2014/04/04 16:21:11, jdduke wrote:
> Other than the cursor creation and the RenderWidgetHostImpl dependency, this
is
> very close to being quite easy to unit test.  I'd feel a lot more comfortable
if
> we had a few very basic tests where we feed in some mouse events and check the
> output, toggle the shift modifier, etc...  Doesn't have to be anything too
> complicated, more as a sanity check that can be fleshed out.
> 
> Perhaps add a TouchEmulatorClient with the RWHI dispatch methods that are
> currently used?  If the WebCursor creation from resource is a problem, we
could
> just provide it as an argument in the constructor?

Done.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.h:30: // Returns |true| if
the event ack was consumed.
On 2014/04/04 16:11:28, jdduke wrote:
> Also note here that the consumed ack should not propagate any further (that
may
> sound implicit, but it can't hurt).

Done.

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.h:44: // Whether we should
convert scroll into pinches.
On 2014/04/04 16:11:28, jdduke wrote:
> scroll -> scrolls

Done.

[pfeldman, 2014-04-07 17:30:09]

overall lgtm,
devtools lgtm

This still needs to be blessed by input people.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:77: void
TouchEmulator::InitCursorFromResource(WebCursor* cursor, int resource_id) {
Is there anything you could reuse here?

[jdduke (slow), 2014-04-07 18:07:26]

Adding tdresser@ who's working on finishing touches for the Aura GR.  tdresser@:
Could you take a glance at TouchEmulator and see if there's anything we might be
missing?

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:62: // TODO(dgozman):
Should we also send TouchCancel?
On 2014/04/07 16:54:55, dgozman wrote:
> On 2014/04/04 16:11:28, jdduke wrote:
> > You should be able to accomplish the pinch/scroll cancellation by calling
> > |CancelTouch()|, but you might want to DCHECK that this terminates the
> > pinch/scroll.
> 
> Looks like canceling touch does not initiate a synchronous callback chain.
Thus,
> emulator is already gone when we get any gesture end event.

It should, but only if there's an active touch (mouse) sequence.  We should
check to see why you're not getting a TouchCancel synchronously when
|touch_active_| is already true.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:28:
gesture_provider_(ui::DefaultGestureProviderConfig(), this) {
We'll need to modify the default config slightly, now that
https://codereview.chromium.org/223673006 has landed.  In short, just add a
helper function like:

ui::GestureProvider::Config GetGestureProviderConfig() {
  ui::GestureProvider::Config config = ui::DefaultGestureProviderConfig();
  config.gesture_begin_end_types_enabled = false;
  return config;
}

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:87: void
DestroyEmulator() {
Nit: This isn't used.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:97: case
WebInputEvent::TouchStart:
You can use |WebInputEventTraits::GetName(type)| in
content/common/input/web_input_event_traits.h.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:226: Expect("");
Will |EXPECT_EQ(ExpectedEvents(), "")| not work? I ask because, using the
current |Expect("")|, will you not lose the calling line # if there's an error?
(same with the rest of the Expect calls)

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:277: "
SCROLL_BEGIN SCROLL_UPDATE TOUCH_MOVE PINCH_BEGIN");
Hmm, did you want to add some more code here?

[jdduke (slow), 2014-04-07 18:12:36]

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_client.h (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_client.h:21: virtual void
ForwardTouchEventWithLatencyInfo(
Let's call this |ForwardTouchEvent()|, and add a new method to RWHI that
provides the empty ui::LatencyInfo (the same as ForwardGestureEvent).

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:305: virtual void
ForwardGestureEvent(
It's OK to re-order the TouchEmulatorClient specific methods into a single
grouping here, particularly if you add the method |ForwardTouchEvent| that does
the same logical thing as |ForwardGestureEvent|.

[tdresser, 2014-04-07 18:43:54]

On 2014/04/07 18:07:26, jdduke wrote:
> Adding tdresser@ who's working on finishing touches for the Aura GR. 
tdresser@:
> Could you take a glance at TouchEmulator and see if there's anything we might
be
> missing?
> 
>
https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_emulator.cc (right):
> 
>
https://codereview.chromium.org/138163016/diff/260001/content/browser/rendere...
> content/browser/renderer_host/input/touch_emulator.cc:62: // TODO(dgozman):
> Should we also send TouchCancel?
> On 2014/04/07 16:54:55, dgozman wrote:
> > On 2014/04/04 16:11:28, jdduke wrote:
> > > You should be able to accomplish the pinch/scroll cancellation by calling
> > > |CancelTouch()|, but you might want to DCHECK that this terminates the
> > > pinch/scroll.
> > 
> > Looks like canceling touch does not initiate a synchronous callback chain.
> Thus,
> > emulator is already gone when we get any gesture end event.
> 
> It should, but only if there's an active touch (mouse) sequence.  We should
> check to see why you're not getting a TouchCancel synchronously when
> |touch_active_| is already true.
> 
>
https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_emulator.cc (right):
> 
>
https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
> content/browser/renderer_host/input/touch_emulator.cc:28:
> gesture_provider_(ui::DefaultGestureProviderConfig(), this) {
> We'll need to modify the default config slightly, now that
> https://codereview.chromium.org/223673006 has landed.  In short, just add a
> helper function like:
> 
> ui::GestureProvider::Config GetGestureProviderConfig() {
>   ui::GestureProvider::Config config = ui::DefaultGestureProviderConfig();
>   config.gesture_begin_end_types_enabled = false;
>   return config;
> }
> 
>
https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):
> 
>
https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
> content/browser/renderer_host/input/touch_emulator_unittest.cc:87: void
> DestroyEmulator() {
> Nit: This isn't used.
> 
>
https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
> content/browser/renderer_host/input/touch_emulator_unittest.cc:97: case
> WebInputEvent::TouchStart:
> You can use |WebInputEventTraits::GetName(type)| in
> content/common/input/web_input_event_traits.h.
> 
>
https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
> content/browser/renderer_host/input/touch_emulator_unittest.cc:226:
Expect("");
> Will |EXPECT_EQ(ExpectedEvents(), "")| not work? I ask because, using the
> current |Expect("")|, will you not lose the calling line # if there's an
error?
> (same with the rest of the Expect calls)
> 
>
https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
> content/browser/renderer_host/input/touch_emulator_unittest.cc:277: "
> SCROLL_BEGIN SCROLL_UPDATE TOUCH_MOVE PINCH_BEGIN");
> Hmm, did you want to add some more code here?

content/browser/renderer_host/input/touch_emulator* LGTM.

I'd like to see an integration test or two though - there's a bunch of untested
logic here. Perhaps something in |render_widget_host_unittest| that would sanity
check the glue code in RWHI?

[tdresser, 2014-04-07 18:44:04]

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:221: 
I'd be more comfortable if these tests looked at gesture event attributes other
than just the event type.

Ensuring that the scroll deltas are sane, for instance, would be a worthwhile
test.

[dgozman, 2014-04-08 16:56:48]

PTAL

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:28:
gesture_provider_(ui::DefaultGestureProviderConfig(), this) {
On 2014/04/07 18:07:26, jdduke wrote:
> We'll need to modify the default config slightly, now that
> https://codereview.chromium.org/223673006 has landed.  In short, just add a
> helper function like:
> 
> ui::GestureProvider::Config GetGestureProviderConfig() {
>   ui::GestureProvider::Config config = ui::DefaultGestureProviderConfig();
>   config.gesture_begin_end_types_enabled = false;
>   return config;
> }

Done.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:77: void
TouchEmulator::InitCursorFromResource(WebCursor* cursor, int resource_id) {
On 2014/04/07 17:30:09, pfeldman wrote:
> Is there anything you could reuse here?

Didn't find anything.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_client.h (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_client.h:21: virtual void
ForwardTouchEventWithLatencyInfo(
On 2014/04/07 18:12:37, jdduke wrote:
> Let's call this |ForwardTouchEvent()|, and add a new method to RWHI that
> provides the empty ui::LatencyInfo (the same as ForwardGestureEvent).

Done.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:87: void
DestroyEmulator() {
On 2014/04/07 18:07:26, jdduke wrote:
> Nit: This isn't used.

Done.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:97: case
WebInputEvent::TouchStart:
On 2014/04/07 18:07:26, jdduke wrote:
> You can use |WebInputEventTraits::GetName(type)| in
> content/common/input/web_input_event_traits.h.

Nice! Done.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:221: 
On 2014/04/07 18:44:04, tdresser wrote:
> I'd be more comfortable if these tests looked at gesture event attributes
other
> than just the event type.
> 
> Ensuring that the scroll deltas are sane, for instance, would be a worthwhile
> test.

I'm not sure how to test this. Looks like properly checking gesture details does
not belong to emulator test.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:226: Expect("");
On 2014/04/07 18:07:26, jdduke wrote:
> Will |EXPECT_EQ(ExpectedEvents(), "")| not work? I ask because, using the
> current |Expect("")|, will you not lose the calling line # if there's an
error?
> (same with the rest of the Expect calls)

You are right, fixed.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:277: "
SCROLL_BEGIN SCROLL_UPDATE TOUCH_MOVE PINCH_BEGIN");
On 2014/04/07 18:07:26, jdduke wrote:
> Hmm, did you want to add some more code here?

At first, I planned to call DestroyEmulator here. But now, I just check for
expected PinchEnd and ScrollEnd in each test.
So, this test just destroys emulator while mouse is down.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:305: virtual void
ForwardGestureEvent(
On 2014/04/07 18:12:37, jdduke wrote:
> It's OK to re-order the TouchEmulatorClient specific methods into a single
> grouping here, particularly if you add the method |ForwardTouchEvent| that
does
> the same logical thing as |ForwardGestureEvent|.

Done.

[tdresser, 2014-04-08 18:39:15]

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:221: 
On 2014/04/08 16:56:49, dgozman wrote:
> On 2014/04/07 18:44:04, tdresser wrote:
> > I'd be more comfortable if these tests looked at gesture event attributes
> other
> > than just the event type.
> > 
> > Ensuring that the scroll deltas are sane, for instance, would be a
worthwhile
> > test.
> 
> I'm not sure how to test this. Looks like properly checking gesture details
does
> not belong to emulator test.

ForwardGestureEvent could record the most recent WebGestureEvent's metadata, and
then you could assert on it throughout the test.

For instance (in ForwardGestureEvent):
if (event.type == WebInputEvent::GestureScrollUpdate) {
       scroll_x_ = event.data.scrollUpdate.deltaX;
       scroll_y_ = event.data.scrollUpdate.deltaY;
}

And then assert on the value of scroll_x and scroll_y during the tests.

[jdduke (slow), 2014-04-08 19:14:10]

content/browser/renderer_host/input lgtm with a few nits.  The main change I'd
like to see is a switch, or clearer logic flow, in TouchEmulator::OnGestureEvent
(that's probably my fault, as I think I wrote a good chunk of that initially). 
Comments inline.

Thanks! This is going to be a great addition.

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:221: 
On 2014/04/08 18:39:15, tdresser wrote:
> On 2014/04/08 16:56:49, dgozman wrote:
> > On 2014/04/07 18:44:04, tdresser wrote:
> > > I'd be more comfortable if these tests looked at gesture event attributes
> > other
> > > than just the event type.
> > > 
> > > Ensuring that the scroll deltas are sane, for instance, would be a
> worthwhile
> > > test.
> > 
> > I'm not sure how to test this. Looks like properly checking gesture details
> does
> > not belong to emulator test.
> 
> ForwardGestureEvent could record the most recent WebGestureEvent's metadata,
and
> then you could assert on it throughout the test.
> 
> For instance (in ForwardGestureEvent):
> if (event.type == WebInputEvent::GestureScrollUpdate) {
>        scroll_x_ = event.data.scrollUpdate.deltaX;
>        scroll_y_ = event.data.scrollUpdate.deltaY;
> }
> 
> And then assert on the value of scroll_x and scroll_y during the tests.

I think event orderings are more important than checking precise gesture values
here, as we already have coverage for gesture detection output.  It might be
worthwhile to check the generated *touch* events just to verify they always have
the same coordinates as the mouse.

https://codereview.chromium.org/138163016/diff/320001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/320001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:36: } // namespace
Nit: 2 spaces before //

https://codereview.chromium.org/138163016/diff/320001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:45:
ui::GestureProvider::Config gesture_config)
Nit: While the Config is fairly simple, we should probably pass by const ref.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:77: if (enabled_) {
Actually, the safest thing here would be to do nothing, and put a comment that
the owner is responsible for any event cleanup.  The reason is, we only destroy
the emulator if the renderer has crashed, so there's no need for cleanup.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:139: if (mouse_event.type
== WebInputEvent::MouseUp)
Nit: else if

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:195: // Convert scrolls to
pinches while shift is pressed.
I wonder if this method would be easier to follow with a switch
(gesture_event.type), in which the final statement is
|client_->ForwardGestureEvent()| and we return early where necessary.  It took
me a while to find that line =/.  Just a thought.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:138:
emulator()->HandleMouseEvent(event);
One cheap addition here would be recording the touch coordinates in
|ForwardTouchEvent|, and verifying CHECK_EQ(last_mouse_x_, last_touch_x_), same
for y, after calling |HandleMouseEvent|.  As long as those look good, I don't
think we need to check gesture coordinates.  (You may need to verify a touch was
actually forward before performing the check, if you do in fact test Disable()
behavior.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:240:
TEST_F(TouchEmulatorTest, DestroyWhilePinch) {
Maybe change this test to a very basic enable/disable test, wherein disabling
the emulator prevents touches from being forwarded, enabling resumes.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:2412:
host_->DisableGestureDebounce();
Nice.

[tdresser, 2014-04-08 19:21:23]

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/300001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:221: 
On 2014/04/08 19:14:11, jdduke wrote:
> On 2014/04/08 18:39:15, tdresser wrote:
> > On 2014/04/08 16:56:49, dgozman wrote:
> > > On 2014/04/07 18:44:04, tdresser wrote:
> > > > I'd be more comfortable if these tests looked at gesture event
attributes
> > > other
> > > > than just the event type.
> > > > 
> > > > Ensuring that the scroll deltas are sane, for instance, would be a
> > worthwhile
> > > > test.
> > > 
> > > I'm not sure how to test this. Looks like properly checking gesture
details
> > does
> > > not belong to emulator test.
> > 
> > ForwardGestureEvent could record the most recent WebGestureEvent's metadata,
> and
> > then you could assert on it throughout the test.
> > 
> > For instance (in ForwardGestureEvent):
> > if (event.type == WebInputEvent::GestureScrollUpdate) {
> >        scroll_x_ = event.data.scrollUpdate.deltaX;
> >        scroll_y_ = event.data.scrollUpdate.deltaY;
> > }
> > 
> > And then assert on the value of scroll_x and scroll_y during the tests.
> 
> I think event orderings are more important than checking precise gesture
values
> here, as we already have coverage for gesture detection output.  It might be
> worthwhile to check the generated *touch* events just to verify they always
have
> the same coordinates as the mouse.

Good call - I'd be perfectly happy with a single test which ensures the
generated touch event co-ordinates are reasonable.

[dgozman, 2014-04-09 17:00:05]

PTAL

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:77: if (enabled_) {
On 2014/04/08 19:14:11, jdduke wrote:
> Actually, the safest thing here would be to do nothing, and put a comment that
> the owner is responsible for any event cleanup.  The reason is, we only
destroy
> the emulator if the renderer has crashed, so there's no need for cleanup.

Sounds reasonable. Done.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:139: if (mouse_event.type
== WebInputEvent::MouseUp)
On 2014/04/08 19:14:11, jdduke wrote:
> Nit: else if

Done.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:195: // Convert scrolls to
pinches while shift is pressed.
On 2014/04/08 19:14:11, jdduke wrote:
> I wonder if this method would be easier to follow with a switch
> (gesture_event.type), in which the final statement is
> |client_->ForwardGestureEvent()| and we return early where necessary.  It took
> me a while to find that line =/.  Just a thought.

Switch looks better. However, we still need different places for
|client_->ForwardGestureEvent()| as the logic is somewhat complex.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:138:
emulator()->HandleMouseEvent(event);
On 2014/04/08 19:14:11, jdduke wrote:
> One cheap addition here would be recording the touch coordinates in
> |ForwardTouchEvent|, and verifying CHECK_EQ(last_mouse_x_, last_touch_x_),
same
> for y, after calling |HandleMouseEvent|.  As long as those look good, I don't
> think we need to check gesture coordinates.  (You may need to verify a touch
was
> actually forward before performing the check, if you do in fact test Disable()
> behavior.

I've added the similar check to |ForwardTouchEvent| instead. We cannot predict
from here whether touch event will be forwarded (since gesture provider may
block it or touch emulator may be disabled). So, checking instead that every
touch has the last mouse position coordinates seems fine.

https://codereview.chromium.org/138163016/diff/360001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator_unittest.cc:240:
TEST_F(TouchEmulatorTest, DestroyWhilePinch) {
On 2014/04/08 19:14:11, jdduke wrote:
> Maybe change this test to a very basic enable/disable test, wherein disabling
> the emulator prevents touches from being forwarded, enabling resumes.

I previously assumed that emulator destructor (and thus |TearDown|) will clean
up, but now we have Disable, so this test is unnecessary. I've added
DisableAndReenable instead.

[jdduke (slow), 2014-04-10 15:58:30]

https://codereview.chromium.org/138163016/diff/410001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/410001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:247:
touch_event_.timeStampSeconds = base::Time::Now().ToDoubleT();
We should use base::TimeTicks::Now(): (base::TimeTicks::Now() -
base::TimeTicks()).InSecondsF();

[dgozman, 2014-04-10 16:33:28]

Please take another look.

The only issues here I know about are:
- feature requests in various TODOs;
- mouse move dropping logic is not tested in real situation;
- compositor pinch gesture handling differs from blink-side handling used on
mac.

https://codereview.chromium.org/138163016/diff/410001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/138163016/diff/410001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:247:
touch_event_.timeStampSeconds = base::Time::Now().ToDoubleT();
On 2014/04/10 15:58:31, jdduke wrote:
> We should use base::TimeTicks::Now(): (base::TimeTicks::Now() -
> base::TimeTicks()).InSecondsF();

Done.

[jdduke (slow), 2014-04-10 16:49:46]

content/renderer/host/input lgtm, with one nit for the RenderWidgetHostTest.

https://codereview.chromium.org/138163016/diff/430001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/430001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:266: virtual void
OnTouchEventAck(
Is there a reason you didn't re-use the touch ack bookkeeping provided by
TestView below? It exposes |acked_event()| which should be equivalent, so
|view_->acked_event().type| should be what you want (I could be wrong).

[dgozman, 2014-04-10 17:38:03]

Thank you for review!

https://codereview.chromium.org/138163016/diff/430001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/430001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:266: virtual void
OnTouchEventAck(
On 2014/04/10 16:49:46, jdduke wrote:
> Is there a reason you didn't re-use the touch ack bookkeeping provided by
> TestView below? It exposes |acked_event()| which should be equivalent, so
> |view_->acked_event().type| should be what you want (I could be wrong).

Touch emulator handles ack before view and consumes it.

[jdduke (slow), 2014-04-10 17:40:21]

https://codereview.chromium.org/138163016/diff/430001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/138163016/diff/430001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:266: virtual void
OnTouchEventAck(
On 2014/04/10 17:38:03, dgozman wrote:
> On 2014/04/10 16:49:46, jdduke wrote:
> > Is there a reason you didn't re-use the touch ack bookkeeping provided by
> > TestView below? It exposes |acked_event()| which should be equivalent, so
> > |view_->acked_event().type| should be what you want (I could be wrong).
> 
> Touch emulator handles ack before view and consumes it.

Of course, silly me! Still lgtm :)

[dgozman, 2014-04-10 17:42:25]

Hello owners,

Could you please take a look:
- tsepez@ at content/common/view_messages.h;
- jochen@ at content overall, especially at render_widget_host*, gyp files and
resources.

Thank you,
Dmitry

[Tom Sepez, 2014-04-10 18:14:29]

Messages LGTM.

[jochen (gone - plz use gerrit), 2014-04-14 07:48:09]

lgtm

[dgozman, 2014-04-14 14:49:39]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-04-14 14:49:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/138163016/470001

[commit-bot: I haz the power, 2014-04-14 16:47:16]

Change committed as 263644