Bug 61962 - [EFL] DRT: Add EventSender.{cpp,h}.
Summary: [EFL] DRT: Add EventSender.{cpp,h}.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Pereira
URL:
Keywords:
Depends on: 61974 62034
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-02 14:16 PDT by Leandro Pereira
Modified: 2011-06-27 08:11 PDT (History)
3 users (show)

See Also:


Attachments
Patch (26.75 KB, patch)
2011-06-02 14:21 PDT, Leandro Pereira
eric: review-
Details | Formatted Diff | Diff
EventSender, take 2 (24.07 KB, patch)
2011-06-15 11:18 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Take 2, now pleasing the style bot (24.06 KB, patch)
2011-06-15 11:25 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (27.24 KB, patch)
2011-06-21 10:49 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
New patch following Kent's suggestions (23.35 KB, patch)
2011-06-27 06:59 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Pereira 2011-06-02 14:16:57 PDT
[EFL] DRT: Add EventSender.{cpp,h}.
Comment 1 Leandro Pereira 2011-06-02 14:21:27 PDT
Created attachment 95810 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-10 12:35:25 PDT
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.
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-06-15 11:18:22 PDT
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.
Comment 4 WebKit Review Bot 2011-06-15 11:20:28 PDT
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.
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-06-15 11:25:21 PDT
Created attachment 97334 [details]
Take 2, now pleasing the style bot
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-06-20 07:16:38 PDT
Eric, is there anything you'd still like us to change in the patch?
Comment 7 Eric Seidel (no email) 2011-06-20 11:17:30 PDT
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.
Comment 8 Eric Seidel (no email) 2011-06-20 11:17:58 PDT
I don't really want to be involved in EFL patches anymore.  The c-style code is just depressing.
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-06-21 05:48:29 PDT
(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.
Comment 10 Raphael Kubo da Costa (:rakuco) 2011-06-21 10:49:26 PDT
Created attachment 98010 [details]
Patch
Comment 11 Kent Tamura 2011-06-23 02:35:10 PDT
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.
Comment 12 Raphael Kubo da Costa (:rakuco) 2011-06-27 06:59:56 PDT
Created attachment 98717 [details]
New patch following Kent's suggestions
Comment 13 Raphael Kubo da Costa (:rakuco) 2011-06-27 07:26:17 PDT
CC'ing Kent after adding a new patch after his suggestions.
Comment 14 Kent Tamura 2011-06-27 07:29:47 PDT
Comment on attachment 98717 [details]
New patch following Kent's suggestions

Rubber-stamped by tkent
Comment 15 WebKit Review Bot 2011-06-27 08:11:10 PDT
Comment on attachment 98717 [details]
New patch following Kent's suggestions

Clearing flags on attachment: 98717

Committed r89820: <http://trac.webkit.org/changeset/89820>
Comment 16 WebKit Review Bot 2011-06-27 08:11:16 PDT
All reviewed patches have been landed.  Closing bug.