Bug 91731

Created attachment 153239 [details] patch

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.

Created attachment 153243 [details] patch

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 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?

Created attachment 153414 [details] patch

It seems that there are around 700 cases using eventSender. All done, thanks!

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 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.

Created attachment 153423 [details] patch

Done, thanks!

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.

Created attachment 153424 [details] patch

Done, thanks!

Comment on attachment 153424 [details] patch LGTM. Thanks.

(In reply to comment #15) > (From update of attachment 153424 [details]) > LGTM. Thanks. Thanks for your review!

(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. :)

Looks good to me, too. It's really important patch for WTR/Efl. Thank you very much.

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*.

(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 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?

Created attachment 153439 [details] patch

Done, thanks!

Created attachment 153445 [details] patch

gyuyoung: I've modified all 'wtrEvent' variable to 'event'. Hope this would look better to you. :)

Comment on attachment 153445 [details] patch Looks good to me now. Thanks.

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.

(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. :)

(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.

(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~

(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 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 on attachment 153445 [details] patch Clearing flags on attachment: 153445 Committed r123203: <http://trac.webkit.org/changeset/123203>

All reviewed patches have been landed. Closing bug.