Bug 100225 – [EFL][WK2] Add APIs to get/set the default text encoding name

• Component: WebKit EFL
• Status: RESOLVED DUPLICATE

[Yuni Jeong]

Add setting APIs for default text encoding and a unit test.

[Yuni Jeong]

Created attachment 170356 [details]
Patch

[Jinwoo Song]

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?

[Kangil Han]

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);?

[Chris Dumez]

Comment on attachment 170356 [details]
Patch

Needs rebasing due to recent Ewk_Settings design changes.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 170356 [details]
Patch

Clearing the r? flag meanwhile.

[Gyuyoung Kim]

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.

[Yuni Jeong]

Created attachment 180964 [details]
Patch

[Yuni Jeong]

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.

[Chris Dumez]

Comment on attachment 180964 [details]
Patch

LGTM.

[Gyuyoung Kim]

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();

[Kangil Han]

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?

[Benjamin Poulain]

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.

[Chris Dumez]

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.

[Ryuan Choi]

Already fixed at Bug 126736

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