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.
Created attachment 235442 [details] Patch
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.
Created attachment 235461 [details] Patch
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 on attachment 235461 [details] Patch Sorry, my bad! I told Wenson to set cq+ and see what happens.
Comment on attachment 235461 [details] Patch Clearing flags on attachment: 235461 Committed r171531: <http://trac.webkit.org/changeset/171531>
All reviewed patches have been landed. Closing bug.
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.
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.
Created attachment 235561 [details] Patch
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
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
Created attachment 235564 [details] Patch
Created attachment 235567 [details] Patch
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 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.
Created attachment 235597 [details] Patch
Comment on attachment 235597 [details] Patch Re-instating Darin's r+ and adding a cq+!
Comment on attachment 235597 [details] Patch Clearing flags on attachment: 235597 Committed r171685: <http://trac.webkit.org/changeset/171685>
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.