Summary: | [EFL] Add API for Web Database handling | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Thiago Marcos P. Santos <tmpsantos> | ||||||||||||
Component: | WebKit EFL | Assignee: | 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
Thiago Marcos P. Santos
2012-04-30 04:48:12 PDT
Created attachment 142072 [details]
Patch
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 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 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
(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. Created attachment 142217 [details]
Patch
(In reply to comment #6) > Created an attachment (id=142217) [details] > Patch Fixed the style issue. Created attachment 142232 [details]
Patch
(In reply to comment #8) > Created an attachment (id=142232) [details] > Patch Rebased. (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. (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. (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 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 If you fix Antonio's comment, I will land this patch. Created attachment 142498 [details]
Patch
Fixed unused parameters.
Comment on attachment 142498 [details] Patch Clearing flags on attachment: 142498 Committed r117464: <http://trac.webkit.org/changeset/117464> All reviewed patches have been landed. Closing bug. |