Disable overscroll navigation when emulating touch.

Disable overscroll navigation when emulating touch.

When emulating another device in DevTools (including touch),
overscroll navigation is confusing for the user:
- if device does not support touch, user is not used
to the overscroll navigation and may be surprised;
- with content not filling the whole view, overscroll navigation
looks unnatural.

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

[dgozman, 2014-06-04 09:52:33]

Could you please take a look?

[mfomitchev, 2014-06-04 21:50:36]

+sadrul FYI

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

https://codereview.chromium.org/313083002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_delegate.h:63: virtual void
SetOverscrollContentDisabled(bool disabled) {}
This doesn't really belong in the RWHDelegate interface IMHO since RWH knows
nothing about overscroll. Perhaps it would make sense to handle
ViewHostMsg_SetTouchEventEmulationEnabled in web_contents_impl instead? Then we
wouldn't have to change the delegate interface (although we may need to make the
method for enabling touch emulation in RWHI public).

[dgozman, 2014-06-06 14:07:31]

+avi for advice as a content owner.

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

https://codereview.chromium.org/313083002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_delegate.h:63: virtual void
SetOverscrollContentDisabled(bool disabled) {}
On 2014/06/04 21:50:36, mfomitchev wrote:
> This doesn't really belong in the RWHDelegate interface IMHO since RWH knows
> nothing about overscroll. Perhaps it would make sense to handle
> ViewHostMsg_SetTouchEventEmulationEnabled in web_contents_impl instead? Then
we
> wouldn't have to change the delegate interface (although we may need to make
the
> method for enabling touch emulation in RWHI public). 

I don't like this one either. Unfortunately, moving message handling to
WebContents is hard, since during navigation it has multiple RWHIs and
complicated logic. Ensuring the right time for this message initiated from Blink
may be unfeasible.

We should either go from RWHI to WebContents (as in this patch), or disable
overscroll on RWHV/OverscrollController level. WDYT?

[Avi (use Gerrit), 2014-06-06 14:36:17]

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

https://codereview.chromium.org/313083002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_delegate.h:63: virtual void
SetOverscrollContentDisabled(bool disabled) {}
Well, RWH knows about "touch emulation" so perhaps this could be renamed as
OnSetTouchEventEmulationEnabled, and let the WebContents say, "hey, in this case
let's disable overscroll".

[mfomitchev, 2014-06-06 15:32:48]

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

https://codereview.chromium.org/313083002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_delegate.h:63: virtual void
SetOverscrollContentDisabled(bool disabled) {}
On 2014/06/06 14:36:17, Avi wrote:
> Well, RWH knows about "touch emulation" so perhaps this could be renamed as
> OnSetTouchEventEmulationEnabled, and let the WebContents say, "hey, in this
case
> let's disable overscroll".

This sounds reasonable to me.

[dgozman, 2014-06-06 15:45:29]

PTAL

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

https://codereview.chromium.org/313083002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_delegate.h:63: virtual void
SetOverscrollContentDisabled(bool disabled) {}
On 2014/06/06 14:36:17, Avi wrote:
> Well, RWH knows about "touch emulation" so perhaps this could be renamed as
> OnSetTouchEventEmulationEnabled, and let the WebContents say, "hey, in this
case
> let's disable overscroll".

Done.

[Avi (use Gerrit), 2014-06-06 15:47:06]

I'm not an expert in emulation and overscroll and stuff, but I'm cool with this.
LGTM with an OK from someone more involved in this than I am.

[dgozman, 2014-06-09 09:47:20]

On 2014/06/06 15:47:06, Avi wrote:
> I'm not an expert in emulation and overscroll and stuff, but I'm cool with
this.
> LGTM with an OK from someone more involved in this than I am.

mfomitchev@ is an overscroll expert. Take a look please.

[mfomitchev, 2014-06-09 13:05:14]

https://codereview.chromium.org/313083002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/313083002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:509: virtual void
OnSetTouchEventEmulationEnabled(bool enabled) OVERRIDE;
Nit: Perhaps call this OnTouchEmulationEnabled or OnTouchEventEmulationEnabled?
"OnSet..." just makes the name longer without making it any more clear what the
method does.

https://codereview.chromium.org/313083002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:1127: bool
overscroll_content_disabled_;
Can we call this touch_emulation_enabled_? Makes it clear what this represents,
plus gets rid of the double negative.

[dgozman, 2014-06-09 14:58:04]

PTAL

https://codereview.chromium.org/313083002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/313083002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:509: virtual void
OnSetTouchEventEmulationEnabled(bool enabled) OVERRIDE;
On 2014/06/09 13:05:14, mfomitchev wrote:
> Nit: Perhaps call this OnTouchEmulationEnabled or
OnTouchEventEmulationEnabled?
> "OnSet..." just makes the name longer without making it any more clear what
the
> method does.

Done.

https://codereview.chromium.org/313083002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:1127: bool
overscroll_content_disabled_;
On 2014/06/09 13:05:14, mfomitchev wrote:
> Can we call this touch_emulation_enabled_? Makes it clear what this
represents,
> plus gets rid of the double negative.

Done.

[mfomitchev, 2014-06-09 14:59:55]

LGTM

[dgozman, 2014-06-09 15:02:17]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-06-09 15:03:33]

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

[commit-bot: I haz the power, 2014-06-10 07:10:23]

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

[commit-bot: I haz the power, 2014-06-10 11:50:53]

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

[commit-bot: I haz the power, 2014-06-10 11:50:54]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[dgozman, 2014-06-10 11:51:37]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-06-10 11:55:10]

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

[commit-bot: I haz the power, 2014-06-10 14:34:34]

Change committed as 276019