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

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from ChangSeok Oh <kevin.cs.oh at gmail.com>  2012-03-08 05:08:40 PST ---
(From update of attachment 128912)
View in context: https://bugs.webkit.org/attachment.cgi?id=128912&action=review

Thanks for comments :)

>> Tools/DumpRenderTree/efl/EventSender.cpp:109
>> +} KeyEventInfo;
> 
> This is C++, declaring the struct with "struct KeyEventInfo" works just fine.

Done.

>> 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.

Done.

>> Tools/DumpRenderTree/efl/EventSender.cpp:428
>> +        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.

Replaced it to 'new'.

>> Tools/DumpRenderTree/efl/EventSender.cpp:431
>> +        keyEventInfo->modifiers = modifiersFromJSValue(context, arguments[1]);
> 
> You need to set 'modifiers' if argumentCount < 2 too, otherwise it'll have a random value.

Initialized it with EvasKeyModifierNone.

>> 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.

Done.

>> Tools/DumpRenderTree/efl/EventSender.cpp:460
>> +    free(keyEventInfo);
> 
> When you change malloc with new. You have to change free() with delete as well.

Replaced it to OwnPtr. So we don't need to free manually.

>> 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.

Done.

>> 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.

Done.

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