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
View All Add an attachment (proposed patch, testcase, etc.)

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.