Add unit tests for custom text encoding setting.
Created attachment 153781 [details] Patch
Created attachment 153850 [details] Patch Add unit tests for set/get custom text encoding.
Comment on attachment 153850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153850&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:39 > + ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0); Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40 > + ewk_view_setting_encoding_custom_set(webView(), "UTF-8"); Would be nice to test the return value. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:43 > + ewk_view_setting_encoding_custom_set(webView(), 0); Ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:44 > + ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0); Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ?
Comment on attachment 153850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153850&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:39 >> + ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0); > > Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ? ASSERT_FALSE also works in this case, but is there anything wrong in using ASSERT_STREQ() here? *_get() returns a C string and I want to assert if C string is NULL(like explained in docs ASSERT_STREQ(NULL, c_string)). But, style checker pushed me to use 0 instead of NULL. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40 >> + ewk_view_setting_encoding_custom_set(webView(), "UTF-8"); > > Would be nice to test the return value. I wonder how much it makes sense to test the return value of set function, because it returns always true(except in case of invalid input arg). That's the reason I am calling get immediately after set and comparing the C string.
Comment on attachment 153850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153850&action=review >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:39 >>> + ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0); >> >> Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ? > > ASSERT_FALSE also works in this case, but is there anything wrong in using ASSERT_STREQ() here? *_get() returns a C string and I want to assert if C string is NULL(like explained in docs ASSERT_STREQ(NULL, c_string)). But, style checker pushed me to use 0 instead of NULL. I don't know if it's wrong but at least it is confusing. >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40 >>> + ewk_view_setting_encoding_custom_set(webView(), "UTF-8"); >> >> Would be nice to test the return value. > > I wonder how much it makes sense to test the return value of set function, because it returns always true(except in case of invalid input arg). That's the reason I am calling get immediately after set and comparing the C string. Well, better safe than sorry. Just because the current implementation always returns TRUE, does not mean the code will stay this way. It is always a good idea to test as much as possible. In this case, it is very easy to add an ASSERT_TRUE() and make sure the return value is really true (and stays that way in the future).
Comment on attachment 153850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153850&action=review >>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40 >>>> + ewk_view_setting_encoding_custom_set(webView(), "UTF-8"); >>> >>> Would be nice to test the return value. >> >> I wonder how much it makes sense to test the return value of set function, because it returns always true(except in case of invalid input arg). That's the reason I am calling get immediately after set and comparing the C string. > > Well, better safe than sorry. Just because the current implementation always returns TRUE, does not mean the code will stay this way. It is always a good idea to test as much as possible. In this case, it is very easy to add an ASSERT_TRUE() and make sure the return value is really true (and stays that way in the future). Yes, it is easy to add and I don't mind adding, but I'm just trying to understand the real use of it with the current implementation. Isn't it true that we should update the test code when we change the APIs implementation?
Created attachment 153874 [details] Patch Fix review comment #3.
Comment on attachment 153874 [details] Patch LGTM.
Comment on attachment 153874 [details] Patch Clearing flags on attachment: 153874 Committed r123545: <http://trac.webkit.org/changeset/123545>
All reviewed patches have been landed. Closing bug.