Bug 77337

Summary: [EFL] Refactor ewk_js files.
Product: WebKit Reporter: Tomasz Morawski <t.morawski>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: barbieri, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
First patch none

Description Tomasz Morawski 2012-01-30 06:45:11 PST
Changes:
- The _ewk_js_npvariant_to_variant function is called when _NPN_GetProperty returns true inside _ewk_js_npobject_property_get function now.
- Renamed all static functions and structures definition. Now they start with _ character.
- Changed Eina_Bool to bool in private functions.
- Simplify the code in ewk_js_object_new and ewk_js_object_invoke.
- Renamed parameter in ewk_js_variant_array_free function.
Comment 1 Tomasz Morawski 2012-01-30 06:55:04 PST
Created attachment 124543 [details]
First patch

Proposition of trivial changes. This patch precedes malloc replace patch for ewk_js file. 
I have put cq- due to that this patch was not tested due to there is not clear information how to use ewk_js API. I will be very grateful for information about how to use js API.
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-02-01 07:47:03 PST
Comment on attachment 124543 [details]
First patch

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

Looks good.

> Source/WebKit/efl/ewk/ewk_js.h:144
>   * Release resources allocated by @a var.

"var" should be "vars" here too.
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-02-01 07:54:29 PST
(In reply to comment #1)
> I have put cq- due to that this patch was not tested due to there is not clear information how to use ewk_js API.

Unfortunately, the whole ewk_js API is... not very well-thought, to put it in mild terms. The fact that it has no clear documentation makes it even worse.

> I will be very grateful for information about how to use js API.

Here's a very short explanation: I don't know how familiar you are with it, so let's start from the very beginning. The code is used to inject custom JavaScript objects in web pages. You use an Ewk_JS_Class_Meta to define what the object looks like (what methods it has, what attributes can be accessed etc). You then use ewk_js_object_new with the metaclass definition to create an Ewk_JS_Object that can later be injected into a view with ewk_view_js_object_add with a given name. This makes the object available inside the page like any other JS object; if you call a method or try to access a property you have defined, the respective callbacks are called etc. Note it currently makes sense to call ewk_view_js_object_add() only after "js,windowobject,clear" has been emitted, otherwise your object will be wiped from the page when the signal is emitted.

Anyway, this is patch is a nice step towards at least making the code more readable. In the future, it will need a complete rewrite, though.
Comment 4 Tomasz Morawski 2012-02-02 00:08:06 PST
(In reply to comment #3)
Thank you very much for your explanation. 

> Anyway, this is patch is a nice step towards at least making the code more readable. In the future, it will need a complete rewrite, though.
Is it really that bad to rewrite everything? If yes, maybe better will be to abandone this patch to avoid useless commits. I would like to leave the change inside _ewk_js_npobject_property_get function only which I think fixes an issue.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-02-02 04:17:50 PST
(In reply to comment #4)
> Is it really that bad to rewrite everything?

IMO, yes. Besides the code having no useful documentation, there are a lot of style issues such as the ones you are trying to clean up (plus a lot of useless C-isms and manual memory management), similar functions with different order of parameters and bad architecture in general (external code shouldn't be able to access so many structures and their fields directly, ewk_js_npobject_to_object duplicates the Ewk_JS_Object creation code etc).
Comment 6 Tomasz Morawski 2012-02-02 05:17:20 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Is it really that bad to rewrite everything?
> 
> IMO, yes. Besides the code having no useful documentation, there are a lot of style issues such as the ones you are trying to clean up (plus a lot of useless C-isms and manual memory management), similar functions with different order of parameters and bad architecture in general (external code shouldn't be able to access so many structures and their fields directly, ewk_js_npobject_to_object duplicates the Ewk_JS_Object creation code etc).

I have prepared a patch not yet publicated that changes almost all c memory allocation to new/delete with smart pointer uses. Use of smart ponters removes also some c-isms but not all. Anyway, I must leave some malloc/free operation related to c-string objects. There is possible to use WTF::String but this could be unnatural in some cases. 

There is also two function that doesn't look good to me. I didn't changed this yet. 

void ewk_js_variant_free(Ewk_JS_Variant* jsVariant)
{
    EINA_SAFETY_ON_NULL_RETURN(jsVariant);
    if (jsVariant->type == EWK_JS_VARIANT_STRING)
        free(jsVariant->value.s);
    else if (jsVariant->type == EWK_JS_VARIANT_OBJECT)
        ewk_js_object_free(jsVariant->value.o);
    free(jsVariant);
}

void ewk_js_variant_array_free(Ewk_JS_Variant* jsVariant, int count)
{
    EINA_SAFETY_ON_NULL_RETURN(jsVariant);
    for (int i = 0; i < count; i++) {
        if (jsVariant[i].type == EWK_JS_VARIANT_STRING)
            free(jsVariant[i].value.s);
        else if (jsVariant[i].type == EWK_JS_VARIANT_OBJECT)
            ewk_js_object_free(jsVariant[i].value.o);
    }
    free(jsVariant);
}

Yes... I think ewk_js_variant_array_free should take a pointer to array of pointers to Ewk_JS_Variant object insted of pointer to array of Ewk_JS_Variant object. After that the ewk_js_variant_free function could be called directly inside ewk_js_variant_array_free function. 

Anyway, to continue work related to this file I need to make some simple test code firsty.
Comment 7 Tomasz Morawski 2012-02-09 00:12:30 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Is it really that bad to rewrite everything?
> 
> IMO, yes. Besides the code having no useful documentation, there are a lot of style issues such as the ones you are trying to clean up (plus a lot of useless C-isms and manual memory management), similar functions with different order of parameters and bad architecture in general (external code shouldn't be able to access so many structures and their fields directly, ewk_js_npobject_to_object duplicates the Ewk_JS_Object creation code etc).

I have made a simple test code and now I see why would you like to make bigger clean up of this API. Due to that I have decided to abondon my patch. I started to make complex refactor of js files with mentioned architecture changes, general clean up and partial reimplementation of internal functions. Manual memory managment will be also removed. I will also try to make some comments inside code and prepare complex code including related html page. I think I will able to show a first version of patch soon.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-02-09 05:01:45 PST
(In reply to comment #7)
> I started to make complex refactor of js files with mentioned architecture changes, general clean up and partial reimplementation of internal functions. Manual memory managment will be also removed. I will also try to make some comments inside code and prepare complex code including related html page. I think I will able to show a first version of patch soon.

By the way, one of the first things which should be included in this revamp is the use of the recently-introduced Eina_Model stuff which should help _a lot_ with reducing the necessary code on the ewk_js side. There's very little documentation on it yet, but it is most likely the way to go.
Comment 9 Tomasz Morawski 2012-02-09 06:37:22 PST
(In reply to comment #8)
> (In reply to comment #7)
> > I started to make complex refactor of js files with mentioned architecture changes, general clean up and partial reimplementation of internal functions. Manual memory managment will be also removed. I will also try to make some comments inside code and prepare complex code including related html page. I think I will able to show a first version of patch soon.
> 
> By the way, one of the first things which should be included in this revamp is the use of the recently-introduced Eina_Model stuff which should help _a lot_ with reducing the necessary code on the ewk_js side. There's very little documentation on it yet, but it is most likely the way to go.

Maybe it will better if I write something about my plan.

1) From API point of view:
It is only a plan that could be changed :) I would like to remove almost all structures form ewk_js.h file and put them into new ewk_js_private.h file. These private files are very often uses in gtk and qt port to store internal object definitions. BTW I don't know why EFL port don't follow this rule and put every thing into ewk_private.h file? These structures except js_variant object definition are not very useful and should not be in public API. 

a) For example instead of direct creating and managing of js meta class object I am suggesting to add few new API functions to manage it indirectly. I would like to introduce these function:

ewk_js_meta_class_new()
ewk_js_meta_class_free(Ewk_JS_Class_Meta *meta_cls)
ewk_js_meta_class_method_add(Ewk_JS_Class_Meta *meta_cls, const char *name, Ewk_JS_Invoke_Cb);
ewk_js_meta_class_property_add(Ewk_JS_Class_Meta *meta_cls, const char *name, Ewk_JS_Variant *variant);
ewk_js_meta_class_property_default_cb_set(Ewk_JS_Class_Meta *meta_cls, const char* name, Ewk_JS_Set_Cb set, Ewk_JS_Get_Cb get, Ewk_JS_Del_Cb del)
ewk_js_meta_class_properties_default_cb_set(Ewk_JS_Class_Meta *meta_cls, Ewk_JS_Set_Cb set, Ewk_JS_Get_Cb get, Ewk_JS_Del_Cb del);

b) Adding a new function to create a Ewk_JS_Variant object. This function will initialize it to an empty value. 

c) Adding a new function to get value of property form object like this:
Ewk_JS_Property *ewk_js_object_property_value_get(Ewk_JS_Object *object, const char *name)

d) Remove:
EAPI Eina_Hash *ewk_js_object_properties_get(const Ewk_JS_Object *obj);
EAPI void ewk_js_variant_array_free(Ewk_JS_Variant *var, int count);

2) From cpp point of view:
After removing c memory management, renaming internal function, simplify some internal implementations the code looks much better. But it could be better if it is possible to use for example WTF:: containers or c++ struct with methods and inheritance for internal objects. :) Currently I am not sure about using Eina_Model. The Eina_Model looks to heavy for this simple purpose.
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-02-09 07:00:39 PST
(In reply to comment #9)
> BTW I don't know why EFL port don't follow this rule and put every thing into ewk_private.h file?

Mostly because that's what the EFL themselves do.

> a) For example instead of direct creating and managing of js meta class object I am suggesting to add few new API functions to manage it indirectly. I would like to introduce these function:
> 
> ewk_js_meta_class_new()
> ewk_js_meta_class_free(Ewk_JS_Class_Meta *meta_cls)
> ewk_js_meta_class_method_add(Ewk_JS_Class_Meta *meta_cls, const char *name, Ewk_JS_Invoke_Cb);
> ewk_js_meta_class_property_add(Ewk_JS_Class_Meta *meta_cls, const char *name, Ewk_JS_Variant *variant);
> ewk_js_meta_class_property_default_cb_set(Ewk_JS_Class_Meta *meta_cls, const char* name, Ewk_JS_Set_Cb set, Ewk_JS_Get_Cb get, Ewk_JS_Del_Cb del)
> ewk_js_meta_class_properties_default_cb_set(Ewk_JS_Class_Meta *meta_cls, Ewk_JS_Set_Cb set, Ewk_JS_Get_Cb get, Ewk_JS_Del_Cb del);
> 
> b) Adding a new function to create a Ewk_JS_Variant object. This function will initialize it to an empty value. 
> 
> c) Adding a new function to get value of property form object like this:
> Ewk_JS_Property *ewk_js_object_property_value_get(Ewk_JS_Object *object, const char *name)
>
> d) Remove:
> EAPI Eina_Hash *ewk_js_object_properties_get(const Ewk_JS_Object *obj);
> EAPI void ewk_js_variant_array_free(Ewk_JS_Variant *var, int count);

This could all be replaced with Eina_Model (even if Eina_Model itself needs some new code for that).
Comment 11 Tomasz Morawski 2012-02-09 07:57:28 PST
(In reply to comment #10)
> (In reply to comment #9)
> > BTW I don't know why EFL port don't follow this rule and put every thing into ewk_private.h file?
> 
> Mostly because that's what the EFL themselves do.
But why into one, single the ewk_private.h file? It is not logic. Even EFL like ecore, evas etc. libraries use more then one private file for example the ecore uses 17 such private files. 

> This could all be replaced with Eina_Model (even if Eina_Model itself needs some new code for that).
How it may work? Do you suggest to define Ewk_JS_Object, Ewk_JS_Class_Meta, Ewk_JS_Variant, Ewk_JS_Method, Ewk_JS_Property, Ewk_JS_Class_Meta as Eina_Model object inside ewk_js.h file?
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-02-09 08:17:53 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > BTW I don't know why EFL port don't follow this rule and put every thing into ewk_private.h file?
> > 
> > Mostly because that's what the EFL themselves do.
> But why into one, single the ewk_private.h file? It is not logic.

It's just a convention.

> Even EFL like ecore, evas etc. libraries use more then one private file for example the ecore uses 17 such private files. 

You can consider evas has a single evas_private.h that's got 1k lines; if you compare it with ecore, you should consider ewk equivalent to one ecore module.

Note I am not saying I agree with this, but rather explaining why things were done this way the last time the port was rewritten.
 
> > This could all be replaced with Eina_Model (even if Eina_Model itself needs some new code for that).
> How it may work? Do you suggest to define Ewk_JS_Object, Ewk_JS_Class_Meta, Ewk_JS_Variant, Ewk_JS_Method, Ewk_JS_Property, Ewk_JS_Class_Meta as Eina_Model object inside ewk_js.h file?

Eina_Model already has the concept of properties, Eina_Value is a better Ewk_JS_Variant, Ewk_JS_Class_Meta shouldn't be needed etc. I haven't thoroughly thought about how it would work exactly, but the point is to leverage what already exists in Eina as much as possible.
Comment 13 Tomasz Morawski 2012-02-16 05:26:20 PST
(In reply to comment #12)
Hi,
Sorry for long absence.

> Note I am not saying I agree with this, but rather explaining why things were done this way the last time the port was rewritten.
I have moved our discussion to webkit-efl list where it is a better place for it. 

> > > This could all be replaced with Eina_Model (even if Eina_Model itself needs some new code for that).
> > How it may work? Do you suggest to define Ewk_JS_Object, Ewk_JS_Class_Meta, Ewk_JS_Variant, Ewk_JS_Method, Ewk_JS_Property, Ewk_JS_Class_Meta as Eina_Model object inside ewk_js.h file?
> 
> Eina_Model already has the concept of properties, Eina_Value is a better Ewk_JS_Variant, Ewk_JS_Class_Meta shouldn't be needed etc. I haven't thoroughly thought about how it would work exactly, but the point is to leverage what already exists in Eina as much as possible.
My main intention was to obtain small, easy and clear API for final user with few functions to create and manipulate objects and put all ewk_js implementation to internal port files. It is a good idea to use Eina_Model and Eina_Value but only internally. These objects should be encapsulated by few public API functions. This way will allow to make browser application independent to future Eina_* object changes, library and it will be more save due to that API functions could be properly handled. 
Anyway, why Ewk_Js_Class_Meta will be not need?
Comment 14 Gustavo Sverzut Barbieri 2012-02-16 08:12:23 PST
(In reply to comment #13)
> (In reply to comment #12)
> > Eina_Model already has the concept of properties, Eina_Value is a better Ewk_JS_Variant, Ewk_JS_Class_Meta shouldn't be needed etc. I haven't thoroughly thought about how it would work exactly, but the point is to leverage what already exists in Eina as much as possible.
> My main intention was to obtain small, easy and clear API for final user with few functions to create and manipulate objects and put all ewk_js implementation to internal port files. It is a good idea to use Eina_Model and Eina_Value but only internally. These objects should be encapsulated by few public API functions. This way will allow to make browser application independent to future Eina_* object changes, library and it will be more save due to that API functions could be properly handled. 

You should not abstract away Eina, actually the other way. It is to be considered a base of common knowledge in EFL. The idea of Eina_Model is to be the common language for hierarchical data objects, with reference counting, signals, properties and inheritance.

There are couple of sites that needs this: EWK, Elementary (GUI), Object-Relational Mappers (ORM/SQL) and also DBus. Imagine you want to expose your recently built model to the WebView, do you want to translate it all over again? Why? Qt did not do this, you could export any object to view without effort.

It was introduced by recently, that's why we did not use it before. And really, people liked it so much that it's going to be made an eina_object with the non-model specific bits, and eina_model will inherit from it.


> Anyway, why Ewk_Js_Class_Meta will be not need?

Because the Eina version already exposes properties and so on. Just methods are missing, given that methods have no string name. What can be done is to add a layer to do this in Ewk, or simply create a value type that is JS_METHOD, thus the property can hold a value that is actually a method (like Python does).
Comment 15 Tomasz Morawski 2012-02-17 02:03:17 PST
(In reply to comment #14)
Thank you for your replay. I understand your point of view and I will try to reimplement this module with Eina_Model use. 

> Because the Eina version already exposes properties and so on. Just methods are missing, given that methods have no string name. What can be done is to add a layer to do this in Ewk, or simply create a value type that is JS_METHOD, thus the property can hold a value that is actually a method (like Python does).

Is http://docs.enlightenment.org/auto/eina/eina_model_01_c.html page show typical use of Eina_Model that could be used in new ewk_js implementation? So, I think it is better to handling methods like Python does. Due to that adding new API layer only for methods will be not consentaneus with properties which will not have such API (or we will add layer for properties too).
Comment 16 Gustavo Sverzut Barbieri 2012-02-17 10:50:26 PST
Hi Tomasz,

That's not the example I like most, but does its thing. Be sure to look at the full documentation at: http://docs.enlightenment.org/auto/eina/group__Eina__Model__Group.html I like the second example more: http://docs.enlightenment.org/auto/eina/eina_model_02_example_page.html

It's heavily based on the Generic Value, which have its own documentation: http://docs.enlightenment.org/auto/eina/group__Eina__Value__Group.html  We'll commit a new example on how to create custom types to SVN, you can check it later to see how to do the JS_METHOD thing.

Hope it helps!
Comment 17 Tomasz Morawski 2012-02-20 23:50:10 PST
(In reply to comment #16)
Hi Gustavo,
Thank you once agin :) it helps a lot. Could you please post an info to his bug if you add "new example" to eina documentation?
Comment 18 Gustavo Sverzut Barbieri 2012-02-21 05:14:45 PST
Hi Tomasz,

It got published: http://docs.enlightenment.org/auto/eina/eina_value_example_03_page.html

Let me know in private mail (or at enlightenment-devel mail list, or IRC #edevelop) if you need further help.