Bug 92900 – [chromium] Upstream android's FlingAnimator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Nate Chapin]

Also, include a USE(FLING_ANIMATOR) flag to keep track of the associated code.

[Nate Chapin]

Created attachment 155863 [details]
patch

[James Robinson]

Comment on attachment 155863 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155863&action=review

> Source/WebKit/chromium/src/android/FlingAnimator.cpp:30
> +#include "base/android/jni_android.h"
> +#include "base/android/scoped_java_ref.h"

This #include won't compile in all configs and is a bad dependency.

I don't think any WebKit code should know about JNI.  Previously when we talked about this animator we had (I thought) decided that this code should call out to chromium code via WebKit API and chromium code can do whatever communication with JNI it needs to.

[Adam Barth]

For context, FlingAnimator is responsible for about 15% of the remaining chromium-android diff in Source/WebKit.

The idea behind this patch is to remove this code from the diff and unblock the upstreaming effort.  Eventually, this code will be removed in favor of asking the embedder to supply the gesture animation curve.

[James Robinson]

I understand, but #include'ing base/... is not something we can sustain upstream.

[Adam Barth]

Maybe we should change FlingAnimator to be WebFlingAnimator and put the Impl in Chromium where it can #include base.

[James Robinson]

That's pretty much what I'm suggesting in comment #2.  We talked about this (me and some clank graphics folks) a few months ago and I thought everyone had agreed to do so at the time, but I'm guessing nobody actually did anything.

[Adam Barth]

Ah, I misunderstood what you were suggesting.  Yes.  The problem is that no one has done that and we're now running up against this as a blocker.

[James Robinson]

Sounds like a good reason to do it!

[Nate Chapin]

Created attachment 156965 [details]
Add public interface so impl can live downstream

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 156965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/src/PlatformGestureCurveFactory.h:39:  The parameter name "point" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/PlatformGestureCurveFactory.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 156965 [details]
Add public interface so impl can live downstream

View in context: https://bugs.webkit.org/attachment.cgi?id=156965&action=review

> Source/Platform/chromium/public/Platform.h:414
> +    virtual WebFlingAnimator* createFlingAnimator() { return 0; }

If you're going to reference WebFlingAnimator from Source/Platform, you should move WebFlingAnimator.h to Source/Platform/chromium/public.

[Nate Chapin]

Created attachment 156980 [details]
Move WebFlingAnimator.h to Platform, fix style issues

[WebKit Review Bot]

Attachment 156980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/src/WebFlingAnimatorToGestureCurveAdapter.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 156980 [details]
Move WebFlingAnimator.h to Platform, fix style issues

View in context: https://bugs.webkit.org/attachment.cgi?id=156980&action=review

> Source/Platform/chromium/public/WebFlingAnimator.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

> Source/Platform/chromium/public/WebFlingAnimator.h:39
> +    virtual void startFling(const WebCore::FloatPoint& velocity, const WebCore::IntRect& range) = 0;

We can't use WebCore:: types in the public API since code that isn't in WebKit can't use these types - stuff in chromium can't include any WebCore headers.  We have wrapper types for geometry classes that you can use instead - WebFloatPoint, WebRect, etc.  They're just simple POD structs with some helpers to easily convert back/forth with WebCore and chromium types.

> Source/WebKit/chromium/src/PlatformGestureCurveFactory.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

I think you could put all of this in WebCore/platform/chromium/support/ - that's the preferred location for implementations of Platform API that depends on WebCore but doesn't depend on anything in WebKit/chromium/src/

> Source/WebKit/chromium/src/WebFlingAnimatorToGestureCurveAdapter.h:46
> +    // WebCore::PlatformGestureCurve implementation:

could you add OVERRIDE annotations to functions that override from some base class? they go after the signature and just before the implementation

[Nate Chapin]

Created attachment 156992 [details]
Address jamesr's comments

[James Robinson]

Comment on attachment 156992 [details]
Address jamesr's comments

I'm happy with WebFlingAnimator, PlatformGestureCurve*, etc but have some questions about the WebCompositorInputHandlerImpl and CC* changes.  Could you break those changes out to a different patch so we can land that part and then hash out the scrolling behavior details?

[Nate Chapin]

Created attachment 157032 [details]
Remove WebKit/chromium and WebCore/platform/graphics/chromium/ changes

[James Robinson]

Comment on attachment 157032 [details]
Remove WebKit/chromium and WebCore/platform/graphics/chromium/ changes

View in context: https://bugs.webkit.org/attachment.cgi?id=157032&action=review

R=me. Will wait for the EWS bot before touching cq, just to be safe.

> Source/Platform/ChangeLog:8
> +        Tests in chromium's WebCompositorInputHandlerImplTest.

This isn't quite accurate any more

> Source/WebCore/ChangeLog:8
> +        Tests in chromium's WebCompositorInputHandlerImplTest.

I think you want to update this bit.

[Nate Chapin]

Created attachment 157041 [details]
Address ChangeLog nits

[Nate Chapin]

Created attachment 157042 [details]
Address ChangeLog nits

[WebKit Review Bot]

Comment on attachment 157042 [details]
Address ChangeLog nits

Rejecting attachment 157042 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
hromium/support/WebFlingAnimatorToGestureCurveAdapter.h
patching file Source/WebCore/platform/chromium/support/PlatformGestureCurveFactory.cpp
patching file Source/WebCore/WebCore.gypi
Hunk #1 FAILED at 8216.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/WebCore.gypi.rej
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13456304

[Nate Chapin]

Created attachment 157253 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 157253 [details]
Patch for landing

Clearing flags on attachment: 157253

Committed r125061: <http://trac.webkit.org/changeset/125061>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

Reopening for the second stage...

[Nate Chapin]

Created attachment 157280 [details]
The remainder (cc and WebKit/chromium changes)

[James Robinson]

New bug, please.

[Nate Chapin]

(In reply to comment #28)
> New bug, please.

Doh, sorry.