Bug 81458 – [chromium] synthesize wheel events for fling on main thread

• Component: UI Events
• Status: RESOLVED FIXED

[Robert Kroeger]

Per extended discussion today: we need a way to synthesize fling events on the main thread.

[Robert Kroeger]

Created attachment 132472 [details]
Patch

[James Robinson]

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

[Robert Kroeger]

Created attachment 132496 [details]
Patch

[WebKit Review Bot]

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.

[Robert Kroeger]

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

[James Robinson]

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?

[James Robinson]

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

[Robert Kroeger]

Created attachment 132513 [details]
Patch

[Robert Kroeger]

(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.

[Robert Kroeger]

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

[James Robinson]

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

[Robert Kroeger]

Created attachment 132516 [details]
Patch

[James Robinson]

Comment on attachment 132516 [details]
Patch

Most of my comments from the previous patch still apply. Not sure why this is up for review.

[Robert Kroeger]

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

[Robert Kroeger]

Created attachment 132519 [details]
Patch

[James Robinson]

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

[Robert Kroeger]

Created attachment 132571 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132571 [details]
Patch

Clearing flags on attachment: 132571

Committed r111182: <http://trac.webkit.org/changeset/111182>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.