Bug 91731

Summary: [EFL][WK2][WTR] Implement EventSenderProxy
Product: WebKit Reporter: Kangil Han <kangil.han>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alexander.shalamov, cdumez, d-r, eric, gyuyoung.kim, gyuyoung.kim, haraken, lucas.de.marchi, morrita, rakuco, ryuan.choi, sw0524.lee, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch none

Description Kangil Han 2012-07-19 04:20:43 PDT
WK2 EFL needs EventSenderProxy to send events to WebKitTestRunner.
Comment 1 Kangil Han 2012-07-19 05:57:16 PDT
Created attachment 153239 [details]
patch
Comment 2 WebKit Review Bot 2012-07-19 06:00:30 PDT
Attachment 153239 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner..." exit_code: 1
Tools/WebKitTestRunner/EventSenderProxy.h:37:  "wtf/Vector.h" already included at Tools/WebKitTestRunner/EventSenderProxy.h:35  [build/include] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Kangil Han 2012-07-19 06:32:26 PDT
Created attachment 153243 [details]
patch
Comment 4 Gyuyoung Kim 2012-07-19 07:57:17 PDT
Comment on attachment 153243 [details]
patch

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

I would review this patch further. BTW, I wonder how many test cases this patch supports on WTR.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:391
> +    // FIXME: Implement this

Personally, I prefer notImplemented() to this.
Comment 5 Chris Dumez 2012-07-19 09:48:22 PDT
Comment on attachment 153243 [details]
patch

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

> Tools/WebKitTestRunner/EventSenderProxy.h:98
> +    void sendOrQueueEvent(WTREvent);

Pass by const reference instead of by value?

> Tools/WebKitTestRunner/EventSenderProxy.h:99
> +    void dispatchEvent(WTREvent);

Ditto.

> Tools/WebKitTestRunner/EventSenderProxy.h:124
> +    Vector<WTREvent> m_eventQueue;

You only push_back() and pop_front() on this container. Vector is definitely not the best container for this. How about a WTF::Deque? it has O(1) for pop_front() and push_back().

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:84
> +    int vertical;

Would add a blank line after this.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:85
> +    WTREvent()

modifiers, horizontal and vertical are not initialized.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:90
> +    }

Would add a blank line after this.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:91
> +    WTREvent(WTREventType eventType, unsigned delay, WKEventModifiers modifiers, int button)

horizontal and vertical are not initialized.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:128
> +    for (unsigned modifier = 0; modifier < 4; ++modifier) {

can we use sizeof(modifierNames)/sizeof(char*) instead of 4 here?
Comment 6 Kangil Han 2012-07-19 22:34:56 PDT
Created attachment 153414 [details]
patch
Comment 7 Kangil Han 2012-07-19 22:44:27 PDT
It seems that there are around 700 cases using eventSender. All done, thanks!
Comment 8 Chris Dumez 2012-07-19 22:59:28 PDT
Comment on attachment 153414 [details]
patch

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

> Tools/WebKitTestRunner/EventSenderProxy.h:97
> +    void sendOrQueueEvent(const WTREvent);

You added "const" but forgot the reference (which is the most important).

> Tools/WebKitTestRunner/EventSenderProxy.h:98
> +    void dispatchEvent(const WTREvent);

Ditto.
Comment 9 Chris Dumez 2012-07-19 23:01:05 PDT
Comment on attachment 153414 [details]
patch

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

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:87
> +    WTREvent()

Modifiers is not initialized.
Comment 10 Kangil Han 2012-07-19 23:12:35 PDT
Created attachment 153423 [details]
patch
Comment 11 Kangil Han 2012-07-19 23:12:48 PDT
Done, thanks!
Comment 12 Chris Dumez 2012-07-19 23:17:19 PDT
Comment on attachment 153423 [details]
patch

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

LGTM but:

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:313
> +        WTREvent wtrEvent = m_eventQueue.first();

You could use Deque::takeFirst().

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:318
> +        m_eventQueue.removeFirst();

This line would become useless.
Comment 13 Kangil Han 2012-07-19 23:20:09 PDT
Created attachment 153424 [details]
patch
Comment 14 Kangil Han 2012-07-19 23:20:22 PDT
Done, thanks!
Comment 15 Chris Dumez 2012-07-19 23:21:23 PDT
Comment on attachment 153424 [details]
patch

LGTM. Thanks.
Comment 16 Kangil Han 2012-07-19 23:22:00 PDT
(In reply to comment #15)
> (From update of attachment 153424 [details])
> LGTM. Thanks.

Thanks for your review!
Comment 17 Kangil Han 2012-07-19 23:23:35 PDT
(In reply to comment #4)
> (From update of attachment 153243 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153243&action=review
> 
> I would review this patch further. BTW, I wonder how many test cases this patch supports on WTR.
> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:391
> > +    // FIXME: Implement this
> 
> Personally, I prefer notImplemented() to this.

gyuyoung: Informal review, please. :)
Comment 18 Ryuan Choi 2012-07-20 00:16:19 PDT
Looks good to me, too.

It's really important patch for WTR/Efl.

Thank you very much.
Comment 19 Gyuyoung Kim 2012-07-20 00:18:51 PDT
Comment on attachment 153424 [details]
patch

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

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:48
> +    WTREventTypeNone,

I think it is better to initialize with default value in order to recognize enum type value more easily.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:65
> +    EvasMouseButtonNone,

ditto.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:166
> +        evas_event_feed_mouse_wheel(evas, 1, -10, 0, 0);

Why don't you make constant variable for scroll direction ? In this case, it looks -10 can be replaced by SCROLL_LEFT. This is more readable.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:298
> +void EventSenderProxy::dispatchEvent(const WTREvent& wtrEvent)

It seems to me *wtr* prefix is not unneeded in *wtrEvent*.
Comment 20 Gyuyoung Kim 2012-07-20 00:20:08 PDT
(In reply to comment #19)

> It seems to me *wtr* prefix is not unneeded in *wtrEvent*.

Sorry.

s/is not unneeded/is unneeded/g
Comment 21 Kangil Han 2012-07-20 00:26:58 PDT
Comment on attachment 153424 [details]
patch

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

>> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:48
>> +    WTREventTypeNone,
> 
> I think it is better to initialize with default value in order to recognize enum type value more easily.

I am afraid I don't understand fully here. Would you please elaborate more?
Comment 22 Kangil Han 2012-07-20 00:59:36 PDT
Created attachment 153439 [details]
patch
Comment 23 Kangil Han 2012-07-20 00:59:57 PDT
Done, thanks!
Comment 24 Kangil Han 2012-07-20 01:19:04 PDT
Created attachment 153445 [details]
patch
Comment 25 Kangil Han 2012-07-20 01:20:15 PDT
gyuyoung: I've modified all 'wtrEvent' variable to 'event'. Hope this would look better to you. :)
Comment 26 Gyuyoung Kim 2012-07-20 01:24:48 PDT
Comment on attachment 153445 [details]
patch

Looks good to me now. Thanks.
Comment 27 Dominik Röttsches (drott) 2012-07-20 02:21:39 PDT
Kangil, can you test both, before and after your patch with
$ run-webkit-tests -2 --efl --debug 

When I try your patch here, it seems to cause more crashes than it fixes.
Comment 28 Kangil Han 2012-07-20 02:39:19 PDT
(In reply to comment #27)
> Kangil, can you test both, before and after your patch with
> $ run-webkit-tests -2 --efl --debug 
> 
> When I try your patch here, it seems to cause more crashes than it fixes.

I will figure out what happened between them. :)
Comment 29 Dominik Röttsches (drott) 2012-07-20 02:42:54 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > Kangil, can you test both, before and after your patch with
> > $ run-webkit-tests -2 --efl --debug 
> > 
> > When I try your patch here, it seems to cause more crashes than it fixes.
> 
> I will figure out what happened between them. :)

Might not be your patch, I think the assertion failures are related to bug 91719, r123085.
Comment 30 Kangil Han 2012-07-20 03:37:03 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > Kangil, can you test both, before and after your patch with
> > > $ run-webkit-tests -2 --efl --debug 
> > > 
> > > When I try your patch here, it seems to cause more crashes than it fixes.
> > 
> > I will figure out what happened between them. :)
> 
> Might not be your patch, I think the assertion failures are related to bug 91719, r123085.

Got it. No more crashes on my side, release build though. :)
I am going to ask a formal review~
Comment 31 Dominik Röttsches (drott) 2012-07-20 03:59:21 PDT
(In reply to comment #26)
> (From update of attachment 153445 [details])
> Looks good to me now. Thanks.

Checked debug build, works fine and reduces number of crashes, LGTM too.
Comment 32 Kentaro Hara 2012-07-20 04:24:39 PDT
Comment on attachment 153445 [details]
patch

The patch received full LGTMs from EFL guys. The patch also affects the EFL port only. Per the discussion in IRC, let me rubberstamp it.
Comment 33 WebKit Review Bot 2012-07-20 04:31:28 PDT
Comment on attachment 153445 [details]
patch

Clearing flags on attachment: 153445

Committed r123203: <http://trac.webkit.org/changeset/123203>
Comment 34 WebKit Review Bot 2012-07-20 04:31:36 PDT
All reviewed patches have been landed.  Closing bug.