Bug 109898 - Make EventDispatcher take an Event object in its constructor.
Summary: Make EventDispatcher take an Event object in its constructor.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
: 110142 (view as bug list)
Depends on: 110143
Blocks: 109905
  Show dependency treegraph
 
Reported: 2013-02-14 23:24 PST by Hayato Ito
Modified: 2013-02-18 23:59 PST (History)
7 users (show)

See Also:


Attachments
Constructor takes an Event object. (20.98 KB, patch)
2013-02-14 23:42 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (24.33 KB, patch)
2013-02-18 23:13 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2013-02-14 23:24:13 PST
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().
Comment 1 Hayato Ito 2013-02-14 23:42:18 PST
Created attachment 188492 [details]
Constructor takes an Event object.
Comment 2 Dimitri Glazkov (Google) 2013-02-15 10:31:37 PST
Comment on attachment 188492 [details]
Constructor takes an Event object.

I'll let the EWS elves to finish their work, but this is great.
Comment 3 WebKit Review Bot 2013-02-17 21:53:30 PST
Comment on attachment 188492 [details]
Constructor takes an Event object.

Clearing flags on attachment: 188492

Committed r143145: <http://trac.webkit.org/changeset/143145>
Comment 4 WebKit Review Bot 2013-02-17 21:53:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Eric Carlson 2013-02-18 07:54:11 PST
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
Comment 6 Eric Carlson 2013-02-18 08:11:45 PST
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.
Comment 7 WebKit Review Bot 2013-02-18 11:04:34 PST
Re-opened since this is blocked by bug 110143
Comment 8 Hayato Ito 2013-02-18 18:24:14 PST
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.
Comment 9 Hayato Ito 2013-02-18 21:07:17 PST
*** Bug 110142 has been marked as a duplicate of this bug. ***
Comment 10 Hayato Ito 2013-02-18 22:40:12 PST
Let me re-land this. I've fixed a crash and added a layout test to catch this kind of crash.
Comment 11 Hayato Ito 2013-02-18 22:44:25 PST
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.
Comment 12 Hayato Ito 2013-02-18 23:13:08 PST
Created attachment 188999 [details]
Patch for landing
Comment 13 WebKit Review Bot 2013-02-18 23:59:18 PST
Comment on attachment 188999 [details]
Patch for landing

Clearing flags on attachment: 188999

Committed r143303: <http://trac.webkit.org/changeset/143303>
Comment 14 WebKit Review Bot 2013-02-18 23:59:22 PST
All reviewed patches have been landed.  Closing bug.