It is required for enabling encoding detector.
Created attachment 170062 [details] Patch
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.
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.
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.
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. :)
(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.
(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.
Okay, we can update when policy would be changed. ;)
Created attachment 171403 [details] Patch
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.
Comment on attachment 171403 [details] Patch LGTM, thanks.
Comment on attachment 171403 [details] Patch LGTM
LGTM. I changed bug title to match with changelog.
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 ?
Created attachment 171779 [details] Patch
To be consistent with existing APIs, i changed "use" to "enable".
Comment on attachment 171779 [details] Patch Clearing flags on attachment: 171779 Committed r133133: <http://trac.webkit.org/changeset/133133>
All reviewed patches have been landed. Closing bug.