RESOLVED FIXED 91731
[EFL][WK2][WTR] Implement EventSenderProxy
https://bugs.webkit.org/show_bug.cgi?id=91731
Summary [EFL][WK2][WTR] Implement EventSenderProxy
Kangil Han
Reported 2012-07-19 04:20:43 PDT
WK2 EFL needs EventSenderProxy to send events to WebKitTestRunner.
Attachments
patch (21.40 KB, patch)
2012-07-19 05:57 PDT, Kangil Han
no flags
patch (21.41 KB, patch)
2012-07-19 06:32 PDT, Kangil Han
no flags
patch (21.50 KB, patch)
2012-07-19 22:34 PDT, Kangil Han
no flags
patch (21.53 KB, patch)
2012-07-19 23:12 PDT, Kangil Han
no flags
patch (21.50 KB, patch)
2012-07-19 23:20 PDT, Kangil Han
no flags
patch (21.63 KB, patch)
2012-07-20 00:59 PDT, Kangil Han
no flags
patch (21.60 KB, patch)
2012-07-20 01:19 PDT, Kangil Han
no flags
Kangil Han
Comment 1 2012-07-19 05:57:16 PDT
WebKit Review Bot
Comment 2 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.
Kangil Han
Comment 3 2012-07-19 06:32:26 PDT
Gyuyoung Kim
Comment 4 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.
Chris Dumez
Comment 5 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?
Kangil Han
Comment 6 2012-07-19 22:34:56 PDT
Kangil Han
Comment 7 2012-07-19 22:44:27 PDT
It seems that there are around 700 cases using eventSender. All done, thanks!
Chris Dumez
Comment 8 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.
Chris Dumez
Comment 9 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.
Kangil Han
Comment 10 2012-07-19 23:12:35 PDT
Kangil Han
Comment 11 2012-07-19 23:12:48 PDT
Done, thanks!
Chris Dumez
Comment 12 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.
Kangil Han
Comment 13 2012-07-19 23:20:09 PDT
Kangil Han
Comment 14 2012-07-19 23:20:22 PDT
Done, thanks!
Chris Dumez
Comment 15 2012-07-19 23:21:23 PDT
Comment on attachment 153424 [details] patch LGTM. Thanks.
Kangil Han
Comment 16 2012-07-19 23:22:00 PDT
(In reply to comment #15) > (From update of attachment 153424 [details]) > LGTM. Thanks. Thanks for your review!
Kangil Han
Comment 17 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. :)
Ryuan Choi
Comment 18 2012-07-20 00:16:19 PDT
Looks good to me, too. It's really important patch for WTR/Efl. Thank you very much.
Gyuyoung Kim
Comment 19 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*.
Gyuyoung Kim
Comment 20 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
Kangil Han
Comment 21 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?
Kangil Han
Comment 22 2012-07-20 00:59:36 PDT
Kangil Han
Comment 23 2012-07-20 00:59:57 PDT
Done, thanks!
Kangil Han
Comment 24 2012-07-20 01:19:04 PDT
Kangil Han
Comment 25 2012-07-20 01:20:15 PDT
gyuyoung: I've modified all 'wtrEvent' variable to 'event'. Hope this would look better to you. :)
Gyuyoung Kim
Comment 26 2012-07-20 01:24:48 PDT
Comment on attachment 153445 [details] patch Looks good to me now. Thanks.
Dominik Röttsches (drott)
Comment 27 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.
Kangil Han
Comment 28 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. :)
Dominik Röttsches (drott)
Comment 29 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.
Kangil Han
Comment 30 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~
Dominik Röttsches (drott)
Comment 31 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.
Kentaro Hara
Comment 32 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.
WebKit Review Bot
Comment 33 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>
WebKit Review Bot
Comment 34 2012-07-20 04:31:36 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.