Bug 83486 – [EFL][DRT] EventSender needs implementation of leapForward function

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Alexander Shalamov]

EventSender requires implementation of leapForward function in order to test drag'n'drop, wheel and other mouse events.

[Alexander Shalamov]

Created attachment 136421 [details]
Patch

Implementation of leapForward in EFL's DRT EventSender

[WebKit Review Bot]

Attachment 136421 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Tools/DumpRenderTree/efl/EventSender.cpp:135:  Missing space inside { }.  [whitespace/braces] [5]
Tools/DumpRenderTree/efl/EventSender.cpp:137:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/efl/EventSender.cpp:143:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/efl/EventSender.cpp:143:  Missing space inside { }.  [whitespace/braces] [5]
Tools/DumpRenderTree/efl/EventSender.cpp:145:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/efl/EventSender.cpp:151:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/efl/EventSender.cpp:151:  Missing space inside { }.  [whitespace/braces] [5]
Tools/DumpRenderTree/efl/EventSender.cpp:160:  The parameter name "eventInfo" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/efl/EventSender.cpp:161:  The parameter name "eventInfo" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/efl/EventSender.cpp:658:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Tools/DumpRenderTree/efl/EventSender.cpp:666:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 11 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Shalamov]

Created attachment 136427 [details]
Patch

Fixed stylebot errors

[Gyuyoung Kim]

Comment on attachment 136427 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:269
> +    MouseEventInfo* eventInfo = new MouseEventInfo(EvasMouseEventDown, modifiers, (EvasMouseButton)button);

Use static_cast instead of c-type casting.

> Tools/DumpRenderTree/efl/EventSender.cpp:352
> +    const int horizontal = - static_cast<int>(JSValueToNumber(context, arguments[0], exception));

To expression above line more clearly, why don't you modify like this.

int horizontal = -(static_cast<int>(JSValueToNumber(context, arguments[0], exception)));

[Alexander Shalamov]

Created attachment 136499 [details]
Patch

Applied review changes

[Alexander Shalamov]

Created attachment 136723 [details]
Patch

Updated Skipped list.

[Gyuyoung Kim]

Comment on attachment 136723 [details]
Patch

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

It looks this patch is based on GTK port implementation. Could you please explain this patch on irc again ?

> Tools/ChangeLog:6
> +        Implemented leapForward function

I think this patch description is poor beside patch size. Could you write more detail description ?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 136723 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

This line usually comes before the change's longer description.

> Tools/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> Tools/DumpRenderTree/efl/EventSender.cpp:69
> +    ulong delay;

This value is always set from an int below, so why is it a ulong here?

> Tools/DumpRenderTree/efl/EventSender.cpp:75
> +static const int gDelayedQueueSize = 1024;
> +static DelayedEvent gEventQueue[gDelayedQueueSize];
> +static unsigned gStartOfEventQueue;
> +static unsigned gEndOfEventQueue;

Why do all this manual bookkeeping instead of using a WTF::Vector?

> Tools/DumpRenderTree/efl/EventSender.cpp:154
> +    MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers)
> +        : event(event)
> +        , modifiers(modifiers)
> +        , button(EvasMouseButtonNone)
> +        , horizontalDelta(0)
> +        , verticalDelta(0)
> +    {
> +    }
> +
> +    MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, EvasMouseButton button)
> +       : event(event)
> +       , modifiers(modifiers)
> +       , button(button)
> +       , horizontalDelta(0)
> +       , verticalDelta(0)
> +    {
> +    }
> +
> +    MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, int horizontalDelta, int verticalDelta)
> +       : event(event)
> +       , modifiers(modifiers)
> +       , button(EvasMouseButtonNone)
> +       , horizontalDelta(horizontalDelta)
> +       , verticalDelta(verticalDelta)
> +    {
> +    }

This set of constructors could be replaced by a single one:

    MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, EvasMouseButton button = EvasMouseButtonNone, int horizontalDelta = 0, int verticalDelta = 0)

> Tools/DumpRenderTree/efl/EventSender.cpp:659
> +static void feedOrQueueMouseEvent(MouseEventInfo* eventInfo, bool shouldFeedQueuedEvents)

It's not clear to me when a function is supposed to pass true or false to shouldFeeedQueuedEvents. Can you elaborate?

> Tools/DumpRenderTree/efl/EventSender.cpp:661
> +    if ((gDragMode || gStartOfEventQueue != gEndOfEventQueue || gEventQueue[gEndOfEventQueue].delay)

Ditto about the values of gDragMode (ie. why it is in this check).

> Tools/DumpRenderTree/efl/EventSender.cpp:688
> +    // FIXME: We need to pass additional information with our events, so that
> +    // we could construct correct PlatformWheelEvent. At the moment, max number
> +    // of clicks is 3

Are there tests which are broken due to this?

[Alexander Shalamov]

Created attachment 140570 [details]
Patch

Fixed according to review comments

[Alexander Shalamov]

(In reply to comment #8)
> (From update of attachment 136723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136723&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> 
> This line usually comes before the change's longer description.

Fixed
 
> > Tools/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> 
> Ditto.

Fixed

> > Tools/DumpRenderTree/efl/EventSender.cpp:69
> > +    ulong delay;
> 
> This value is always set from an int below, so why is it a ulong here?

Fixed
 
> > Tools/DumpRenderTree/efl/EventSender.cpp:75
> > +static const int gDelayedQueueSize = 1024;
> > +static DelayedEvent gEventQueue[gDelayedQueueSize];
> > +static unsigned gStartOfEventQueue;
> > +static unsigned gEndOfEventQueue;
> 
> Why do all this manual bookkeeping instead of using a WTF::Vector?

Fixed (Tried to follow style:)

> > Tools/DumpRenderTree/efl/EventSender.cpp:154
> > +    MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers)
> > +        : event(event)
> > +        , modifiers(modifiers)
> > +        , button(EvasMouseButtonNone)
> > +        , horizontalDelta(0)
> > +        , verticalDelta(0)
> > +    {
> > +    }
...
> 
> This set of constructors could be replaced by a single one:

Fixed


>     MouseEventInfo(EvasMouseEvent event, EvasKeyModifier modifiers, EvasMouseButton button = EvasMouseButtonNone, int horizontalDelta = 0, int verticalDelta = 0)
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:659
> > +static void feedOrQueueMouseEvent(MouseEventInfo* eventInfo, bool shouldFeedQueuedEvents)
> 
> It's not clear to me when a function is supposed to pass true or false to shouldFeeedQueuedEvents. Can you elaborate?

All mouse events except move should fire all events that are in the queue (same as in other ports).
Usually, test code would programatically press mouse button and then send move command to select or drag element and when button is released, all events with their delays would be re-played in order to simulate user actions.

> > Tools/DumpRenderTree/efl/EventSender.cpp:661
> > +    if ((gDragMode || gStartOfEventQueue != gEndOfEventQueue || gEventQueue[gEndOfEventQueue].delay)
> 
> Ditto about the values of gDragMode (ie. why it is in this check).

Removed, not required at the moment.


> > Tools/DumpRenderTree/efl/EventSender.cpp:688
> > +    // FIXME: We need to pass additional information with our events, so that
> > +    // we could construct correct PlatformWheelEvent. At the moment, max number
> > +    // of clicks is 3
> 
> Are there tests which are broken due to this?

Yes. fast/events/click-count.html
WebKit doesn't care about 1-2-3-ple clicks.
It needs click count, right now, in EFL max number of clicks is 3. GTK has same
limitation, but guys have implemented custom click counter that fixes that issue.

[Alexander Shalamov]

Created attachment 140572 [details]
Patch

Fixed style.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140572 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:66
> +struct MouseEventInfo;

Nit: You could move the DetalyedEvent declaration after MouseEventInfo and avoid this forward declaration here.

> Tools/DumpRenderTree/efl/EventSender.cpp:78
> +static WTF::Vector<DelayedEvent> gEventQueue;

Constructing global static objects this way is dangerous -- in general, we use the DEFINE_STATIC_LOCAL macro for that.

> Tools/DumpRenderTree/efl/EventSender.cpp:148
> +static void feedOrQueueMouseEvent(MouseEventInfo*, bool shouldFeedQueuedEvents = true);

I think this can be made more clear, even if it requires a few more lines of code. A boolean parameter in a function with this name is not very expressive, and someone looking at a place which calls this function for the first time will probably get confused. I suggest using an enum instead of a boolean and removing the default value from it, so you end up calling something like
  feedOrQueueMouseEvent(myMouseEvent, FeedQueuedEvents);
or
  feedOrQueueMouseEvent(myMouseEvent, DoNotFeedQueuedEvents);

> Tools/DumpRenderTree/efl/EventSender.cpp:334
> +    // We need to invert scolling values since in EFL negative z value means that

Nit: scolling -> scrolling

[Alexander Shalamov]

Created attachment 140579 [details]
Patch

Fixed:
* Forward declaration of MouseEventInfo
* Use DEFINE_STATIC_LOCAL instead of "static WTF::Vector<DelayedEvent> gEventQueue;"
* Fix typo: scolling -> scrolling
* Use enum FeedQueuedEvents, DoNotFeedQueuedEvents to make code readable

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140579 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:150
> +DEFINE_STATIC_LOCAL(WTF::Vector<DelayedEvent>, gEventQueue, ());

We're almost there. The idea of DEFINE_STATIC_LOCAL is to use it inside a function; there are example uses in ewk and in WebCore.

[Alexander Shalamov]

Created attachment 140679 [details]
Patch

Moved DEFINE_STATIC_LOCAL to static getter

[Alexander Shalamov]

Created attachment 140689 [details]
Patch

Skipping few tests that were unskipped after rebasing

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140689 [details]
Patch

LGTM now, thanks.

[Eric Seidel (no email)]

Comment on attachment 140689 [details]
Patch

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

Seems reasonable.  rs=me.

> Tools/DumpRenderTree/efl/EventSender.cpp:122
> +struct MouseEventInfo {

Structs tend to very quickly become classes. :)

> Tools/DumpRenderTree/efl/EventSender.cpp:302
> +    MouseEventInfo* eventInfo = new MouseEventInfo(EvasMouseEventMove);

OwnPtr?

> Tools/DumpRenderTree/efl/EventSender.cpp:700
> +    delete eventInfo;

:(  Manual memory management is sad times. :(

> Tools/DumpRenderTree/efl/EventSender.cpp:709
> +            usleep(delayedEvent.delay * 1000);

This is sad.  That we need to artificially slow down the testing?

[WebKit Review Bot]

Comment on attachment 140689 [details]
Patch

Clearing flags on attachment: 140689

Committed r116464: <http://trac.webkit.org/changeset/116464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.