Bug 81458 - [chromium] synthesize wheel events for fling on main thread
Summary: [chromium] synthesize wheel events for fling on main thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Kroeger
URL:
Keywords:
Depends on: 81457
Blocks: 81479
  Show dependency treegraph
 
Reported: 2012-03-17 14:43 PDT by Robert Kroeger
Modified: 2012-03-19 07:51 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.95 KB, patch)
2012-03-17 17:29 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (7.21 KB, patch)
2012-03-18 11:37 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (20.16 KB, patch)
2012-03-18 17:41 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (20.12 KB, patch)
2012-03-18 18:25 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (19.69 KB, patch)
2012-03-18 18:39 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (19.70 KB, patch)
2012-03-19 06:59 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Kroeger 2012-03-17 14:43:23 PDT
Per extended discussion today: we need a way to synthesize fling events on the main thread.
Comment 1 Robert Kroeger 2012-03-17 17:29:29 PDT
Created attachment 132472 [details]
Patch
Comment 2 James Robinson 2012-03-17 17:55:05 PDT
Comment on attachment 132472 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:370
> +    , m_flingmodifier(0)

nit: m_flingModifier

> Source/WebKit/chromium/src/WebViewImpl.cpp:617
> +    const float tickDivisor = (float)WebCore::WheelEvent::tickMultiplier;

nit: webkit doesn't typically use const for locals like this
nit: webkit requires static_cast<float>() for stuff like this

> Source/WebKit/chromium/src/WebViewImpl.cpp:633
> +    m_lastWheelGlobalPosition.x += delta.x();
> +    m_lastWheelGlobalPosition.y += delta.y();

is it valid to update these regardless of what the mouseWheel handler did? what if it didn't move the page as far as you thought, or if the page didn't move at all?

i think we might have to be a bit more careful about how this value is calculated

> Source/WebKit/chromium/src/WebViewImpl.cpp:646
> +        FloatPoint fp(event.deltaX, event.deltaY);

nit: need a better name for this, or just do the conversion inside the ::create() call

> Source/WebKit/chromium/src/WebViewImpl.h:729
> +	WebPoint	m_lastWheelGlobalPosition;

nit: spacing went odd here
Comment 3 Robert Kroeger 2012-03-18 11:37:49 PDT
Created attachment 132496 [details]
Patch
Comment 4 WebKit Review Bot 2012-03-18 11:42:35 PDT
Attachment 132496 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/src/WebViewImpl.cpp..." exit_code: 1
Source/WebKit/chromium/src/WebViewImpl.h:729:  Extra space between WebPoint and m_lastWheelGlobalPosition  [whitespace/declaration] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Robert Kroeger 2012-03-18 16:02:52 PDT
Comment on attachment 132472 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:370
>> +    , m_flingmodifier(0)
> 
> nit: m_flingModifier

done, p3

>> Source/WebKit/chromium/src/WebViewImpl.cpp:617
>> +    const float tickDivisor = (float)WebCore::WheelEvent::tickMultiplier;
> 
> nit: webkit doesn't typically use const for locals like this
> nit: webkit requires static_cast<float>() for stuff like this

nit1: I thought it made the lines below easier to read to do it this way. Please let me know your preference.

nit2: done in p3

>> Source/WebKit/chromium/src/WebViewImpl.cpp:633
>> +    m_lastWheelGlobalPosition.y += delta.y();
> 
> is it valid to update these regardless of what the mouseWheel handler did? what if it didn't move the page as far as you thought, or if the page didn't move at all?
> 
> i think we might have to be a bit more careful about how this value is calculated

per your observation in the design note, this position can be fixed with respect to the container -- every wheel event has the same position wrt to the window. fixed in p3

>> Source/WebKit/chromium/src/WebViewImpl.cpp:646
>> +        FloatPoint fp(event.deltaX, event.deltaY);
> 
> nit: need a better name for this, or just do the conversion inside the ::create() call

done, patch3

>> Source/WebKit/chromium/src/WebViewImpl.h:729
>> +	WebPoint	m_lastWheelGlobalPosition;
> 
> nit: spacing went odd here

fixed, p3
Comment 6 James Robinson 2012-03-18 16:58:27 PDT
Comment on attachment 132496 [details]
Patch

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

I don't see any patchset 3 here

> Source/WebKit/chromium/src/WebViewImpl.cpp:643
> +        m_gestureAnimation = ActivePlatformGestureAnimation::create(TouchFlingPlatformGestureCurve::create(fp), this);

shouldn't this be a WheelFlingPlatformGestureCurve?
Comment 7 James Robinson 2012-03-18 17:21:24 PDT
Comment on attachment 132496 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1330
> +    // Create synthetic wheel events as necessary for fling.
> +    if (m_gestureAnimation) {

you need to move this block into WebViewImpl::updateAnimations() or it'll be broken in the threaded compositing path
Comment 8 Robert Kroeger 2012-03-18 17:41:55 PDT
Created attachment 132513 [details]
Patch
Comment 9 Robert Kroeger 2012-03-18 17:44:35 PDT
(In reply to comment #6)
> (From update of attachment 132496 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132496&action=review
> 
> I don't see any patchset 3 here

sorry for the delay in uploading it. I went off and added layout tests.

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:643
> > +        m_gestureAnimation = ActivePlatformGestureAnimation::create(TouchFlingPlatformGestureCurve::create(fp), this);
> 
> shouldn't this be a WheelFlingPlatformGestureCurve?

No. The shape of that curve is even worse.
Comment 10 Robert Kroeger 2012-03-18 17:47:31 PDT
Comment on attachment 132496 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1330
>> +    if (m_gestureAnimation) {
> 
> you need to move this block into WebViewImpl::updateAnimations() or it'll be broken in the threaded compositing path

done, p4
Comment 11 James Robinson 2012-03-18 17:48:54 PDT
Comment on attachment 132513 [details]
Patch

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

Please remove the "[not ready for review]" from the bug title.

Otherwise, very close but not quite.

> Source/WebKit/chromium/src/WebViewImpl.cpp:370
> +    , m_flingModifier(0)

this member is always 0 as far as I can tell. why even have it?

> Source/WebKit/chromium/src/WebViewImpl.cpp:417
> +    m_lastWheelPosition = WebPoint(-1, -1);
> +    m_lastWheelGlobalPosition = WebPoint(-1, -1);

WebPoint members will be initialized to (0, 0) - I don't see any value in initializing them again to a different arbitrary value

> Source/WebKit/chromium/src/WebViewImpl.cpp:616
> +    const float tickDivisor = static_cast<float>(WebCore::WheelEvent::tickMultiplier);

do you need an explicit cast here? i don't think we have

> Source/WebKit/chromium/src/WebViewImpl.cpp:645
> +        // FIXME: put this into a sub-function?

you don't need a sub-function for m_gestureAnimation.clear(), i don't think

> Source/WebKit/chromium/src/WebViewImpl.cpp:649
> +        m_flingModifier = 0;
> +        m_lastWheelPosition = WebPoint(-1, -1);
> +        m_lastWheelGlobalPosition = WebPoint(-1, -1);

these all useless useless - these values won't be touched if m_gestureAnimation is NULL

> Source/WebKit/chromium/src/WebViewImpl.cpp:1326
> +    // Create synthetic wheel events as necessary for fling.

this has to be in updateAnimations(). this function is not called at all when using the threaded compositor

> Source/WebKit/chromium/src/WebViewImpl.cpp:1331
> +        scheduleAnimation();
> +    else
> +        m_gestureAnimation.clear();

indentation here is off
Comment 12 Robert Kroeger 2012-03-18 18:25:38 PDT
Created attachment 132516 [details]
Patch
Comment 13 James Robinson 2012-03-18 18:36:22 PDT
Comment on attachment 132516 [details]
Patch

Most of my comments from the previous patch still apply. Not sure why this is up for review.
Comment 14 Robert Kroeger 2012-03-18 18:38:38 PDT
Comment on attachment 132513 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:370
>> +    , m_flingModifier(0)
> 
> this member is always 0 as far as I can tell. why even have it?

i forgot to initialize it. fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:417
>> +    m_lastWheelGlobalPosition = WebPoint(-1, -1);
> 
> WebPoint members will be initialized to (0, 0) - I don't see any value in initializing them again to a different arbitrary value

fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:645
>> +        // FIXME: put this into a sub-function?
> 
> you don't need a sub-function for m_gestureAnimation.clear(), i don't think

great.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:649
>> +        m_lastWheelGlobalPosition = WebPoint(-1, -1);
> 
> these all useless useless - these values won't be touched if m_gestureAnimation is NULL

done

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1326
>> +    // Create synthetic wheel events as necessary for fling.
> 
> this has to be in updateAnimations(). this function is not called at all when using the threaded compositor

done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1331
>> +        m_gestureAnimation.clear();
> 
> indentation here is off

done
Comment 15 Robert Kroeger 2012-03-18 18:39:01 PDT
Created attachment 132519 [details]
Patch
Comment 16 James Robinson 2012-03-18 18:42:28 PDT
Comment on attachment 132519 [details]
Patch

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

OK

> Source/WebKit/chromium/src/WebViewImpl.cpp:1348
> +    else
> +        m_gestureAnimation.clear();

indentation is still broken here. the "else" should be lined up with the "if" it is the other branch for, not some other if statement. as is, this is very confusing to read
Comment 17 Robert Kroeger 2012-03-19 06:59:19 PDT
Created attachment 132571 [details]
Patch
Comment 18 WebKit Review Bot 2012-03-19 07:51:22 PDT
Comment on attachment 132571 [details]
Patch

Clearing flags on attachment: 132571

Committed r111182: <http://trac.webkit.org/changeset/111182>
Comment 19 WebKit Review Bot 2012-03-19 07:51:28 PDT
All reviewed patches have been landed.  Closing bug.