Bug 91731 – [EFL][WK2][WTR] Implement EventSenderProxy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

WK2 EFL needs EventSenderProxy to send events to WebKitTestRunner.

[Kangil Han]

Created attachment 153239 [details]
patch

[WebKit Review Bot]

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]

Created attachment 153243 [details]
patch

[Gyuyoung Kim]

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

Created attachment 153414 [details]
patch

[Kangil Han]

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

[Chris Dumez]

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

Created attachment 153423 [details]
patch

[Kangil Han]

Done, thanks!

[Chris Dumez]

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]

Created attachment 153424 [details]
patch

[Kangil Han]

Done, thanks!

[Chris Dumez]

Comment on attachment 153424 [details]
patch

LGTM. Thanks.

[Kangil Han]

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

Thanks for your review!

[Kangil Han]

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

Looks good to me, too.

It's really important patch for WTR/Efl.

Thank you very much.

[Gyuyoung Kim]

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]

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

Created attachment 153439 [details]
patch

[Kangil Han]

Done, thanks!

[Kangil Han]

Created attachment 153445 [details]
patch

[Kangil Han]

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

[Gyuyoung Kim]

Comment on attachment 153445 [details]
patch

Looks good to me now. Thanks.

[Dominik Röttsches (drott)]

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]

(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)]

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

(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)]

(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 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 on attachment 153445 [details]
patch

Clearing flags on attachment: 153445

Committed r123203: <http://trac.webkit.org/changeset/123203>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.