Selectively disable rubber banding on mac.

Selectively disable rubber banding on mac.

The logic for handling 2-finger swipes should be:
1. If a 2-finger swipe can scroll the content (or iframe), it should do so.
2. If the 2-finger swipe can cause history navigation, it should do so.
3. The 2-finger swipe should rubber-band the content view.

(2) is handled by the embedder, and (1 & 3) by the renderer. Right now, the
renderer is passed an IPC, and it tries to do (1), and then it tries to do (3).
I've added a new IPC so that there are flags that prevent the renderer from
performing (3) under specific circumstances.

There was an existing mechanism to determine whether the renderer should try
(3), but it was fragile, incorrectly used, and insufficient for newer features
in chrome - the user can cancel a history swipe without cancelling the gesture,
at which point rubber-banding should be possible again. I've simplified and
fixed the logic inside ScrollElasticityController.mm to reflect the desired
behavior.

I modified the IPC InputMsg_HandleInputEvent to include 2 additional bools:
can_rubberband_left and can_rubberband_right. Blink will attempt to consume the
event to scroll the content view, and will only attempt to rubberband if bools
allow it too.

This is part 1 of a two-part change. It only includes the IPC change. The
changes to ScrollElasticityController will land after I've updated Chromium to
use the new API.

BUG=321437
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170102

[erikchen, 2014-03-14 17:43:31]

Hi thakis! The change is not ready for review, but it works locally, and I
wanted you to sanity check it before I clean it up, write tests, etc.

Please take a look at Source/platform/mac/ScrollElasticityController.mm and tell
me if the changes look reasonable to you.

[Nico, 2014-03-14 17:51:46]

+Alexei, rubberband master

The basic approach seems fine, but I wonder if there's a way to do this without
threading these two bools through so many functions…

(And as always, enum params make for more readable call sites than bool params
– http://www.webkit.org/coding/coding-style.html#names-enum-to-bool )

https://codereview.chromium.org/197213011/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/197213011/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:285: // On Mac WebKit1 the underlying NSScrollView
just does the scrolling, but on most other platforms
(this comment can mostly go away)

https://codereview.chromium.org/197213011/diff/1/Source/platform/mac/ScrollEl...
File Source/platform/mac/ScrollElasticityController.mm (right):

https://codereview.chromium.org/197213011/diff/1/Source/platform/mac/ScrollEl...
Source/platform/mac/ScrollElasticityController.mm:128:
!shouldRubberBandInHorizontalDirection(wheelEvent))
Should the checks be in shouldRubberBandInHorizontalDirection()?

[erikchen, 2014-03-17 21:05:15]

asvitkine: light ping.

[Alexei Svitkine (slow), 2014-03-17 21:08:08]

Sorry, I was thrown off by the "This change is not ready for review.".

Do you mind expanding the CL description a bit to describe both the motivation
for this (I assume it is to allow the iframe fix CL not break rubber banding?)
as well as the overall technique? It would be much more helpful to have that
information when reviewing this.

Thanks!

[erikchen, 2014-03-17 21:24:34]

On 2014/03/17 21:08:08, Alexei Svitkine wrote:
> Sorry, I was thrown off by the "This change is not ready for review.".
> 
> Do you mind expanding the CL description a bit to describe both the motivation
> for this (I assume it is to allow the iframe fix CL not break rubber banding?)
> as well as the overall technique? It would be much more helpful to have that
> information when reviewing this.
> 
> Thanks!

Ah, the details were on a bug on which you are cc'ed. I should have included a
link in the description. Please see comments 8 & 9 on
https://code.google.com/p/chromium/issues/detail?id=321437

[Alexei Svitkine (slow), 2014-03-18 15:56:44]

Perhaps the two bools can be part of WheelEvent?

[erikchen, 2014-03-20 18:04:25]

asvitkine: PTAL

https://codereview.chromium.org/197213011/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/197213011/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:285: // On Mac WebKit1 the underlying NSScrollView
just does the scrolling, but on most other platforms
On 2014/03/14 17:51:46, Nico (on GMT time Mar 15 - 24) wrote:
> (this comment can mostly go away)

After following asvitkine's suggestion, I am no longer touching this file or
core/. I will not perform this clean-up in this change, unless you request
otherwise.

https://codereview.chromium.org/197213011/diff/1/Source/platform/mac/ScrollEl...
File Source/platform/mac/ScrollElasticityController.mm (right):

https://codereview.chromium.org/197213011/diff/1/Source/platform/mac/ScrollEl...
Source/platform/mac/ScrollElasticityController.mm:128:
!shouldRubberBandInHorizontalDirection(wheelEvent))
On 2014/03/14 17:51:46, Nico (on GMT time Mar 15 - 24) wrote:
> Should the checks be in shouldRubberBandInHorizontalDirection()?

I've moved all the relevant logic into the method. I am also calling the method
before starting rubber-banding from a momentum event, which is a behavioral
change which I believe to be accurate.

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
File public/web/mac/WebInputEventFactory.h (right):

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
public/web/mac/WebInputEventFactory.h:56: BLINK_EXPORT static WebMouseWheelEvent
mouseWheelEvent(NSEvent*, NSView*, bool canRubberbandLeft, bool
canRubberbandRight);
The eventual caller of this API (back in chrome land) will not be passing in
constants, but rather a dynamically computed bool. I am going to refrain from
changing the input parameters to an enum. Please let me know if you prefer
otherwise.

[Alexei Svitkine (slow), 2014-03-20 18:38:09]

https://codereview.chromium.org/197213011/diff/80001/Source/platform/mac/Scro...
File Source/platform/mac/ScrollElasticityController.mm (right):

https://codereview.chromium.org/197213011/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollElasticityController.mm:422: if
(m_client->pinnedInDirection(FloatSize(-wheelEvent.deltaX(), 0)) &&
!m_client->shouldRubberBandInDirection(ScrollLeft))
Could the pinnedInDirection() check be made inside
m_client->shouldRubberBandInDirection() so two calls aren't required here?

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
File public/web/mac/WebInputEventFactory.h (right):

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
public/web/mac/WebInputEventFactory.h:56: BLINK_EXPORT static WebMouseWheelEvent
mouseWheelEvent(NSEvent*, NSView*, bool canRubberbandLeft, bool
canRubberbandRight);
On 2014/03/20 18:04:26, erikchen1 wrote:
> The eventual caller of this API (back in chrome land) will not be passing in
> constants, but rather a dynamically computed bool. I am going to refrain from
> changing the input parameters to an enum. Please let me know if you prefer
> otherwise.

Do you need to preserve the old signature before the Chrome change is completed
or does Chrome not call this function at all yet?

[erikchen, 2014-03-20 18:44:50]

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
File public/web/mac/WebInputEventFactory.h (right):

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
public/web/mac/WebInputEventFactory.h:56: BLINK_EXPORT static WebMouseWheelEvent
mouseWheelEvent(NSEvent*, NSView*, bool canRubberbandLeft, bool
canRubberbandRight);
On 2014/03/20 18:38:09, Alexei Svitkine wrote:
> On 2014/03/20 18:04:26, erikchen1 wrote:
> > The eventual caller of this API (back in chrome land) will not be passing in
> > constants, but rather a dynamically computed bool. I am going to refrain
from
> > changing the input parameters to an enum. Please let me know if you prefer
> > otherwise.
> 
> Do you need to preserve the old signature before the Chrome change is
completed
> or does Chrome not call this function at all yet?

I wasn't sure about the standard approach to an API change. Chrome does use this
function. Should I push the blink code with a second function, modify chrome to
use the second function, and then push another blink change deleting the
original function?

[Alexei Svitkine (slow), 2014-03-20 18:50:31]

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
File public/web/mac/WebInputEventFactory.h (right):

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
public/web/mac/WebInputEventFactory.h:56: BLINK_EXPORT static WebMouseWheelEvent
mouseWheelEvent(NSEvent*, NSView*, bool canRubberbandLeft, bool
canRubberbandRight);
On 2014/03/20 18:44:51, erikchen1 wrote:
> On 2014/03/20 18:38:09, Alexei Svitkine wrote:
> > On 2014/03/20 18:04:26, erikchen1 wrote:
> > > The eventual caller of this API (back in chrome land) will not be passing
in
> > > constants, but rather a dynamically computed bool. I am going to refrain
> from
> > > changing the input parameters to an enum. Please let me know if you prefer
> > > otherwise.
> > 
> > Do you need to preserve the old signature before the Chrome change is
> completed
> > or does Chrome not call this function at all yet?
> 
> I wasn't sure about the standard approach to an API change. Chrome does use
this
> function. Should I push the blink code with a second function, modify chrome
to
> use the second function, and then push another blink change deleting the
> original function?

Yes, I think that's the standard way to do this.

[erikchen, 2014-03-24 22:12:04]

asvitkine: PTAL

https://codereview.chromium.org/197213011/diff/80001/Source/platform/mac/Scro...
File Source/platform/mac/ScrollElasticityController.mm (right):

https://codereview.chromium.org/197213011/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollElasticityController.mm:422: if
(m_client->pinnedInDirection(FloatSize(-wheelEvent.deltaX(), 0)) &&
!m_client->shouldRubberBandInDirection(ScrollLeft))
On 2014/03/20 18:38:09, Alexei Svitkine wrote:
> Could the pinnedInDirection() check be made inside
> m_client->shouldRubberBandInDirection() so two calls aren't required here?

I've reworked the logic of this method to be much simpler.
m_client->shouldRubberBandInDirection() is no longer used - see my comment in
the header file and the CL description.

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
File public/web/mac/WebInputEventFactory.h (right):

https://codereview.chromium.org/197213011/diff/80001/public/web/mac/WebInputE...
public/web/mac/WebInputEventFactory.h:56: BLINK_EXPORT static WebMouseWheelEvent
mouseWheelEvent(NSEvent*, NSView*, bool canRubberbandLeft, bool
canRubberbandRight);
On 2014/03/20 18:50:31, Alexei Svitkine wrote:
> On 2014/03/20 18:44:51, erikchen1 wrote:
> > On 2014/03/20 18:38:09, Alexei Svitkine wrote:
> > > On 2014/03/20 18:04:26, erikchen1 wrote:
> > > > The eventual caller of this API (back in chrome land) will not be
passing
> in
> > > > constants, but rather a dynamically computed bool. I am going to refrain
> > from
> > > > changing the input parameters to an enum. Please let me know if you
prefer
> > > > otherwise.
> > > 
> > > Do you need to preserve the old signature before the Chrome change is
> > completed
> > > or does Chrome not call this function at all yet?
> > 
> > I wasn't sure about the standard approach to an API change. Chrome does use
> this
> > function. Should I push the blink code with a second function, modify chrome
> to
> > use the second function, and then push another blink change deleting the
> > original function?
> 
> Yes, I think that's the standard way to do this.

Done.

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
File Source/platform/mac/ScrollElasticityController.h (right):

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
Source/platform/mac/ScrollElasticityController.h:53: // TODO(erikchen): This
method is no longer used. Remove all references.
I left this as a TODO to try to keep this CL smaller and more self-contained.
Let me know if you'd like me to remove all 11 references to this method as a
part of this change.

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
File Source/platform/mac/ScrollElasticityController.mm (right):

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
Source/platform/mac/ScrollElasticityController.mm:133: if (wheelEvent.phase() ==
PlatformWheelEventPhaseEnded || wheelEvent.phase() ==
PlatformWheelEventPhaseCancelled) {
this change wasn't necessary to make this CL work, but it appeared to be an
oversight from the original author. The phase PlatformWheelEventPhaseCancelled
is not mentioned anywhere in this file.

[Alexei Svitkine (slow), 2014-03-25 15:25:39]

LGTM

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
File Source/platform/mac/ScrollElasticityController.h (right):

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
Source/platform/mac/ScrollElasticityController.h:53: // TODO(erikchen): This
method is no longer used. Remove all references.
On 2014/03/24 22:12:05, erikchen1 wrote:
> I left this as a TODO to try to keep this CL smaller and more self-contained.
> Let me know if you'd like me to remove all 11 references to this method as a
> part of this change.

SGTM.

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
Source/platform/mac/ScrollElasticityController.h:73: // Events are passed by IPC
from the embedder. Events on mac are grouped
Nit: Mac

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
Source/platform/mac/ScrollElasticityController.h:110: // has begun. A careful
audit of the code may deprecate the need for this parameter.
Nit: Wrap the end of this comment since everything else has consistent line
length.

[erikchen, 2014-03-26 18:24:29]

asvitkine: Looking for another quick review.

I've decided to break this CL into 2 parts, the API change and the functionality
change. Please see the diff from patchset 5 to patchset 6 - I just removed all
changes to ScrollElasticityController.{h,mm}.

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
File Source/platform/mac/ScrollElasticityController.h (right):

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
Source/platform/mac/ScrollElasticityController.h:73: // Events are passed by IPC
from the embedder. Events on mac are grouped
On 2014/03/25 15:25:40, Alexei Svitkine wrote:
> Nit: Mac

Done.

https://codereview.chromium.org/197213011/diff/110001/Source/platform/mac/Scr...
Source/platform/mac/ScrollElasticityController.h:110: // has begun. A careful
audit of the code may deprecate the need for this parameter.
On 2014/03/25 15:25:40, Alexei Svitkine wrote:
> Nit: Wrap the end of this comment since everything else has consistent line
> length. 

Done.

[Alexei Svitkine (slow), 2014-03-26 19:08:35]

LGTM, I agree the split makes things much cleaner (and avoids problems if we
need to revert).

[erikchen, 2014-03-26 20:10:31]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-03-26 20:10:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/197213011/160001

[commit-bot: I haz the power, 2014-03-26 20:20:34]

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

[commit-bot: I haz the power, 2014-03-26 20:20:34]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[Nico, 2014-03-26 20:22:55]

platform/ owners lgtm

+dglazkov for public/OWNERS and web/OWNERS

[dglazkov, 2014-03-26 20:29:37]

rslgtm.

[Nico, 2014-03-26 20:29:56]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-03-26 20:30:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/197213011/160001

[commit-bot: I haz the power, 2014-03-26 21:14:37]

Change committed as 170102