Bug 46613 - [EFL] remove strdup in ewk_setting.cpp using eina_stringshare
Summary: [EFL] remove strdup in ewk_setting.cpp using eina_stringshare
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-27 02:37 PDT by Ryuan Choi
Modified: 2010-10-05 09:00 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.49 KB, patch)
2010-09-27 02:45 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (4.92 KB, patch)
2010-09-27 08:57 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (5.47 KB, patch)
2010-10-04 20:14 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (5.66 KB, patch)
2010-10-04 23:54 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
patch_without_folder_creation (5.61 KB, patch)
2010-10-05 08:17 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
patch_without_folder_creation (5.47 KB, patch)
2010-10-05 08:24 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2010-09-27 02:37:40 PDT
ewk_settings_web_database_path_get and ewk_settings_icon_database_path_get returns newly allocated string using strdup.

I think that we can remove it using eina_stringshare.
Comment 1 Ryuan Choi 2010-09-27 02:45:32 PDT
Created attachment 68891 [details]
Patch
Comment 2 Rafael Antognolli 2010-09-27 07:13:31 PDT
Patch looks good to me.
Comment 3 Lucas De Marchi 2010-09-27 07:58:39 PDT
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.
Comment 4 Ryuan Choi 2010-09-27 08:51:25 PDT
(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.
Comment 5 Ryuan Choi 2010-09-27 08:57:55 PDT
Created attachment 68915 [details]
Patch
Comment 6 Ryuan Choi 2010-10-04 04:16:06 PDT
Hi Kenneth and Antonio,

If you have time, could you please review this?
Comment 7 Antonio Gomes 2010-10-04 07:49:43 PDT
Comment on attachment 68915 [details]
Patch

Lucas or Raphael, please set cq+ if it also gets your approval.
Comment 8 Lucas De Marchi 2010-10-04 09:42:09 PDT
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.
Comment 9 Ryuan Choi 2010-10-04 20:05:13 PDT
(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.
Comment 10 Ryuan Choi 2010-10-04 20:14:03 PDT
Created attachment 69732 [details]
Patch
Comment 11 Ryuan Choi 2010-10-04 23:54:37 PDT
Created attachment 69760 [details]
Patch
Comment 12 Ryuan Choi 2010-10-04 23:59:11 PDT
(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
Comment 13 Ryuan Choi 2010-10-05 08:17:26 PDT
Created attachment 69786 [details]
patch_without_folder_creation
Comment 14 Ryuan Choi 2010-10-05 08:20:53 PDT
(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
Comment 15 Ryuan Choi 2010-10-05 08:24:48 PDT
Created attachment 69788 [details]
patch_without_folder_creation
Comment 16 WebKit Commit Bot 2010-10-05 09:00:36 PDT
Comment on attachment 69788 [details]
patch_without_folder_creation

Clearing flags on attachment: 69788

Committed r69113: <http://trac.webkit.org/changeset/69113>
Comment 17 WebKit Commit Bot 2010-10-05 09:00:42 PDT
All reviewed patches have been landed.  Closing bug.