Bug 79601 - [EFL] [DRT] Implement scheduleAsynchronousKeyDown.
Summary: [EFL] [DRT] Implement scheduleAsynchronousKeyDown.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-26 01:15 PST by ChangSeok Oh
Modified: 2012-03-13 08:41 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.81 KB, patch)
2012-02-26 01:57 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (11.16 KB, patch)
2012-03-08 04:59 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (11.11 KB, patch)
2012-03-09 10:10 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ChangSeok Oh 2012-02-26 01:15:41 PST
EFL port DRT doesn't support scheduleAsynchronousKeyDown api.
Comment 1 ChangSeok Oh 2012-02-26 01:57:03 PST
Created attachment 128912 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-02-26 13:25:20 PST
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.
Comment 3 Gyuyoung Kim 2012-02-26 18:09:50 PST
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.
Comment 4 ChangSeok Oh 2012-03-08 04:59:45 PST
Created attachment 130807 [details]
Patch
Comment 5 ChangSeok Oh 2012-03-08 05:08:40 PST
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.
Comment 6 Gyuyoung Kim 2012-03-08 17:16:10 PST
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.
Comment 7 ChangSeok Oh 2012-03-08 17:49:49 PST
(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.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-03-08 18:29:34 PST
(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.
Comment 9 Gyuyoung Kim 2012-03-08 19:01:06 PST
(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.
Comment 10 Gyuyoung Kim 2012-03-08 19:04:41 PST
(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.
Comment 11 Gustavo Noronha (kov) 2012-03-09 08:33:47 PST
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.
Comment 12 ChangSeok Oh 2012-03-09 10:10:48 PST
Created attachment 131052 [details]
Patch
Comment 13 ChangSeok Oh 2012-03-09 10:12:35 PST
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.
Comment 14 WebKit Review Bot 2012-03-13 08:41:07 PDT
Comment on attachment 131052 [details]
Patch

Clearing flags on attachment: 131052

Committed r110568: <http://trac.webkit.org/changeset/110568>
Comment 15 WebKit Review Bot 2012-03-13 08:41:29 PDT
All reviewed patches have been landed.  Closing bug.