Bug 61962 – [EFL] DRT: Add EventSender.{cpp,h}.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Leandro Pereira]

[EFL] DRT: Add EventSender.{cpp,h}.

[Leandro Pereira]

Created attachment 95810 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 95810 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:102
> +    if (!modifiers) {
> +        for (unsigned modifier = 0; modifier < 4; ++modifier)
> +            evas_key_modifier_off(evas, modifierNames[modifier]);
> +    } else {

This is redundant, the rest of the loop has this same behavior when !modifiers.

> Tools/DumpRenderTree/efl/EventSender.cpp:164
> +    if (vertical && horizontal) {
> +        if (event & EVAS_MOUSE_EVENT_SCROLL_UP)
> +            evas_event_feed_mouse_wheel(evas, 0, 10, timestamp, 0);
> +        else
> +            evas_event_feed_mouse_wheel(evas, 0, -10, timestamp, 0);
> +
> +        if (event & EVAS_MOUSE_EVENT_SCROLL_LEFT)
> +            evas_event_feed_mouse_wheel(evas, 1, 10, timestamp, 0);
> +        else
> +            evas_event_feed_mouse_wheel(evas, 1, -10, timestamp, 0);
> +
> +        ++timestamp;
> +    } else {
> +        if (event & EVAS_MOUSE_EVENT_SCROLL_UP)
> +            evas_event_feed_mouse_wheel(evas, 0, 10, timestamp++, 0);
> +        else if (event & EVAS_MOUSE_EVENT_SCROLL_DOWN)
> +            evas_event_feed_mouse_wheel(evas, 0, -10, timestamp++, 0);
> +
> +        if (event & EVAS_MOUSE_EVENT_SCROLL_LEFT)
> +            evas_event_feed_mouse_wheel(evas, 1, 10, timestamp++, 0);
> +        else if (event & EVAS_MOUSE_EVENT_SCROLL_RIGHT)
> +            evas_event_feed_mouse_wheel(evas, 1, -10, timestamp++, 0);
> +    }

Can't this be written more concisely with some smrart ifs to set a value -1 or 1 to be multipled by the 10?

> Tools/DumpRenderTree/efl/EventSender.cpp:173
> +    Evas* evas = evas_object_evas_get(mainFrame);

No free, I assume?  If not, this whole function can be 1 line.

> Tools/DumpRenderTree/efl/EventSender.cpp:197
> +    JSStringRef string = JSValueToStringCopy(context, value, 0);

Don't we have a smart pointer for the JSC API?  We must.  Some flavor of RetainPtr maybe?

> Tools/DumpRenderTree/efl/EventSender.cpp:273
> +    lastClickPositionX = lastMousePositionX;
> +    lastClickPositionY = lastMousePositionY;
> +    lastClickButton = buttonCurrentlyDown;
> +    lastClickTimeOffset = timeOffset;

Are these globals?  Can we name them in such a way to distinguish that?

> Tools/DumpRenderTree/efl/EventSender.cpp:317
> +    unsigned mouseEvent = 0;
> +    if (vertical > 0)
> +        mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_UP;
> +    else if (vertical < 0)
> +        mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_DOWN;
> +
> +    if (horizontal > 0)
> +        mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_RIGHT;
> +    else if (horizontal < 0)
> +        mouseEvent |= EVAS_MOUSE_EVENT_SCROLL_LEFT;

I might have made this a helper function.

> Tools/DumpRenderTree/efl/EventSender.cpp:357
> +    if (location == DOM_KEY_LOCATION_NUMPAD) {
> +        if (JSStringIsEqualToUTF8CString(character, "leftArrow"))
> +            keyName = "KP_Left";
> +        else if (JSStringIsEqualToUTF8CString(character, "rightArrow"))
> +            keyName = "KP_Right";
> +        else if (JSStringIsEqualToUTF8CString(character, "upArrow"))
> +            keyName = "KP_Up";
> +        else if (JSStringIsEqualToUTF8CString(character, "downArrow"))
> +            keyName = "KP_Down";
> +        else if (JSStringIsEqualToUTF8CString(character, "pageUp"))

I definitely would have made these two huge if blocks helper functions. :)

> Tools/DumpRenderTree/efl/EventSender.cpp:466
> +    zoomIn(FALSE);

Bools are a horrible way to specify behavior.  They're unreadable at teh callsite.  Enums are much preferred.  But I'm nto sure you even want such.  This is dead code at the moment and woudl be better to just remove for now.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 97331 [details]
EventSender, take 2

This version should hopefully be more readable. check-webkit-style will raise a false positive due to context being called context in EventSender.h.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/e..." exit_code: 1

Tools/DumpRenderTree/efl/EventSender.h:38:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 97334 [details]
Take 2, now pleasing the style bot

[Raphael Kubo da Costa (:rakuco)]

Eric, is there anything you'd still like us to change in the patch?

[Eric Seidel (no email)]

Comment on attachment 97334 [details]
Take 2, now pleasing the style bot

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

> Tools/DumpRenderTree/efl/EventSender.cpp:68
> +    DOM_KEY_LOCATION_STANDARD = 0x00,
> +    DOM_KEY_LOCATION_LEFT     = 0x01,
> +    DOM_KEY_LOCATION_RIGHT    = 0x02,
> +    DOM_KEY_LOCATION_NUMPAD   = 0x03

Why define the values in hex?

> Tools/DumpRenderTree/efl/EventSender.cpp:96
> +enum EvasKeyModifier {
> +    EVAS_KEY_MODIFIER_NONE    = 0,
> +    EVAS_KEY_MODIFIER_CONTROL = 1 << 0,
> +    EVAS_KEY_MODIFIER_SHIFT   = 1 << 1,
> +    EVAS_KEY_MODIFIER_ALT     = 1 << 2,
> +    EVAS_KEY_MODIFIER_META    = 1 << 3
> +};
> +
> +enum EvasMouseButton {
> +    EVAS_MOUSE_BUTTON_NONE   = 0x00,
> +    EVAS_MOUSE_BUTTON_LEFT   = 0x01,
> +    EVAS_MOUSE_BUTTON_MIDDLE = 0x02,
> +    EVAS_MOUSE_BUTTON_RIGHT  = 0x03
> +};
> +
> +enum EvasMouseEvent {
> +    EVAS_MOUSE_EVENT_NONE         = 0,
> +    EVAS_MOUSE_EVENT_DOWN         = 1 << 0,
> +    EVAS_MOUSE_EVENT_UP           = 1 << 1,
> +    EVAS_MOUSE_EVENT_MOVE         = 1 << 2,
> +    EVAS_MOUSE_EVENT_SCROLL_UP    = 1 << 3,
> +    EVAS_MOUSE_EVENT_SCROLL_DOWN  = 1 << 4,
> +    EVAS_MOUSE_EVENT_SCROLL_LEFT  = 1 << 5,
> +    EVAS_MOUSE_EVENT_SCROLL_RIGHT = 1 << 6,
> +    EVAS_MOUSE_EVENT_CLICK        = EVAS_MOUSE_EVENT_MOVE | EVAS_MOUSE_EVENT_DOWN | EVAS_MOUSE_EVENT_UP,
> +};

Are these enums which are defined in some EFL header which we should be importing instead of duplicating?

> Tools/DumpRenderTree/efl/EventSender.cpp:179
> +    if (JSStringIsEqualToUTF8CString(jsKeyValue.get(), "ctrlKey") || JSStringIsEqualToUTF8CString(jsKeyValue.get(), "addSelectionKey"))

I feel like we talked about some sort of equals() helper in one of these patches.

[Eric Seidel (no email)]

I don't really want to be involved in EFL patches anymore.  The c-style code is just depressing.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> (From update of attachment 97334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97334&action=review
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:68
> > +    DOM_KEY_LOCATION_STANDARD = 0x00,
> > +    DOM_KEY_LOCATION_LEFT     = 0x01,
> > +    DOM_KEY_LOCATION_RIGHT    = 0x02,
> > +    DOM_KEY_LOCATION_NUMPAD   = 0x03
> 
> Why define the values in hex?

Fixed locally.

> > Tools/DumpRenderTree/efl/EventSender.cpp:96
> > +enum EvasKeyModifier {
> > +    EVAS_KEY_MODIFIER_NONE    = 0,
> > +    EVAS_KEY_MODIFIER_CONTROL = 1 << 0,
> > +    EVAS_KEY_MODIFIER_SHIFT   = 1 << 1,
> > +    EVAS_KEY_MODIFIER_ALT     = 1 << 2,
> > +    EVAS_KEY_MODIFIER_META    = 1 << 3
> > +};
> > +
> > +enum EvasMouseButton {
> > +    EVAS_MOUSE_BUTTON_NONE   = 0x00,
> > +    EVAS_MOUSE_BUTTON_LEFT   = 0x01,
> > +    EVAS_MOUSE_BUTTON_MIDDLE = 0x02,
> > +    EVAS_MOUSE_BUTTON_RIGHT  = 0x03
> > +};
> > +
> > +enum EvasMouseEvent {
> > +    EVAS_MOUSE_EVENT_NONE         = 0,
> > +    EVAS_MOUSE_EVENT_DOWN         = 1 << 0,
> > +    EVAS_MOUSE_EVENT_UP           = 1 << 1,
> > +    EVAS_MOUSE_EVENT_MOVE         = 1 << 2,
> > +    EVAS_MOUSE_EVENT_SCROLL_UP    = 1 << 3,
> > +    EVAS_MOUSE_EVENT_SCROLL_DOWN  = 1 << 4,
> > +    EVAS_MOUSE_EVENT_SCROLL_LEFT  = 1 << 5,
> > +    EVAS_MOUSE_EVENT_SCROLL_RIGHT = 1 << 6,
> > +    EVAS_MOUSE_EVENT_CLICK        = EVAS_MOUSE_EVENT_MOVE | EVAS_MOUSE_EVENT_DOWN | EVAS_MOUSE_EVENT_UP,
> > +};
> 
> Are these enums which are defined in some EFL header which we should be importing instead of duplicating?

No.

> > Tools/DumpRenderTree/efl/EventSender.cpp:179
> > +    if (JSStringIsEqualToUTF8CString(jsKeyValue.get(), "ctrlKey") || JSStringIsEqualToUTF8CString(jsKeyValue.get(), "addSelectionKey"))
> 
> I feel like we talked about some sort of equals() helper in one of these patches.

Fixed locally.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 98010 [details]
Patch

[Kent Tamura]

Comment on attachment 98010 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:294
> +    int horizontal = (int)JSValueToNumber(context, arguments[0], exception);
> +    if (exception && *exception)
> +        return JSValueMakeUndefined(context);
> +    int vertical = (int)JSValueToNumber(context, arguments[1], exception);

C-style cast is not recommended in WebKit.

> Tools/DumpRenderTree/efl/EventSender.h:38
> +JSObjectRef makeEventSender(JSContextRef, bool);

We avoid using bool for a function parameter, and use enum instead.
If you need to use bool, you should write an explanation of the argument in the header.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 98717 [details]
New patch following Kent's suggestions

[Raphael Kubo da Costa (:rakuco)]

CC'ing Kent after adding a new patch after his suggestions.

[Kent Tamura]

Comment on attachment 98717 [details]
New patch following Kent's suggestions

Rubber-stamped by tkent

[WebKit Review Bot]

Comment on attachment 98717 [details]
New patch following Kent's suggestions

Clearing flags on attachment: 98717

Committed r89820: <http://trac.webkit.org/changeset/89820>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.