Bug 43446 – [EFL] Exporting JavaScript objects for EFL port

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jonas Gastal]

Exporting JavaScript objects for EFL port

[Jonas Gastal]

Created attachment 63381 [details]
Patch

[WebKit Review Bot]

Attachment 63381 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ewk/ewk_view.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonas Gastal]

Created attachment 63383 [details]
Patch

[WebKit Review Bot]

Attachment 63383 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/EWebLauncher/main.c:86:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebKit/efl/EWebLauncher/main.c:94:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:99:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:104:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:112:  js_meta is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonas Gastal]

Created attachment 64592 [details]
Adds support for exporting a JS API to EFL port.

[Jonas Gastal]

Created attachment 64593 [details]
Example code for injecting JS API.

[WebKit Review Bot]

Attachment 64592 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ewk/ewk_js.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/efl/ewk/ewk_js.cpp:389:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 64593 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/EWebLauncher/main.c:93:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:98:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:103:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:111:  js_meta is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonas Gastal]

As far as I can tell all the style complaints from check-webkit-style are false positives.

[Kenneth Rohde Christiansen]

Comment on attachment 64592 [details]
Adds support for exporting a JS API to EFL port.

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

> WebKit/efl/ChangeLog:171
>          * DefaultTheme/widget/slider/slider_v.png: Copied from WebKit/efl/DefaultTheme/widget/scrollbar/scrollbar_v.png.
>  
> [phone]  Jonas M. Gastal  <[e-mail]>
> +

This should be the first entry of the ChangeLog

[Jonas Gastal]

Created attachment 71165 [details]
Adds support for exporting a JS API to EFL port.

Fixed ChangeLog as request by kenneth.

[Jonas Gastal]

Created attachment 71166 [details]
Example code for injecting JS API.

Fixed ChangeLog as requested by kenneth.

[Jonas Gastal]

Could some one please review these patches?

[Kent Tamura]

Comment on attachment 71165 [details]
Adds support for exporting a JS API to EFL port.

The patch looks not to be applied to the trunk cleanly.  Would you update it please?

[Kent Tamura]

Comment on attachment 71166 [details]
Example code for injecting JS API.

ditto.

[Lucas De Marchi]

Hi Jonas.


When you have time, please upload a new patch. I'm closing this one now, feel free to re-open it later.

Thanks

[Flavio Ceolin]

Created attachment 97602 [details]
Patch

It's the Jonas Gastal's patch rebased with the upstream and with small fixes to be possible build with NETSCAPE_PLUGIN_API disabled.
Could some one please review this patch ?


Regards, Ceolin.

[WebKit Review Bot]

Attachment 97602 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/CMakeLists.txt', u'Source/We..." exit_code: 1

Source/WebKit/efl/ewk/ewk_js.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/efl/ewk/ewk_js.cpp:127:  Declaration has space between type name and * in JavaScriptObject *jso  [whitespace/declaration] [3]
Source/WebKit/efl/ewk/ewk_js.cpp:404:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebKit/efl/ewk/ewk_js.cpp:716:  Declaration has space between type name and * in Eina_Hash *ewk_js_object_properties_get  [whitespace/declaration] [3]
Source/WebKit/efl/ewk/ewk_js.cpp:718:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/efl/ewk/ewk_js.cpp:723:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/efl/ewk/ewk_js.cpp:734:  Declaration has space between type name and * in Ewk_JS_Object *ewk_js_object_new  [whitespace/declaration] [3]
Source/WebKit/efl/ewk/ewk_js.cpp:736:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/efl/ewk/ewk_private.h:65:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 9 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Leandro Pereira]

Reopening the bug, as a new patch has been submitted.

[Flavio Ceolin]

Created attachment 97605 [details]
Patch

Fixed code style errors.

Regards, Ceolin

[Gyuyoung Kim]

It seems to me goal of this bug is for WebOS, right ? If you mention why this patch is needed, it can be helpful to review this bug.

[Flavio Ceolin]

(In reply to comment #21)
> It seems to me goal of this bug is for WebOS, right ? If you mention why this patch is needed, it can be helpful to review this bug.

It is not for WebOS, others ports like qt already has this functionality. The goal of this patch is the ewebkit be able to create rich webwidgets.

Regards, Ceolin

[Gyuyoung Kim]

Comment on attachment 97605 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_js.cpp:21
> +

You don't need to add empty line to here.

> Source/WebKit/efl/ewk/ewk_js.cpp:93
> +       return (Ewk_JS_Object*)np_obj;

Please adhere indentation.

> Source/WebKit/efl/ewk/ewk_js.cpp:96
> +        return 0;

ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:714
> +#else

Personally, I don't like to make dummy functions for macro. Other ewk functions are using macro its inside for disabling macro.

const char *ewk_settings_web_database_path_get(void)
{
#if ENABLE(DATABASE)
    return _ewk_default_web_database_path;
#else
    return 0;
#endif
}

> Source/WebKit/efl/ewk/ewk_private.h:1
> +

Please remove empty line.

[Flavio Ceolin]

Created attachment 98225 [details]
Patch

[Flavio Ceolin]

(In reply to comment #23)
> (From update of attachment 97605 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97605&action=review
> 
> > Source/WebKit/efl/ewk/ewk_js.cpp:21
> > +
> 
> You don't need to add empty line to here.

Fixed

> 
> > Source/WebKit/efl/ewk/ewk_js.cpp:93
> > +       return (Ewk_JS_Object*)np_obj;
> 

Fixed

> Please adhere indentation.
> 
> > Source/WebKit/efl/ewk/ewk_js.cpp:96
> > +        return 0;
> 
> ditto.
>
I couldn't see this mistake. The indentation seems to be right.
 
> > Source/WebKit/efl/ewk/ewk_js.cpp:714
> > +#else
> 
> Personally, I don't like to make dummy functions for macro. Other ewk functions are using macro its inside for disabling macro.
> 
> const char *ewk_settings_web_database_path_get(void)
> {
> #if ENABLE(DATABASE)
>     return _ewk_default_web_database_path;
> #else
>     return 0;
> #endif
> }
> 
> > Source/WebKit/efl/ewk/ewk_private.h:1
> > +
> 
> Please remove empty line.

Fixed

I sent a new patch fixing the mistakes. About the dummy functions, my point is 
that will be necessary too many #if without them. To my mind the dummy 
functions is better in this case (although I don't like too). If you still 
prefer the #if inside the functions I can send a new patch.

Thanks for the advices :)

Regards, Ceolin

[Leandro Pereira]

Comment on attachment 98225 [details]
Patch

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

Informal r-.

> Source/WebKit/ChangeLog:8
> [phone]  Jonas M. Gastal <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Exporting JavaScript objects for EFL port
> +        https://bugs.webkit.org/show_bug.cgi?id=43446
> +
> +        * CMakeLists.txt:

Be more clear here: what is being changed in CMakeLists.txt as a whole and what specific change has been performed.

> Source/WebKit/efl/ChangeLog:2
> [phone]  Jonas M. Gastal <[e-mail]>
> +        Reviewed by NOBODY (OOPS!).

Usually, there is an empty line after the date/name/mail line.

> Source/WebKit/efl/ChangeLog:7
> +        Exporting JavaScript objects for EFL port
> +        https://bugs.webkit.org/show_bug.cgi?id=43446
> +
> +        * CMakeListsEfl.txt:

A paragraph explaining in detail would be nice.

> Source/WebKit/efl/ewk/ewk_js.cpp:78
> +    /*NPAllocateFunctionPtr*/ 0,
> +    /*NPDeallocateFunctionPtr*/ 0,
> +    /*NPInvalidateFunctionPtr*/ 0,
> +    /*NPHasMethodFunctionPtr*/ _ewk_js_method_has,
> +    /*NPInvokeFunctionPtr*/ _ewk_js_method_invoke,
> +    /*NPInvokeDefaultFunctionPtr*/ 0,
> +    /*NPHasPropertyFunctionPtr*/ _ewk_js_property_has,
> +    /*NPGetPropertyFunctionPtr*/ _ewk_js_property_get,
> +    /*NPSetPropertyFunctionPtr*/ _ewk_js_property_set,
> +    /*NPRemovePropertyFunctionPtr*/ _ewk_js_property_remove,
> +    /*NPEnumerationFunctionPtr*/ _ewk_js_properties_enumerate,
> +    /*NPConstructFunctionPtr*/ 0

C++ comments, please.

> Source/WebKit/efl/ewk/ewk_js.cpp:92
> +    if (EINA_MAGIC_CHECK((Ewk_JS_Object*)np_obj, EWK_JS_OBJECT_MAGIC)) // It already is a Ewk_JS_Object
> +        return (Ewk_JS_Object*)np_obj;

Use C++ casts on .cpp files, please. Also, is that comment really needed?

> Source/WebKit/efl/ewk/ewk_js.cpp:97
> +    cls = (Ewk_JS_Class*)malloc(sizeof(Ewk_JS_Class));

What happens if malloc() fails?

> Source/WebKit/efl/ewk/ewk_js.cpp:120
> +    obj = (Ewk_JS_Object*)malloc(sizeof(Ewk_JS_Object));

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:130
> +    if (!strncmp("Array", jso->imp->className().ascii().data(), strlen("Array")))
> +        obj->type = EWK_JS_OBJECT_ARRAY;
> +    else if (!strncmp("Function", jso->imp->className().ascii().data(), strlen("Function")))
> +        obj->type = EWK_JS_OBJECT_FUNCTION;

Since you're comparing with string literals, there's no need to use strncmp(), since they're guaranteed to be zero-terminated.

> Source/WebKit/efl/ewk/ewk_js.cpp:197
> +        STRINGZ_TO_NPVARIANT(strdup(data->value.s), *result);

strdup() might return NULL. Will STRINGZ_TO_NPVARIANT be able to handle that?

> Source/WebKit/efl/ewk/ewk_js.cpp:203
> +        OBJECT_TO_NPVARIANT((NPObject*)data->value.o, *result);

C++ casts.

> Source/WebKit/efl/ewk/ewk_js.cpp:235
> +        data->value.s = (char*)malloc(sizeof(char) * (sz + 1));

malloc() might fail. Please review all malloc() usage.

> Source/WebKit/efl/ewk/ewk_js.cpp:264
> +    if (!_NPN_IdentifierIsString(name)) {
> +        ERR("int NPIdentifier is not supported.");
> +        return fail;

Just return false here.

> Source/WebKit/efl/ewk/ewk_js.cpp:267
> +    if (eina_hash_find(obj->properties, prop_name)) // should search methods too?

This comment should include a FIXME: and be proper sentences (begin with uppercase letters, etc).

Also, this if can be ditched:

char* prop...;
bool fail = eina_hash_find(...);
free(prop_name);
return fail;

> Source/WebKit/efl/ewk/ewk_js.cpp:291
> +    if (prop && prop->get) { // class has property and property has getter

Proper sentences must be used on comments.

> Source/WebKit/efl/ewk/ewk_js.cpp:619
> +    bool fail = EINA_FALSE;

Please do not mix 'bool' and 'Eina_Bool' types. Not harmful, but not pretty either.

> Source/WebKit/efl/ewk/ewk_js.cpp:694
> +    int i;
> +    for (i = 0; i < count; i++) {

for (int i = 0; ...)

> Source/WebKit/efl/ewk/ewk_private.h:59
> +    Eina_Hash *properties; // key=name, value=current value of property

Is this comment really needed?

[Flavio Ceolin]

Created attachment 103098 [details]
Patch

New patch with the suggested fixes.

[Lucas De Marchi]

(In reply to comment #27)
> Created an attachment (id=103098) [details]
> Patch
> 
> New patch with the suggested fixes.

It's a very big patch and hard to review, but I think the major flaws were covered on this last patch.

Informal r+.

[WebKit Review Bot]

Comment on attachment 103098 [details]
Patch

Rejecting attachment 103098 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
/efl/ewk/ewk_view.cpp
Hunk #3 succeeded at 3395 (offset 17 lines).
Hunk #4 succeeded at 3556 (offset 19 lines).
patching file Source/WebKit/efl/ewk/ewk_view.h
Hunk #1 succeeded at 85 (offset 62 lines).
Hunk #2 FAILED at 87.
Hunk #3 succeeded at 2176 with fuzz 1 (offset 491 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebKit/efl/ewk/ewk_view.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Antonio Gomes', u'--fo..." exit_code: 1

Full output: http://queues.webkit.org/results/9462838

[Grzegorz Czajkowski]

Recently there were a lot of patches which move API documentation of WebKit-EFL to the header files. Could you please adjust your patch to these changes and add short description of ewk_js.h (@brief command)?

Additionally I think we do not need dots in descriptions of parameters and return value (see EFL's or ewk documetations).

[Lucas De Marchi]

Committed r93637: <http://trac.webkit.org/changeset/93637>