[webkit-reviews] review denied: [Bug 61962] [EFL] DRT: Add EventSender.{cpp, h}. : [Attachment 95810] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Leandro Pereira
<leandro at profusion.mobi>'s request for review:
Bug 61962: [EFL] DRT: Add EventSender.{cpp,h}.
https://bugs.webkit.org/show_bug.cgi?id=61962

Attachment 95810: Patch
https://bugs.webkit.org/attachment.cgi?id=95810&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.