[webkit-reviews] review denied: [Bug 79601] [EFL] [DRT] Implement scheduleAsynchronousKeyDown. : [Attachment 128912] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 79601: [EFL] [DRT] Implement scheduleAsynchronousKeyDown.
https://bugs.webkit.org/show_bug.cgi?id=79601

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

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
View in context: https://bugs.webkit.org/attachment.cgi?id=128912&action=review


> Tools/DumpRenderTree/efl/EventSender.cpp:109
> +typedef struct {
> +    EvasKeyModifier modifiers;
> +    const char* keyName;
> +} KeyEventInfo;

This is C++, declaring the struct with "struct KeyEventInfo" works just fine.

> Tools/DumpRenderTree/efl/EventSender.cpp:409
> +static KeyEventInfo* createKeyEventInfo(JSContextRef context, size_t
argumentCount, const JSValueRef arguments[], JSValueRef* exception)

I'd rather have this function return a PassOwnPtr so you don't need to worry
about manual memory handling.

> Tools/DumpRenderTree/efl/EventSender.cpp:428
> +    KeyEventInfo* keyEventInfo =
static_cast<KeyEventInfo*>(malloc(sizeof(KeyEventInfo)));
> +    if (!keyEventInfo)
> +	   return 0;

Please don't use malloc in C++ code unless there's no other option; using "new"
also brings the benefit of not having to check for its return, as it never
returns NULL.

> Tools/DumpRenderTree/efl/EventSender.cpp:431
>      if (argumentCount >= 2)
> -	   setEvasModifiers(evas, modifiersFromJSValue(context, arguments[1]));

> +	   keyEventInfo->modifiers = modifiersFromJSValue(context,
arguments[1]);

You need to set 'modifiers' if argumentCount < 2 too, otherwise it'll have a
random value.

> Tools/DumpRenderTree/efl/EventSender.cpp:446
> +    if (evas) {

The code currently doesn't check for that, and evas_object_evas_get() shouln't
return NULL in normal conditions. I'd replace the checks for keyEventInfo and
evas with an ASSERT.

> Tools/DumpRenderTree/efl/EventSender.cpp:545
> +    return FALSE;

You shouldn't use FALSE in C++ code, false exits for the same purpose. In this
case, it's better to return ECORE_CALLBACK_CANCEL to be explicit.

> Tools/DumpRenderTree/efl/EventSender.cpp:551
> +    if (keyEventInfo)

You're not checking if it's not 0 in keyDownCallback, either check if there too
or remove the check here.