Bug 127420 - [EFL][WK2] defaultTextEncodingName should be stored as a member variable of EwkSettings
Summary: [EFL][WK2] defaultTextEncodingName should be stored as a member variable of E...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jinwoo Song
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-22 02:52 PST by Jinwoo Song
Modified: 2014-01-22 19:05 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.52 KB, patch)
2014-01-22 03:01 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jinwoo Song 2014-01-22 02:52:36 PST
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.
Comment 1 Jinwoo Song 2014-01-22 03:01:32 PST
Created attachment 221849 [details]
Patch
Comment 2 Ryuan Choi 2014-01-22 03:20:37 PST
Looks fine to me.
Comment 3 Ryuan Choi 2014-01-22 03:26:33 PST
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.
Comment 4 Gyuyoung Kim 2014-01-22 03:47:30 PST
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 ?
Comment 5 Jinwoo Song 2014-01-22 17:49:26 PST
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?
Comment 6 Gyuyoung Kim 2014-01-22 18:07:56 PST
(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.
Comment 7 WebKit Commit Bot 2014-01-22 19:05:13 PST
Comment on attachment 221849 [details]
Patch

Clearing flags on attachment: 221849

Committed r162584: <http://trac.webkit.org/changeset/162584>
Comment 8 WebKit Commit Bot 2014-01-22 19:05:18 PST
All reviewed patches have been landed.  Closing bug.