Bug 85178 - [EFL] Add API for Web Database handling
: [EFL] Add API for Web Database handling
Status: RESOLVED FIXED
: WebKit
WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 84572
: 86456 86521
  Show dependency treegraph
 
Reported: 2012-04-30 04:48 PST by
Modified: 2012-05-17 11:09 PST (History)


Attachments
Patch (20.25 KB, patch)
2012-05-15 15:10 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.26 MB, application/zip)
2012-05-15 19:34 PST, WebKit Review Bot
no flags Details
Patch (20.25 KB, patch)
2012-05-16 04:18 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.31 KB, patch)
2012-05-16 05:42 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.71 KB, patch)
2012-05-17 10:21 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-30 04:48:12 PST
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 From 2012-05-15 15:10:35 PST -------
Created an attachment (id=142072) [details]
Patch
------- Comment #2 From 2012-05-15 19:02:21 PST -------
(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.

> 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 From 2012-05-15 19:34:04 PST -------
(From update of attachment 142072 [details])
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 From 2012-05-15 19:34:10 PST -------
Created an attachment (id=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 From 2012-05-16 01:16:02 PST -------
(In reply to comment #2)
> (From update of attachment 142072 [details] [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 From 2012-05-16 04:18:57 PST -------
Created an attachment (id=142217) [details]
Patch
------- Comment #7 From 2012-05-16 04:19:54 PST -------
(In reply to comment #6)
> Created an attachment (id=142217) [details] [details]
> Patch

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

Rebased.
------- Comment #10 From 2012-05-16 08:41:34 PST -------
(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.
------- Comment #11 From 2012-05-16 08:57:58 PST -------
(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.
------- Comment #12 From 2012-05-16 12:10:58 PST -------
(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] [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 From 2012-05-16 17:28:42 PST -------
(From update of attachment 142232 [details])
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 From 2012-05-16 18:08:16 PST -------
If you fix Antonio's comment, I will land this patch.
------- Comment #15 From 2012-05-17 10:21:21 PST -------
Created an attachment (id=142498) [details]
Patch

Fixed unused parameters.
------- Comment #16 From 2012-05-17 11:08:54 PST -------
(From update of attachment 142498 [details])
Clearing flags on attachment: 142498

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