Bug 109898 – Make EventDispatcher take an Event object in its constructor.

• Component: UI Events
• Status: RESOLVED FIXED

[Hayato Ito]

The benefit:
  EventDispatcher will become more *RAII*. We can calculate EventPath in the constructor if an Event object is available. We can also remove EventDispatcher::ensrueEventPath().

[Hayato Ito]

Created attachment 188492 [details]
Constructor takes an Event object.

[Dimitri Glazkov (Google)]

Comment on attachment 188492 [details]
Constructor takes an Event object.

I'll let the EWS elves to finish their work, but this is great.

[WebKit Review Bot]

Comment on attachment 188492 [details]
Constructor takes an Event object.

Clearing flags on attachment: 188492

Committed r143145: <http://trac.webkit.org/changeset/143145>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

I am getting frequent crashes when I try to scroll a page with a track pad:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000001106af4cb WebCore::EventDispatcher::EventDispatcher(WebCore::Node*, WTF::PassRefPtr<WebCore::Event>) + 283 (EventDispatcher.cpp:67)
1   com.apple.WebCore             	0x00000001106af39d WebCore::EventDispatcher::EventDispatcher(WebCore::Node*, WTF::PassRefPtr<WebCore::Event>) + 29 (EventDispatcher.cpp:70)
2   com.apple.WebCore             	0x00000001106af2f3 WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::EventDispatchMediator>) + 195 (EventDispatcher.cpp:54)
3   com.apple.WebCore             	0x0000000111334657 WebCore::Node::dispatchWheelEvent(WebCore::PlatformWheelEvent const&) + 103 (Node.cpp:2402)
4   com.apple.WebCore             	0x00000001106c0739 WebCore::EventHandler::handleWheelEvent(WebCore::PlatformWheelEvent const&) + 1161 (EventHandler.cpp:2375)
5   com.apple.WebKit2             	0x000000010dce7640 WebKit::handleWheelEvent(WebKit::WebWheelEvent const&, WebCore::Page*) + 96 (WebPage.cpp:1615)
6   com.apple.WebKit2             	0x000000010dce7587 WebKit::WebPage::wheelEvent(WebKit::WebWheelEvent const&) + 103 (WebPage.cpp:1625)
7   com.apple.WebKit2             	0x000000010da73e9b WebKit::EventDispatcher::dispatchWheelEvent(unsigned long long, WebKit::WebWheelEvent const&) + 155 (EventDispatcher.cpp:131)
8   com.apple.WebKit2             	0x000000010da75262 WTF::FunctionWrapper<void (WebKit::EventDispatcher::*)(unsigned long long, WebKit::WebWheelEvent const&)>::operator()(WebKit::EventDispatcher*, unsigned long long, WebKit::WebWheelEvent const&) + 130 (Functional.h:274)
9   com.apple.WebKit2             	0x000000010da751d9 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::EventDispatcher::*)(unsigned long long, WebKit::WebWheelEvent const&)>, void (WebKit::EventDispatcher*, unsigned long long, WebKit::WebWheelEvent)>::operator()() + 121 (Functional.h:550)
10  com.apple.WebCore             	0x000000011176f4de WTF::Function<void ()>::operator()() const + 142 (Functional.h:704)
11  com.apple.WebCore             	0x000000011176f250 WebCore::RunLoop::performWork() + 400 (RunLoop.cpp:93)
12  com.apple.WebCore             	0x000000011177054e WebCore::RunLoop::performWork(void*) + 62 (RunLoopCF.cpp:66)
13  com.apple.CoreFoundation      	0x00007fff8ed102b1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17

[Eric Carlson]

The WheelEventDispatchMediator constructor returns early without creating/setting the event if deltaX and deltaY are both 0:

WheelEventDispatchMediator::WheelEventDispatchMediator(const PlatformWheelEvent& event, PassRefPtr<AbstractView> view)
{
    if (!(event.deltaX() || event.deltaY()))
        return;


and none of the downstream code checks to see if there is an event before the EventDispatcher constructor asserts.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110143

[Hayato Ito]

Eric, Thank you for the reporting. I appreciate that.

I should have tested this patch interactively. I've tested this patch only by layout tests. My bad.
Maybe, we should have a layout test so that we could catch the regression in the future.

Let me fix that.

(In reply to comment #6)
> The WheelEventDispatchMediator constructor returns early without creating/setting the event if deltaX and deltaY are both 0:
> 
> WheelEventDispatchMediator::WheelEventDispatchMediator(const PlatformWheelEvent& event, PassRefPtr<AbstractView> view)
> {
>     if (!(event.deltaX() || event.deltaY()))
>         return;
> 
> 
> and none of the downstream code checks to see if there is an event before the EventDispatcher constructor asserts.

[Hayato Ito]

*** Bug 110142 has been marked as a duplicate of this bug. ***

[Hayato Ito]

Let me re-land this. I've fixed a crash and added a layout test to catch this kind of crash.

[Hayato Ito]

Note that I couldn't reproduce the crash interactively since my environment does not issue such a PlatformWheelEvent. Therefore I've added a layout test which caused a crash for the previous patch.
The crash doesn't happen in a new patch which I'll upload soon.

(In reply to comment #10)
> Let me re-land this. I've fixed a crash and added a layout test to catch this kind of crash.

[Hayato Ito]

Created attachment 188999 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 188999 [details]
Patch for landing

Clearing flags on attachment: 188999

Committed r143303: <http://trac.webkit.org/changeset/143303>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.