Bug 85178

Summary: [EFL] Add API for Web Database handling
Product: WebKit Reporter: Thiago Marcos P. Santos <tmpsantos>
Component: WebKit EFLAssignee: Thiago Marcos P. Santos <tmpsantos>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, dglazkov, gyuyoung.kim, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 84572    
Bug Blocks: 86456, 86521    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Patch
none
Patch none

Description Thiago Marcos P. Santos 2012-04-30 04:48:12 PDT
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
Comment 1 Thiago Marcos P. Santos 2012-05-15 15:10:35 PDT
Created attachment 142072 [details]
Patch
Comment 2 Gyuyoung Kim 2012-05-15 19:02:21 PDT
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.
Comment 3 WebKit Review Bot 2012-05-15 19:34:04 PDT
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
Comment 4 WebKit Review Bot 2012-05-15 19:34:10 PDT
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
Comment 5 Thiago Marcos P. Santos 2012-05-16 01:16:02 PDT
(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.
Comment 6 Thiago Marcos P. Santos 2012-05-16 04:18:57 PDT
Created attachment 142217 [details]
Patch
Comment 7 Thiago Marcos P. Santos 2012-05-16 04:19:54 PDT
(In reply to comment #6)
> Created an attachment (id=142217) [details]
> Patch

Fixed the style issue.
Comment 8 Thiago Marcos P. Santos 2012-05-16 05:42:59 PDT
Created attachment 142232 [details]
Patch
Comment 9 Thiago Marcos P. Santos 2012-05-16 05:43:40 PDT
(In reply to comment #8)
> Created an attachment (id=142232) [details]
> Patch

Rebased.
Comment 10 Rafael Antognolli 2012-05-16 08:41:34 PDT
(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.
Comment 11 Thiago Marcos P. Santos 2012-05-16 08:57:58 PDT
(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.
Comment 12 Rafael Antognolli 2012-05-16 12:10:58 PDT
(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.
Comment 13 Antonio Gomes 2012-05-16 17:28:42 PDT
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
Comment 14 Gyuyoung Kim 2012-05-16 18:08:16 PDT
If you fix Antonio's comment, I will land this patch.
Comment 15 Thiago Marcos P. Santos 2012-05-17 10:21:21 PDT
Created attachment 142498 [details]
Patch

Fixed unused parameters.
Comment 16 WebKit Review Bot 2012-05-17 11:08:54 PDT
Comment on attachment 142498 [details]
Patch

Clearing flags on attachment: 142498

Committed r117464: <http://trac.webkit.org/changeset/117464>
Comment 17 WebKit Review Bot 2012-05-17 11:09:00 PDT
All reviewed patches have been landed.  Closing bug.