[Webkit-unassigned] [Bug 79601] [EFL] [DRT] Implement scheduleAsynchronousKeyDown.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79601


Raphael Kubo da Costa <kubo at profusion.mobi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128912|review?                     |review-
               Flag|                            |




--- Comment #2 from Raphael Kubo da Costa <kubo at profusion.mobi>  2012-02-26 13:25:20 PST ---
(From update of attachment 128912)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.