Disable vsync on input for webview

Disable vsync on input for webview

Add param to ContentViewCore initialize to decide if input can trigger vsync.
The logic for Chrome on Android is moved to ContentView. For webview, disable
vsync on input.

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

[boliu, 2013-05-03 22:03:04]

joth: Could you test this locally first?

[boliu, 2013-05-07 08:32:00]

On 2013/05/03 22:03:04, boliu wrote:
> joth: Could you test this locally first?

Err...and upon closer inspection, of course this does nothing

[boliu, 2013-05-07 09:28:25]

New patch out. Android bot backed up, and just bricked my device, so no testing
done.

[boliu, 2013-05-07 10:17:49]

On 2013/05/07 09:28:25, boliu wrote:
> New patch out. Android bot backed up, and just bricked my device, so no
testing
> done.

And should wait until https://codereview.chromium.org/14821004/ lands and pass
option into ContentViewCore as parameter.

[Sami, 2013-05-07 10:23:00]

https://codereview.chromium.org/14577008/diff/4001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/14577008/diff/4001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:211:
boolean triggerVsyncOnLastGesture) {
Mind making this use named constants instead of bool? Alternatively we could
have a separate constructor that defaults to vsync enabled in addition to the
one that takes the new parameter.

I also thought about having this class check whether the MotionEventDelegate
supports input-as-vsync with a query, but maybe that's a little overkill since
that fact is supposed to remain constant during the life time of this class.

https://codereview.chromium.org/14577008/diff/4001/content/public/android/jav...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java
(right):

https://codereview.chromium.org/14577008/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:1035:
return;
Nit: indent by 4.

[boliu, 2013-05-08 14:19:56]

PTAL. Named constants a bit verbose, but I guess that's the point.

[Sami, 2013-05-08 14:35:08]

Thanks, lgtm. Can't resist squabbling over the naming tho :)

https://codereview.chromium.org/14577008/diff/14001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14577008/diff/14001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:80:
public static final int INPUT_EVENTS_NONE = 0;
bikeshed: how about INPUT_EVENTS_DELIVERED_IMMEDIATELY?

https://codereview.chromium.org/14577008/diff/14001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:595:
int inputEventsDeliveredAtVSync) {
bikeshed: inputEventDeliveryMode? Same for other instances.

[boliu, 2013-05-08 15:02:57]

Rename done

[Sami, 2013-05-08 15:08:37]

Awesome, lgtm.

[joth, 2013-05-08 15:20:11]

basically, lgtm

https://codereview.chromium.org/14577008/diff/17001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/14577008/diff/17001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:333:
ContentViewCore.INPUT_EVENTS_DELIVERED_IMMEDIATELY);
this probably warrants a comment, as it is bit of a lie: the input events are
actually delivered at vsync however we want to trick ContentViewCore into
running the behavior it has for the immediate delivery case (i.e., never drawing
inside the input handler). It'd be nice if we could make this contant reflect
our actual requirement here: INPUT_EVENT_DOES_NOT_IMPLY_VSYNC or something.

https://codereview.chromium.org/14577008/diff/17001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/14577008/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:130:
private boolean mInputEventsDeliveredAtVSync = false;
nit: remove init (as it's set in c'tor whatever)

[boliu, 2013-05-08 15:42:05]

ContentViewGestureHandler.mInputEventsDeliveredAtVSync is now final to reflect
it should never change. Added note in AwContents

[joth, 2013-05-08 15:55:16]

https://codereview.chromium.org/14577008/diff/18006/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/14577008/diff/18006/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:331: //
See note above about INPUT_EVENTS_DELIVERED_IMMEDIATELY.
See note _below_ ... ?
(and again)

given the duplicated comment, how about factoring out a initializeContentView
method?

https://codereview.chromium.org/14577008/diff/18006/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/14577008/diff/18006/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:130:
final private boolean mInputEventsDeliveredAtVSync;
nit: 'private' is normally the first token on the line

[boliu, 2013-05-08 16:45:14]

Done!

https://codereview.chromium.org/14577008/diff/21002/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/14577008/diff/21002/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:596: new
AwPinchGestureStateListener(), mContentsClient.getContentViewClient(),
Found a bug that this AwPinchGestureStateListener was not set here.

[boliu, 2013-05-08 19:59:46]

FYI, had to fix the test for ics devices.

[commit-bot: I haz the power, 2013-05-08 20:00:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/14577008/27001

[commit-bot: I haz the power, 2013-05-09 05:01:16]

Change committed as 199116