DevTools: control touch emulation from the browser side only.

DevTools: control touch emulation from the browser side only.

TBR=tsepez (message is removed from content/common/view_messages.h)

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

[dgozman, 2014-08-16 11:12:09]

Can we become out-of-sync with renderer part of touch emulation due to
navigations?

Also, here in browser we don't have inspector state, and should manage it
manually. Do you think it's worth to generalize?

[pfeldman, 2014-08-16 11:15:23]

On 2014/08/16 11:12:09, dgozman wrote:
> Can we become out-of-sync with renderer part of touch emulation due to
> navigations?
> 

I don't think we can. But if that is a concern, we should make a direct call to
render_view that would enable emulation, not the other way around.

> Also, here in browser we don't have inspector state, and should manage it
> manually. Do you think it's worth to generalize?

I like this approach better because renderer has less control over what is
happening. It is also more explicit.

[pfeldman, 2014-08-17 07:19:02]

Rebaselined on ToT. I think getting rid of a renderer->browser IPC and
OnTouchEmulationEnabled callback in the overrides handler is worth it. If we get
out of sync with RenderViewHost in the overrides handler vs RenderView in the
renderer, we are in a much bigger problem anyways. Overall, with blink/ getting
closer to content/ + out of process iframes + mojo, I think we should place more
of the agents code into browser/content.

[dgozman, 2014-08-17 12:39:21]

Overall, I support this. I think that patch title implies removing any handling
on the renderer side, which is not true. Could you please rephrase?

https://codereview.chromium.org/474213002/diff/80001/content/browser/devtools...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/474213002/diff/80001/content/browser/devtools...
content/browser/devtools/renderer_overrides_handler.cc:999: void
RendererOverridesHandler::SetTouchEventEmulationEnabled(bool enabled) {
Calls from start/stopScreencast and setTouchEventEmulation may interfere here. I
see them both useful for android with external keyboard for example. I propose
to rename this method to |UpdateTouchEventEmulationInHost| and check for both
flags: |screencast_command_| and |touch_emulation_enabled_|.

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

https://codereview.chromium.org/474213002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1605: bool enabled) {
nit: this should fit previous line

[pfeldman, 2014-08-17 14:47:14]

https://codereview.chromium.org/474213002/diff/80001/content/browser/devtools...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/474213002/diff/80001/content/browser/devtools...
content/browser/devtools/renderer_overrides_handler.cc:999: void
RendererOverridesHandler::SetTouchEventEmulationEnabled(bool enabled) {
Could you remind me why we enable touch while screencasting?

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

https://codereview.chromium.org/474213002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1605: bool enabled) {
On 2014/08/17 12:39:20, dgozman wrote:
> nit: this should fit previous line

Done.

[dgozman, 2014-08-17 15:01:41]

https://codereview.chromium.org/474213002/diff/80001/content/browser/devtools...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/474213002/diff/80001/content/browser/devtools...
content/browser/devtools/renderer_overrides_handler.cc:999: void
RendererOverridesHandler::SetTouchEventEmulationEnabled(bool enabled) {
On 2014/08/17 14:47:14, pfeldman wrote:
> Could you remind me why we enable touch while screencasting?

We use touch emulator to handle emulateTouchFromMouseEvent command.

[pfeldman, 2014-08-17 15:26:34]

This seems wrong: what if that command is issued by the client without enabling
the screencast?

[pfeldman, 2014-08-18 20:23:19]

PTAL

[dgozman, 2014-08-19 04:11:50]

lgtm. Please follow up with blink patch.

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

https://codereview.chromium.org/474213002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1605: bool enabled) {
nit: fits one line.

[pfeldman, 2014-08-19 08:40:42]

+jam for OWNERS stamp.

@jam: two changes to approve here:

- removal of ViewHostMsg_SetTouchEventEmulationEnabled that goes from renderer
to the browser because we should not need it
- RenderWidgetHostDelegate::OnTouchEmulationEnabled callback is replaced with
explicit WebContentsImpl::SetForceDisableOverscrollContent method. That's in
fact was OnTouchEmulationEnabled was for.

rest is the changes to the call sites.

[pfeldman, 2014-08-19 12:37:21]

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

https://codereview.chromium.org/474213002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1605: bool enabled) {
On 2014/08/19 04:11:50, dgozman wrote:
> nit: fits one line.

Done.

[jam, 2014-08-19 19:51:31]

lgtm

[pfeldman, 2014-08-19 20:12:04]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-08-19 20:12:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pfeldman@chromium.org/474213002/180001

[commit-bot: I haz the power, 2014-08-19 21:57:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-19 22:01:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-19 22:01:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pfeldman, 2014-08-20 05:08:39]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-08-20 05:09:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pfeldman@chromium.org/474213002/180001

[commit-bot: I haz the power, 2014-08-20 06:16:16]

Committed patchset #6 (180001) as 290775