Bug 100225 - [EFL][WK2] Add APIs to get/set the default text encoding name
Summary: [EFL][WK2] Add APIs to get/set the default text encoding name
Status: RESOLVED DUPLICATE of bug 126736
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-24 03:49 PDT by Yuni Jeong
Modified: 2014-02-07 22:41 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.41 KB, patch)
2012-10-24 03:56 PDT, Yuni Jeong
no flags Details | Formatted Diff | Diff
Patch (5.43 KB, patch)
2012-12-30 22:15 PST, Yuni Jeong
benjamin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuni Jeong 2012-10-24 03:49:49 PDT
Add setting APIs for default text encoding and a unit test.
Comment 1 Yuni Jeong 2012-10-24 03:56:32 PDT
Created attachment 170356 [details]
Patch
Comment 2 Jinwoo Song 2012-10-24 18:01:26 PDT
Comment on attachment 170356 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:277
> +

Why not return the encoding name from WKPrefercenCopyDefaultTextEncodingName()?

> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:41
> +    mutable WKEinaSharedString defaultTextEncoding;

Why do we need to add this variable?
Comment 3 Kangil Han 2012-10-24 22:36:29 PDT
Comment on attachment 170356 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:265
> +

EINA_SAFETY_ON_NULL_RETURN_VAL(encoding, false);?
Comment 4 Chris Dumez 2012-10-24 22:50:43 PDT
Comment on attachment 170356 [details]
Patch

Needs rebasing due to recent Ewk_Settings design changes.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-10-29 02:32:46 PDT
Comment on attachment 170356 [details]
Patch

Clearing the r? flag meanwhile.
Comment 6 Gyuyoung Kim 2012-10-29 09:07:07 PDT
Comment on attachment 170356 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:320
> +EAPI const char* ewk_settings_default_encoding_get(const Ewk_Settings *settings);

Nit : wrong * place.
Comment 7 Yuni Jeong 2012-12-30 22:15:28 PST
Created attachment 180964 [details]
Patch
Comment 8 Yuni Jeong 2012-12-30 22:17:34 PST
Comment on attachment 170356 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:265
>> +
> 
> EINA_SAFETY_ON_NULL_RETURN_VAL(encoding, false);?

I added EINA_SAFETY_ON_NULL_RETURN_VAL(encoding, false).

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:320
>> +EAPI const char* ewk_settings_default_encoding_get(const Ewk_Settings *settings);
> 
> Nit : wrong * place.

I changed * place to "const char *ewk_settings_default_encoding_get".

>> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:41
>> +    mutable WKEinaSharedString defaultTextEncoding;
> 
> Why do we need to add this variable?

Because memory for returned default encoding name is released after ewk_settings_default_encoding_get() is called,
So, defaultTextEncoding variable is needed to keep returned default encoding name.
Comment 9 Chris Dumez 2012-12-30 23:33:36 PST
Comment on attachment 180964 [details]
Patch

LGTM.
Comment 10 Gyuyoung Kim 2013-01-01 18:04:07 PST
Comment on attachment 180964 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:59
> +    if (defaultTextEncoding.isEmpty())

I wonder what meaning this condition has. Because, isEmpty() just checks if String has a character at least or null. Look at isEmpty() function. 

   bool isEmpty() const { return !m_impl || !m_impl->length(); }

However, I think "preferences()->defaultTextEncodingName()" won't be null. So, it seems to me that this condition isn't needed, doesn't it ?

Is below enough ?

   m_defaultTextEncoding = preferences()->defaultTextEncodingName().utf8().data();
Comment 11 Kangil Han 2013-01-01 18:24:13 PST
Comment on attachment 180964 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:62
> +    m_defaultTextEncoding = defaultTextEncoding.utf8().data();

Seems saved 'm_defaultTextEncoding' isn't used elsewhere.
Then, can we remove it?
Comment 12 Benjamin Poulain 2013-01-08 18:06:44 PST
Comment on attachment 180964 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:62
>> +    m_defaultTextEncoding = defaultTextEncoding.utf8().data();

This is incorrect. The char buffer is invalid pas that line.
You need to hold on the CString if you want to keep a character buffer alive.
Comment 13 Chris Dumez 2013-01-08 21:56:26 PST
Comment on attachment 180964 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:62
>>> +    m_defaultTextEncoding = defaultTextEncoding.utf8().data();
>> 
>> Seems saved 'm_defaultTextEncoding' isn't used elsewhere.
>> Then, can we remove it?
> 
> This is incorrect. The char buffer is invalid pas that line.
> You need to hold on the CString if you want to keep a character buffer alive.

m_defaulltTextEncoding is not a char* but a WKEinaSharedString. The string is actually copied so the code is correct. This is bit of a particularity of EFL port since our APIs returns EINA Stringshared strings.
Comment 14 Ryuan Choi 2014-02-07 22:41:14 PST
Already fixed at Bug 126736

*** This bug has been marked as a duplicate of bug 126736 ***