Bug 85178 – [EFL] Add API for Web Database handling

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

Currently information about the database can only be obtained when the quota is exceeded. Add an API for that like the other ports.

Reference for implementation:
http://webkitgtk.org/reference/webkitgtk/unstable/WebKitWebDatabase.html
http://doc.qt.nokia.com/4.7-snapshot/qwebdatabase.html

[Thiago Marcos P. Santos]

Created attachment 142072 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 142072 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_security_origin.cpp:85
> +Eina_List* ewk_security_origin_web_database_get_all(const Ewk_Security_Origin* origin)

Generally, efl port have added _get at the end of function.

> Source/WebKit/efl/ewk/ewk_web_database.cpp:83
> +const char* ewk_web_database_name_get(Ewk_Web_Database* database)

Is it better to use const keyword for _get() function ?

> Source/WebKit/efl/ewk/ewk_web_database.cpp:138
> +void ewk_web_database_list_free(Eina_List *databaseList)

Move '*' to data type side.

> Source/WebKit/efl/ewk/ewk_web_database.h:73
> +EAPI const char  *ewk_web_database_filename_get(Ewk_Web_Database *o);

Need to use const keyword.

> Source/WebKit/efl/ewk/ewk_web_database.h:86
> +

ditto.

[WebKit Review Bot]

Comment on attachment 142072 [details]
Patch

Attachment 142072 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12694831

New failing tests:
inspector/profiler/heap-snapshot-comparison-expansion-preserved-when-sorting.html

[WebKit Review Bot]

Created attachment 142131 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Thiago Marcos P. Santos]

(In reply to comment #2)
> (From update of attachment 142072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142072&action=review
> 
> > Source/WebKit/efl/ewk/ewk_security_origin.cpp:85
> > +Eina_List* ewk_security_origin_web_database_get_all(const Ewk_Security_Origin* origin)
> 
> Generally, efl port have added _get at the end of function.
> 

This one returns a list. So I used the same naming pattern as in:
EAPI Eina_List *ewk_cookies_get_all(void);

> > Source/WebKit/efl/ewk/ewk_web_database.cpp:83
> > +const char* ewk_web_database_name_get(Ewk_Web_Database* database)
> 
> Is it better to use const keyword for _get() function ?
> 

Yes, but in this case (and the other getters returning a char*), I create a stringshare and it is stored on the Ewk_Web_Database wrapper. These could be created on the _new function, but IMO a lazy loading is more appropriated. 

> > Source/WebKit/efl/ewk/ewk_web_database.cpp:138
> > +void ewk_web_database_list_free(Eina_List *databaseList)
> 
> Move '*' to data type side.
> 
> > Source/WebKit/efl/ewk/ewk_web_database.h:73
> > +EAPI const char  *ewk_web_database_filename_get(Ewk_Web_Database *o);
> 
> Need to use const keyword.
> 
> > Source/WebKit/efl/ewk/ewk_web_database.h:86
> > +
> 
> ditto.

[Thiago Marcos P. Santos]

Created attachment 142217 [details]
Patch

[Thiago Marcos P. Santos]

(In reply to comment #6)
> Created an attachment (id=142217) [details]
> Patch

Fixed the style issue.

[Thiago Marcos P. Santos]

Created attachment 142232 [details]
Patch

[Thiago Marcos P. Santos]

(In reply to comment #8)
> Created an attachment (id=142232) [details]
> Patch

Rebased.

[Rafael Antognolli]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 142072 [details] [details])
> > > Source/WebKit/efl/ewk/ewk_web_database.cpp:83
> > > +const char* ewk_web_database_name_get(Ewk_Web_Database* database)
> > 
> > Is it better to use const keyword for _get() function ?
> > 
> 
> Yes, but in this case (and the other getters returning a char*), I create a stringshare and it is stored on the Ewk_Web_Database wrapper. These could be created on the _new function, but IMO a lazy loading is more appropriated. 

Makes sense. However, I would keep the both API and behavior similar to other wrappers on webkit-efl, where the wrapper structure is filled on creation. Unless you have a good reason for that. e.g. is retrieving these strings too expensive? If that's the case, then ok.

Other than that, the last patch looks good to me.

[Thiago Marcos P. Santos]

(In reply to comment #10)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > (From update of attachment 142072 [details] [details] [details])
> > > > Source/WebKit/efl/ewk/ewk_web_database.cpp:83
> > > > +const char* ewk_web_database_name_get(Ewk_Web_Database* database)
> > > 
> > > Is it better to use const keyword for _get() function ?
> > > 
> > 
> > Yes, but in this case (and the other getters returning a char*), I create a stringshare and it is stored on the Ewk_Web_Database wrapper. These could be created on the _new function, but IMO a lazy loading is more appropriated. 
> 
> Makes sense. However, I would keep the both API and behavior similar to other wrappers on webkit-efl, where the wrapper structure is filled on creation. Unless you have a good reason for that. e.g. is retrieving these strings too expensive? If that's the case, then ok.
> 
> Other than that, the last patch looks good to me.

IMO it can be expensive because the only way of getting a database is by retrieving a list databases from a security origin (this is a pattern followed by other ports). You might get just one database or a thousand, that is one of the reasons why I used lazy loading.

The other is the fact that the main use case of this API will probably be to delete a database. Before the only option was to delete for all security origins. And for this, you don't need to query the properties.

[Rafael Antognolli]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #5)
> > > (In reply to comment #2)
> > > > (From update of attachment 142072 [details] [details] [details] [details])
> > > > > Source/WebKit/efl/ewk/ewk_web_database.cpp:83
> > > > > +const char* ewk_web_database_name_get(Ewk_Web_Database* database)
> > > > 
> > > > Is it better to use const keyword for _get() function ?
> > > > 
> > > 
> > > Yes, but in this case (and the other getters returning a char*), I create a stringshare and it is stored on the Ewk_Web_Database wrapper. These could be created on the _new function, but IMO a lazy loading is more appropriated. 
> > 
> > Makes sense. However, I would keep the both API and behavior similar to other wrappers on webkit-efl, where the wrapper structure is filled on creation. Unless you have a good reason for that. e.g. is retrieving these strings too expensive? If that's the case, then ok.
> > 
> > Other than that, the last patch looks good to me.
> 
> IMO it can be expensive because the only way of getting a database is by retrieving a list databases from a security origin (this is a pattern followed by other ports). You might get just one database or a thousand, that is one of the reasons why I used lazy loading.
> 
> The other is the fact that the main use case of this API will probably be to delete a database. Before the only option was to delete for all security origins. And for this, you don't need to query the properties.

Right, that's a good reason. Informal r+ from me.

[Antonio Gomes]

Comment on attachment 142232 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_security_origin.cpp:91
> +Eina_List* ewk_security_origin_web_database_get_all(const Ewk_Security_Origin* origin)

#else
unused_param(origin)

> Source/WebKit/efl/ewk/ewk_web_database.cpp:53
> +#else
> +    return 0;
> +#endif

ditto

> Source/WebKit/efl/ewk/ewk_web_database.cpp:64
> +#else
> +    return 0;
> +#endif

ditto

[Gyuyoung Kim]

If you fix Antonio's comment, I will land this patch.

[Thiago Marcos P. Santos]

Created attachment 142498 [details]
Patch

Fixed unused parameters.

[WebKit Review Bot]

Comment on attachment 142498 [details]
Patch

Clearing flags on attachment: 142498

Committed r117464: <http://trac.webkit.org/changeset/117464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.