[Webkit-unassigned] [Bug 43446] [EFL] Exporting JavaScript objects for EFL port

[bugzilla-daemon at webkit.org]

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


Leandro Pereira <leandro at profusion.mobi> changed:

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




--- Comment #26 from Leandro Pereira <leandro at profusion.mobi>  2011-07-07 12:16:04 PST ---
(From update of attachment 98225)
View in context: https://bugs.webkit.org/attachment.cgi?id=98225&action=review

Informal r-.

> Source/WebKit/ChangeLog:8
> +2011-06-16  Jonas M. Gastal <jgastal at profusion.mobi>
> +
> +        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
> +2011-06-16  Jonas M. Gastal <jgastal at profusion.mobi>
> +        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?

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