Summary: | [EFL] remove strdup in ewk_setting.cpp using eina_stringshare | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | antognolli+webkit, bunhere, commit-queue, kenneth, lucas.de.marchi, tonikitoo | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Attachments: |
|
Description
Ryuan Choi
2010-09-27 02:37:40 PDT
Created attachment 68891 [details]
Patch
Patch looks good to me. Comment on attachment 68891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68891&action=review > WebKit/efl/ChangeLog:8 > + Add variables shared by eins_stringshare and remove strdup. typo here > WebKit/efl/ewk/ewk_settings.cpp:85 > + * @return database path or NULL if none or web database is not supported. You are not mentioning in documentation that this is stringshared and user should not free it, but instead using eina_stringshare functions. Look at ewk_* APIs that return stringshared chars and let a doc similar to that. (In reply to comment #3) > (From update of attachment 68891 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68891&action=review > > > WebKit/efl/ChangeLog:8 > > + Add variables shared by eins_stringshare and remove strdup. > > typo here > > > WebKit/efl/ewk/ewk_settings.cpp:85 > > + * @return database path or NULL if none or web database is not supported. > > You are not mentioning in documentation that this is stringshared and user should not free it, but instead using eina_stringshare functions. Look at ewk_* APIs that return stringshared chars and let a doc similar to that. Thanks, I'll fix it as you mentioned. Created attachment 68915 [details]
Patch
Hi Kenneth and Antonio, If you have time, could you please review this? Comment on attachment 68915 [details]
Patch
Lucas or Raphael, please set cq+ if it also gets your approval.
Comment on attachment 68915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68915&action=review I think it's good, given the issue below is fixed. cq- for now. > WebKit/efl/ewk/ewk_settings.cpp:164 > + return _ewk_icon_database_path; HUmn.. a getter like this is ok for web database, but for icon there's a problem: it is not initialized during ewk_init(), so if user calls this getter without previously calling the setter, he will get a NULL instead of the real path. Since we were previously going through webcore, this was not really an issue. However if we want to make this shortcut now, we need to initialize the icon path in _ewk_init_body() in the same way the web database path is. (In reply to comment #8) > (From update of attachment 68915 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68915&action=review > > I think it's good, given the issue below is fixed. > cq- for now. > > > WebKit/efl/ewk/ewk_settings.cpp:164 > > + return _ewk_icon_database_path; > > HUmn.. a getter like this is ok for web database, but for icon there's a problem: it is not initialized during ewk_init(), so if user calls this getter without previously calling the setter, he will get a NULL instead of the real path. > right, He will get a NULL if it was called before setter. but we can't get real path before setter because we need to open and enable icon_database using setter. It's same as current code. > Since we were previously going through webcore, this was not really an issue. However if we want to make this shortcut now, we need to initialize the icon path in _ewk_init_body() in the same way the web database path is. Ok, I'll prepare new patch including it. Created attachment 69732 [details]
Patch
Created attachment 69760 [details]
Patch
(In reply to comment #11) > Created an attachment (id=69760) [details] > Patch I create folder to initialize icon_database_path. If not, we got below error message. ERR:ewebkit /workspace/webkit/WebKit/efl/ewk/ewk_settings.cpp:116 ewk_settings_icon_database_path_set() could not stat(/root/.webkit): No such file or directory Created attachment 69786 [details]
patch_without_folder_creation
(In reply to comment #13) > Created an attachment (id=69786) [details] > patch_without_folder_creation As following discussion on IRC a few minutes ago, I removed folder creation. We need to fix the problem of default folder in another bug Created attachment 69788 [details]
patch_without_folder_creation
Comment on attachment 69788 [details] patch_without_folder_creation Clearing flags on attachment: 69788 Committed r69113: <http://trac.webkit.org/changeset/69113> All reviewed patches have been landed. Closing bug. |