Bug 100066 – [WK2] Add APIs to get/set encoding detector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Yuni Jeong]

It is required for enabling encoding detector.

[Yuni Jeong]

Created attachment 170062 [details]
Patch

[Jinwoo Song]

Comment on attachment 170062 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Add APIs to get/set encoding detector

This patch not only adds ewk setting APIs but also adds WKPreference APIs.
So it is not proper to add [EFL] prefix to the title.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_settings.cpp:165
> +    ASSERT_FALSE(ewk_setting_uses_encoding_detector_get(settings));

It would be better to add comments that the default setting value is 'false' to explain why ASSERT_FALSE() is used.

[Kangil Han]

Comment on attachment 170062 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPreferences.h:223
> +// Defaults to false.

IMHO, it depends on policy. So I would like to remove this comment.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_settings.cpp:165
>> +    ASSERT_FALSE(ewk_setting_uses_encoding_detector_get(settings));
> 
> It would be better to add comments that the default setting value is 'false' to explain why ASSERT_FALSE() is used.

IMHO, it depends on policy. So I would like to remove this line of test.

[Jinwoo Song]

Comment on attachment 170062 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKPreferences.h:223
>> +// Defaults to false.
> 
> IMHO, it depends on policy. So I would like to remove this comment.

There are default values for each setting member variables in Setting.cpp. I think its common to comment it.

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_settings.cpp:165
>>> +    ASSERT_FALSE(ewk_setting_uses_encoding_detector_get(settings));
>> 
>> It would be better to add comments that the default setting value is 'false' to explain why ASSERT_FALSE() is used.
> 
> IMHO, it depends on policy. So I would like to remove this line of test.

Ditto.

[Kangil Han]

Comment on attachment 170062 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/C/WKPreferences.h:223
>>> +// Defaults to false.
>> 
>> IMHO, it depends on policy. So I would like to remove this comment.
> 
> There are default values for each setting member variables in Setting.cpp. I think its common to comment it.

If we set this setting value true on ewk initialization step, as an example, this function should return true.
It means it is port specific.

Then, caller might have confusion between this comment and actual result.
Actually, there was a discussion to enable encoding detector by default in efl port even though it hasn't been happened. :)

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 170062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170062&action=review
> 
> >> Source/WebKit2/UIProcess/API/C/WKPreferences.h:223
> >> +// Defaults to false.
> > 
> > IMHO, it depends on policy. So I would like to remove this comment.
> 
> There are default values for each setting member variables in Setting.cpp. I think its common to comment it.

I agree with Jinwoo here. It is good (and common practice) to document the default value (even if ports can override it). Ports document their default values in their port-specific API anyway.

[Ryuan Choi]

(In reply to comment #5)
> (From update of attachment 170062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170062&action=review
> 
> >>> Source/WebKit2/UIProcess/API/C/WKPreferences.h:223
> >>> +// Defaults to false.
> >> 
> >> IMHO, it depends on policy. So I would like to remove this comment.
> > 
> > There are default values for each setting member variables in Setting.cpp. I think its common to comment it.
> 
> If we set this setting value true on ewk initialization step, as an example, this function should return true.
> It means it is port specific.
> 
> Then, caller might have confusion between this comment and actual result.
> Actually, there was a discussion to enable encoding detector by default in efl port even though it hasn't been happened. :)

If someone changes default value, they should update doxygen also.

I supports jinwoo and chris.

[Kangil Han]

Okay, we can update when policy would be changed. ;)

[Yuni Jeong]

Created attachment 171403 [details]
Patch

[Yuni Jeong]

As Jinwoo Song's comment,
I removed [EFL] prefix in the title and added comment that encoding detector is not used by default to unit test for ewk_setting_uses_encoding_detector.

[Kangil Han]

Comment on attachment 171403 [details]
Patch

LGTM, thanks.

[Mikhail Pozdnyakov]

Comment on attachment 171403 [details]
Patch

LGTM

[Ryuan Choi]

LGTM.

I changed bug title to match with changelog.

[Gyuyoung Kim]

Comment on attachment 171403 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:947
> +void WKPreferencesSetUsesEncodingDetector(WKPreferencesRef preferencesRef, bool use)

To be consistent with existing APIs, it would be good if you use *enabled* or *flag* instead of *use*.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:274
> +Eina_Bool ewk_setting_uses_encoding_detector_set(Ewk_Settings* settings, Eina_Bool use)

ditto. In this case, enable ?

[Yuni Jeong]

Created attachment 171779 [details]
Patch

[Yuni Jeong]

To be consistent with existing APIs, i changed "use" to "enable".

[WebKit Review Bot]

Comment on attachment 171779 [details]
Patch

Clearing flags on attachment: 171779

Committed r133133: <http://trac.webkit.org/changeset/133133>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.