Bug 135244 – Let WheelEvent wrap a PlatformWheelEvent

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

If WheelEvent is initialized by a PlatformWheelEvent, store a reference to the PlatformWheelEvent. Subproblem of https://bugs.webkit.org/show_bug.cgi?id=135195 since ScrollAnimator::handleWheelEvent takes in a PlatformWheelEvent but the PlatformWheelEvent is discarded and rebuilt as a WheelEvent in Element::dispatchWheelEvent, we need a way of accessing the original PlatformWheelEvent in EventHandler. This will ultimately allow ScrollAnimator to know about momentum phases from the PlatformWheelEvent, which is necessary to implement scroll snapping (see https://bugs.webkit.org/show_bug.cgi?id=134283), among other things.

[Wenson Hsieh]

Created attachment 235442 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/dom/WheelEvent.cpp:73:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


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

[Wenson Hsieh]

Created attachment 235461 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 235461 [details]
Patch

Rejecting attachment 235461 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Beth Dakin]

Comment on attachment 235461 [details]
Patch

Sorry, my bad! I told Wenson to set cq+ and see what happens.

[WebKit Commit Bot]

Comment on attachment 235461 [details]
Patch

Clearing flags on attachment: 235461

Committed r171531: <http://trac.webkit.org/changeset/171531>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

Comment on attachment 235461 [details]
Patch

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

> Source/WebCore/dom/WheelEvent.h:114
> +    std::unique_ptr<PlatformWheelEvent> m_wheelEvent;

This seems like it is duplicating state now.  I would prefer if we just had the PlatformWheelEvent itself (not in a std::unique_ptr) and we remove the duplicate state.

[Sam Weinig]

Re-opening, since I don't think this fix is quite right.  We shouldn't need to use a unique_ptr or duplicate the state.

[Wenson Hsieh]

Created attachment 235561 [details]
Patch

[Build Bot]

Comment on attachment 235561 [details]
Patch

Attachment 235561 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4633332109279232

New failing tests:
fast/events/wheelevent-constructor.html

[Build Bot]

Created attachment 235563 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Wenson Hsieh]

Created attachment 235564 [details]
Patch

[Wenson Hsieh]

Created attachment 235567 [details]
Patch

[Darin Adler]

Comment on attachment 235567 [details]
Patch

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

> Source/WebCore/dom/WheelEvent.cpp:53
> +    , m_wheelEvent(PlatformWheelEvent())

No need to include this. This is what happens if you don’t explicitly initialize m_wheelEvent, so you should just omit this line.

> Source/WebCore/dom/WheelEvent.cpp:65
> +    , m_wheelEvent(PlatformWheelEvent())

No need to include this. This is what happens if you don’t explicitly initialize m_wheelEvent, so you should just omit this line.

> Source/WebCore/dom/WheelEvent.h:81
> +    // Returns a null pointer if WheelEvent was not created using a PlatformWheelEvent.
> +    const PlatformWheelEvent* wheelEvent() const { return m_initializedWithPlatformWheelEvent ? &m_wheelEvent : nullptr; }

This comment is not needed if we put the implementation inline like this, since the code says the same thing the comment does.

But also, I am not convinced this boolean and null pointer technique is the right way to achieve what you want to achieve. Since this patch doesn’t include the changes to code that uses the wheelEvent() function, I can’t judge whether an alternative solution would be better, since I don’t know what problem the nullptr is trying to solve.

[Wenson Hsieh]

Comment on attachment 235567 [details]
Patch

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

Thank you for the review! I'll put up a new version very soon.

>> Source/WebCore/dom/WheelEvent.cpp:53
>> +    , m_wheelEvent(PlatformWheelEvent())
> 
> No need to include this. This is what happens if you don’t explicitly initialize m_wheelEvent, so you should just omit this line.

Good catch -- thanks!

>> Source/WebCore/dom/WheelEvent.cpp:65
>> +    , m_wheelEvent(PlatformWheelEvent())
> 
> No need to include this. This is what happens if you don’t explicitly initialize m_wheelEvent, so you should just omit this line.

Fixed.

>> Source/WebCore/dom/WheelEvent.h:81
>> +    const PlatformWheelEvent* wheelEvent() const { return m_initializedWithPlatformWheelEvent ? &m_wheelEvent : nullptr; }
> 
> This comment is not needed if we put the implementation inline like this, since the code says the same thing the comment does.
> 
> But also, I am not convinced this boolean and null pointer technique is the right way to achieve what you want to achieve. Since this patch doesn’t include the changes to code that uses the wheelEvent() function, I can’t judge whether an alternative solution would be better, since I don’t know what problem the nullptr is trying to solve.

Got it -- I'll take out the comment. The problem I'm planning to address with this is that I need to distinguish events initialized through JavaScript (e.g. document.createEvent("WheelEvent")) that don't have corresponding PlatformWheelEvents from regular wheel events created using PlatformWheelEvent. Depending on which type the wheel event is, I take a different code path -- one code path calls ScrollableArea::scroll directly with a scroll delta, granularity, and direction, while the other code path calls ScrollableArea::handleWheelEvent with the PlatformWheelEvent. Ultimately, I need the PlatformWheelEvent's momentum phase information to implement scroll snapping on a Mac, although this information is also necessary for implementing behaviors like rubber-banding when overflow scrolling.

[Wenson Hsieh]

Created attachment 235597 [details]
Patch

[Beth Dakin]

Comment on attachment 235597 [details]
Patch

Re-instating Darin's r+ and adding a cq+!

[WebKit Commit Bot]

Comment on attachment 235597 [details]
Patch

Clearing flags on attachment: 235597

Committed r171685: <http://trac.webkit.org/changeset/171685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Wenson Hsieh]

Comment on attachment 235597 [details]
Patch

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

> Source/WebCore/dom/WheelEvent.h:81
>      double deltaX() const { return m_deltaX; } // Positive when scrolling right.

I've tried wrapping these up along with the PlatformWheelEvent, but they ended up breaking LayoutTests/fast/events/wheelevent-constructor.html, since double precision is required (the diff ended up looking like 20.3 vs. 20.29999...).

> Source/WebCore/dom/WheelEvent.h:87
>      unsigned deltaMode() const { return m_deltaMode; }

m_deltaMode could also be removed from WheelEvent, but doing so would mean that we need to compute the deltaMode on the fly using PlatformWheelEvent.granularity() every time deltaMode() is called. I chose not to take this option and store the result in m_deltaMode instead.