Bug 135244 - Let WheelEvent wrap a PlatformWheelEvent
Summary: Let WheelEvent wrap a PlatformWheelEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 135195
  Show dependency treegraph
 
Reported: 2014-07-24 11:39 PDT by Wenson Hsieh
Modified: 2014-07-28 12:46 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.60 KB, patch)
2014-07-24 12:07 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (2.54 KB, patch)
2014-07-24 14:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (8.85 KB, patch)
2014-07-25 20:12 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (553.76 KB, application/zip)
2014-07-25 20:44 PDT, Build Bot
no flags Details
Patch (5.28 KB, patch)
2014-07-25 21:49 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2014-07-25 23:21 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.14 KB, patch)
2014-07-28 09:04 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2014-07-24 11:39:12 PDT
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.
Comment 1 Wenson Hsieh 2014-07-24 12:07:29 PDT
Created attachment 235442 [details]
Patch
Comment 2 WebKit Commit Bot 2014-07-24 12:10:46 PDT
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.
Comment 3 Wenson Hsieh 2014-07-24 14:22:44 PDT
Created attachment 235461 [details]
Patch
Comment 4 WebKit Commit Bot 2014-07-24 14:44:14 PDT
Comment on attachment 235461 [details]
Patch

Rejecting attachment 235461 [details] from commit-queue.

wenson_hsieh@apple.com 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.
Comment 5 Beth Dakin 2014-07-24 14:45:48 PDT
Comment on attachment 235461 [details]
Patch

Sorry, my bad! I told Wenson to set cq+ and see what happens.
Comment 6 WebKit Commit Bot 2014-07-24 15:18:19 PDT
Comment on attachment 235461 [details]
Patch

Clearing flags on attachment: 235461

Committed r171531: <http://trac.webkit.org/changeset/171531>
Comment 7 WebKit Commit Bot 2014-07-24 15:18:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Sam Weinig 2014-07-25 16:34:22 PDT
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.
Comment 9 Sam Weinig 2014-07-25 16:36:08 PDT
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.
Comment 10 Wenson Hsieh 2014-07-25 20:12:09 PDT
Created attachment 235561 [details]
Patch
Comment 11 Build Bot 2014-07-25 20:44:05 PDT
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
Comment 12 Build Bot 2014-07-25 20:44:09 PDT
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
Comment 13 Wenson Hsieh 2014-07-25 21:49:20 PDT
Created attachment 235564 [details]
Patch
Comment 14 Wenson Hsieh 2014-07-25 23:21:24 PDT
Created attachment 235567 [details]
Patch
Comment 15 Darin Adler 2014-07-27 23:14:48 PDT
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.
Comment 16 Wenson Hsieh 2014-07-28 00:00:47 PDT
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.
Comment 17 Wenson Hsieh 2014-07-28 09:04:56 PDT
Created attachment 235597 [details]
Patch
Comment 18 Beth Dakin 2014-07-28 12:32:28 PDT
Comment on attachment 235597 [details]
Patch

Re-instating Darin's r+ and adding a cq+!
Comment 19 WebKit Commit Bot 2014-07-28 12:36:05 PDT
Comment on attachment 235597 [details]
Patch

Clearing flags on attachment: 235597

Committed r171685: <http://trac.webkit.org/changeset/171685>
Comment 20 WebKit Commit Bot 2014-07-28 12:36:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Wenson Hsieh 2014-07-28 12:46:31 PDT
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.