WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
patch
(21.41 KB, patch)
2012-07-19 06:32 PDT
,
Kangil Han
no flags
Details
Formatted Diff
Diff
patch
(21.50 KB, patch)
2012-07-19 22:34 PDT
,
Kangil Han
no flags
Details
Formatted Diff
Diff
patch
(21.53 KB, patch)
2012-07-19 23:12 PDT
,
Kangil Han
no flags
Details
Formatted Diff
Diff
patch
(21.50 KB, patch)
2012-07-19 23:20 PDT
,
Kangil Han
no flags
Details
Formatted Diff
Diff
patch
(21.63 KB, patch)
2012-07-20 00:59 PDT
,
Kangil Han
no flags
Details
Formatted Diff
Diff
patch
(21.60 KB, patch)
2012-07-20 01:19 PDT
,
Kangil Han
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Kangil Han
Comment 1
2012-07-19 05:57:16 PDT
Created
attachment 153239
[details]
patch
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
Created
attachment 153243
[details]
patch
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
Created
attachment 153414
[details]
patch
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
Created
attachment 153423
[details]
patch
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
Created
attachment 153424
[details]
patch
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
Created
attachment 153439
[details]
patch
Kangil Han
Comment 23
2012-07-20 00:59:57 PDT
Done, thanks!
Kangil Han
Comment 24
2012-07-20 01:19:04 PDT
Created
attachment 153445
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug