Pass gesture detector config type when enabling touch emulator.

Pass gesture detector config type when enabling touch emulator.

BUG=425586
Committed: https://crrev.com/7db626dc977e1c0171251431ab5d718048a8627e
Cr-Commit-Position: refs/heads/master@{#312144}

[dgozman, 2014-12-23 15:06:48]

dgozman@chromium.org changed reviewers:
+ jdduke@chromium.org, rbyers@chromium.org

[dgozman, 2014-12-23 15:06:48]

Could you please take a look? This is a follow-up to
https://codereview.chromium.org/820053002/.

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_emulator_unittest.cc:389:
TEST_F(TouchEmulatorTest, DisableAndReenableDifferentConfig) {
I've added this test just as a sanity check. Not sure how to test the difference
in configs. If you have any ideas, please suggest something.

[jdduke (slow), 2014-12-30 16:59:50]

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_emulator_unittest.cc:509:
emulator()->Enable(ui::GestureProviderConfigType::GENERIC_MOBILE);
Can we try GENERIC_DESKTOP somewhere?

[dgozman, 2014-12-30 18:05:45]

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_emulator_unittest.cc:509:
emulator()->Enable(ui::GestureProviderConfigType::GENERIC_MOBILE);
On 2014/12/30 16:59:50, jdduke wrote:
> Can we try GENERIC_DESKTOP somewhere?

It is used in the new DisableAndReenableDifferentConfig test above. Not sure how
to measure the difference though.

[jdduke (slow), 2014-12-30 18:13:01]

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_emulator.cc:87:
gesture_provider_.reset(new ui::FilteredGestureProvider(
I'm a little concerned about repeated enables (without an intervening
Disable()), either of the same type or of different types, particularly when
there's an active touch sequence.

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_emulator_unittest.cc:509:
emulator()->Enable(ui::GestureProviderConfigType::GENERIC_MOBILE);
On 2014/12/30 18:05:44, dgozman wrote:
> On 2014/12/30 16:59:50, jdduke wrote:
> > Can we try GENERIC_DESKTOP somewhere?
> 
> It is used in the new DisableAndReenableDifferentConfig test above. Not sure
how
> to measure the difference though.

Ah, I missed that. No need to measure differences, just want to make sure we
have a smoke test for the two config types.

[Rick Byers, 2015-01-05 18:31:41]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[Rick Byers, 2015-01-05 18:31:57]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-01-05 18:31:58]

Moving myself to CC - Jared is really the best reviewer here.

[dgozman, 2015-01-14 14:00:23]

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_emulator.cc:87:
gesture_provider_.reset(new ui::FilteredGestureProvider(
On 2014/12/30 18:13:00, jdduke wrote:
> I'm a little concerned about repeated enables (without an intervening
> Disable()), either of the same type or of different types, particularly when
> there's an active touch sequence. 

Yeah, but what else can we do?
With this code, renderer could get a broken gesture sequence (i.e. pinch start
with pinch end), but all touches should be in correct order. Is this a problem?

[jdduke (slow), 2015-01-14 15:54:17]

lgtm

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_emulator_unittest.cc:389:
TEST_F(TouchEmulatorTest, DisableAndReenableDifferentConfig) {
On 2014/12/23 15:06:48, dgozman wrote:
> I've added this test just as a sanity check. Not sure how to test the
difference
> in configs. If you have any ideas, please suggest something.

This is just fine, the differences in detection are a detail the emulator
probably shouldn't be concerned with, just as long as it functions properly when
switching between different configurations.

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

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:275: bool enabled,
ui::GestureProviderConfigType config_type);
This looks fine, I guess the alternative is |DisableTouchEventEmulation()| and
|EnableTouchEventEmulation(config_type)|, but maybe that's not any better.

[dgozman, 2015-01-15 11:07:11]

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

https://codereview.chromium.org/821223002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:275: bool enabled,
ui::GestureProviderConfigType config_type);
On 2015/01/14 15:54:17, jdduke wrote:
> This looks fine, I guess the alternative is |DisableTouchEventEmulation()| and
> |EnableTouchEventEmulation(config_type)|, but maybe that's not any better.

Yeah, I considered this and decided it's not worth splitting.

https://codereview.chromium.org/821223002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/821223002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:89: if (!gesture_provider_
|| gesture_provider_config_type_ != config_type) {
I decided to be a little safer here and not recreate provider when we don't have
to.

[dgozman, 2015-01-15 11:09:18]

dgozman@chromium.org changed reviewers:
+ avi@chromium.org
- rbyers@chromium.org

[dgozman, 2015-01-15 11:09:18]

Avi,

Could you please look at content/browser/renderer_host/render_widget_host_* ?

Thanks,
Dmitry

[dgozman, 2015-01-19 11:12:52]

dgozman@chromium.org changed reviewers:
+ jochen@chromium.org
- avi@chromium.org

[dgozman, 2015-01-19 11:12:52]

Jochen,

Could you please look at content/browser/renderer_host/render_widget_host_* ?

Thanks,
Dmitry

[Avi (use Gerrit), 2015-01-19 19:24:26]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2015-01-19 19:24:27]

LGTM

[dgozman, 2015-01-19 20:11:35]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2015-01-19 20:12:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/821223002/40001

[commit-bot: I haz the power, 2015-01-19 21:01:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-19 21:02:39]

Patchset 3 (id:??) landed as
https://crrev.com/7db626dc977e1c0171251431ab5d718048a8627e
Cr-Commit-Position: refs/heads/master@{#312144}