Also, include a USE(FLING_ANIMATOR) flag to keep track of the associated code.
Created attachment 155863 [details] patch
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.
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.
I understand, but #include'ing base/... is not something we can sustain upstream.
Maybe we should change FlingAnimator to be WebFlingAnimator and put the Impl in Chromium where it can #include base.
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.
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.
Sounds like a good reason to do it!
Created attachment 156965 [details] Add public interface so impl can live downstream
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
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.
Created attachment 156980 [details] Move WebFlingAnimator.h to Platform, fix style issues
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.
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
Created attachment 156992 [details] Address jamesr's comments
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?
Created attachment 157032 [details] Remove WebKit/chromium and WebCore/platform/graphics/chromium/ changes
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.
Created attachment 157041 [details] Address ChangeLog nits
Created attachment 157042 [details] Address ChangeLog nits
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
Created attachment 157253 [details] Patch for landing
Comment on attachment 157253 [details] Patch for landing Clearing flags on attachment: 157253 Committed r125061: <http://trac.webkit.org/changeset/125061>
All reviewed patches have been landed. Closing bug.
Reopening for the second stage...
Created attachment 157280 [details] The remainder (cc and WebKit/chromium changes)
New bug, please.
(In reply to comment #28) > New bug, please. Doh, sorry.