Bug 127420 – [EFL][WK2] defaultTextEncodingName should be stored as a member variable of EwkSettings

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

[Jinwoo Song]

As the defaultTextEncodingName is returned as a local WKEinaSharedString variable, 
the stringshared data is de-refed by eina_stringshare_del().
To maintain the reference counter, the defaultTextEncodingName should be stored 
as a member variable of EwkSettings class.

[Jinwoo Song]

Created attachment 221849 [details]
Patch

[Ryuan Choi]

Looks fine to me.

[Ryuan Choi]

Comment on attachment 221849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221849&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:353
> +void EwkSettings::setDefaultTextEncodingName(const char* name)

Nit, we normally added methods above the ewk APIs.

[Gyuyoung Kim]

Comment on attachment 221849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221849&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:188
> +    settings->setDefaultTextEncodingName(encoding);

IIRC, we have tried to use C-API in ewk directly. For example, ewk_view.cpp, ewk_context_menu.cpp and so on. Can't we adjust it to ewk_settings as well ?

[Jinwoo Song]

Comment on attachment 221849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221849&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:188
>> +    settings->setDefaultTextEncodingName(encoding);
> 
> IIRC, we have tried to use C-API in ewk directly. For example, ewk_view.cpp, ewk_context_menu.cpp and so on. Can't we adjust it to ewk_settings as well ?

There is a previous bug, https://bugs.webkit.org/show_bug.cgi?id=107820. I maybe follow up the bug after this one.
How do you think about this?

[Gyuyoung Kim]

(In reply to comment #5)
> (From update of attachment 221849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221849&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:188
> >> +    settings->setDefaultTextEncodingName(encoding);
> > 
> > IIRC, we have tried to use C-API in ewk directly. For example, ewk_view.cpp, ewk_context_menu.cpp and so on. Can't we adjust it to ewk_settings as well ?
> 
> There is a previous bug, https://bugs.webkit.org/show_bug.cgi?id=107820. I maybe follow up the bug after this one.
> How do you think about this?

I see. Let's update the bug after landing this.

[WebKit Commit Bot]

Comment on attachment 221849 [details]
Patch

Clearing flags on attachment: 221849

Committed r162584: <http://trac.webkit.org/changeset/162584>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.