Bug 79601 – [EFL] [DRT] Implement scheduleAsynchronousKeyDown.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[ChangSeok Oh]

EFL port DRT doesn't support scheduleAsynchronousKeyDown api.

[ChangSeok Oh]

Created attachment 128912 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 128912 [details]
Patch

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.

[Gyuyoung Kim]

Comment on attachment 128912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128912&action=review

By the way, can't this patch remove skipped test cases? If so, you need to update Skipped file.

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

It looks it is better to add an empty line to here.

> Tools/DumpRenderTree/efl/EventSender.cpp:460
> +    free(keyEventInfo);

When you change malloc with new. You have to change free() with delete as well.

[ChangSeok Oh]

Created attachment 130807 [details]
Patch

[ChangSeok Oh]

Comment on attachment 128912 [details]
Patch

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.

[Gyuyoung Kim]

Comment on attachment 130807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130807&action=review

> Tools/DumpRenderTree/efl/EventSender.cpp:540
> +static Eina_Bool sendAsynchronousKeyDown(void* userData)

We need to use standard boolean instead of Eina_Bool.

[ChangSeok Oh]

(In reply to comment #6)
> (From update of attachment 130807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130807&action=review
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:540
> > +static Eina_Bool sendAsynchronousKeyDown(void* userData)
> 
> We need to use standard boolean instead of Eina_Bool.

Do you mean we should replace all Eina_Bool to bool that has been used?
sendClick has also used Eina_Bool in the same file. There are so many places using Eina_Bool, too. I just hope to make a consistence. 
In my opinion, using Eina_Bool is proper here. Because sendClick & sendAsynchronousKeydown are called by EFL, so we need to meet EFL's interface.
For such reasons, GTK port also uses its own type, gboolean.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 130807 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=130807&action=review
> > 
> > > Tools/DumpRenderTree/efl/EventSender.cpp:540
> > > +static Eina_Bool sendAsynchronousKeyDown(void* userData)
> > 
> > We need to use standard boolean instead of Eina_Bool.
> 
> Do you mean we should replace all Eina_Bool to bool that has been used?

Please let's not have this discussion again for the 38213921th time :-(

This is a minor issue that's not really related to this patch; I think we can just leave it as is and not worry about this for now.

[Gyuyoung Kim]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 130807 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=130807&action=review
> > > 
> > > > Tools/DumpRenderTree/efl/EventSender.cpp:540
> > > > +static Eina_Bool sendAsynchronousKeyDown(void* userData)
> > > 
> > > We need to use standard boolean instead of Eina_Bool.
> > 
> > Do you mean we should replace all Eina_Bool to bool that has been used?
> 
> Please let's not have this discussion again for the 38213921th time :-(
> 
> This is a minor issue that's not really related to this patch; I think we can just leave it as is and not worry about this for now.

I don't want to file bugs related to coding style in future again. So, IMHO, we need to review coding style.

[Gyuyoung Kim]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 130807 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=130807&action=review
> > 
> > > Tools/DumpRenderTree/efl/EventSender.cpp:540
> > > +static Eina_Bool sendAsynchronousKeyDown(void* userData)
> > 
> > We need to use standard boolean instead of Eina_Bool.
> 
> Do you mean we should replace all Eina_Bool to bool that has been used?
> sendClick has also used Eina_Bool in the same file. There are so many places using Eina_Bool, too. I just hope to make a consistence. 
> In my opinion, using Eina_Bool is proper here. Because sendClick & sendAsynchronousKeydown are called by EFL, so we need to meet EFL's interface.
> For such reasons, GTK port also uses its own type, gboolean.

Anyway, let's do not discuss coding style in here anymore. I'm ok for now.

[Gustavo Noronha (kov)]

Comment on attachment 130807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130807&action=review

> Tools/DumpRenderTree/efl/EventSender.cpp:463
> +    OwnPtr<KeyEventInfo> keyEventInfo = createKeyEventInfo(context, argumentCount, arguments, exception);

This will need the adoptPtr(), see bellow.

> Tools/DumpRenderTree/efl/EventSender.cpp:464
> +    sendKeyDown(evas_object_evas_get(browser->mainFrame()), keyEventInfo.release());

This should be keyEventInfo.get()

> Tools/DumpRenderTree/efl/EventSender.cpp:543
> +    sendKeyDown(evas_object_evas_get(browser->mainFrame()), keyEventInfo.release());

This should be keyEventInfo.get().

> Tools/DumpRenderTree/efl/EventSender.cpp:550
> +    OwnPtr<KeyEventInfo> keyEventInfo = createKeyEventInfo(context, argumentCount, arguments, exception);
> +    ecore_idler_add(sendAsynchronousKeyDown, static_cast<void*>(keyEventInfo.leakPtr()));

Churn =( I think it makes more sense to have createKeyEventInfo return a normal pointer, you're essentially not using this OwnPtr here.

[ChangSeok Oh]

Created attachment 131052 [details]
Patch

[ChangSeok Oh]

Comment on attachment 130807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130807&action=review

Thank you for the review! :)

>> Tools/DumpRenderTree/efl/EventSender.cpp:463
>> +    OwnPtr<KeyEventInfo> keyEventInfo = createKeyEventInfo(context, argumentCount, arguments, exception);
> 
> This will need the adoptPtr(), see bellow.

Done.

>> Tools/DumpRenderTree/efl/EventSender.cpp:464
>> +    sendKeyDown(evas_object_evas_get(browser->mainFrame()), keyEventInfo.release());
> 
> This should be keyEventInfo.get()

Done.

>> Tools/DumpRenderTree/efl/EventSender.cpp:543
>> +    sendKeyDown(evas_object_evas_get(browser->mainFrame()), keyEventInfo.release());
> 
> This should be keyEventInfo.get().

Done.

>> Tools/DumpRenderTree/efl/EventSender.cpp:550
>> +    ecore_idler_add(sendAsynchronousKeyDown, static_cast<void*>(keyEventInfo.leakPtr()));
> 
> Churn =( I think it makes more sense to have createKeyEventInfo return a normal pointer, you're essentially not using this OwnPtr here.

Done.

[WebKit Review Bot]

Comment on attachment 131052 [details]
Patch

Clearing flags on attachment: 131052

Committed r110568: <http://trac.webkit.org/changeset/110568>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.