Bug 127420

Summary: [EFL][WK2] defaultTextEncodingName should be stored as a member variable of EwkSettings
Product: WebKit Reporter: Jinwoo Song <jinwoo7.song>
Component: WebKit EFLAssignee: Jinwoo Song <jinwoo7.song>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, ryuan.choi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

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.