Bug 46613 – [EFL] remove strdup in ewk_setting.cpp using eina_stringshare

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

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.

[Ryuan Choi]

Created attachment 68891 [details]
Patch

[Rafael Antognolli]

Patch looks good to me.

[Lucas De Marchi]

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.

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 68915 [details]
Patch

[Ryuan Choi]

Hi Kenneth and Antonio,

If you have time, could you please review this?

[Antonio Gomes]

Comment on attachment 68915 [details]
Patch

Lucas or Raphael, please set cq+ if it also gets your approval.

[Lucas De Marchi]

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.

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 69732 [details]
Patch

[Ryuan Choi]

Created attachment 69760 [details]
Patch

[Ryuan Choi]

(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

[Ryuan Choi]

Created attachment 69786 [details]
patch_without_folder_creation

[Ryuan Choi]

(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

[Ryuan Choi]

Created attachment 69788 [details]
patch_without_folder_creation

[WebKit Commit Bot]

Comment on attachment 69788 [details]
patch_without_folder_creation

Clearing flags on attachment: 69788

Committed r69113: <http://trac.webkit.org/changeset/69113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.