Bug 91639

Created attachment 153055 [details] Patch

Comment on attachment 153055 [details] Patch Attachment 153055 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13277603

Created attachment 153077 [details] Patch Reuploading the same patch again to make ewk-efl happy, now that the dependency landed.

Comment on attachment 153077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153077&action=review > Source/WebKit2/PlatformEfl.cmake:7 > + ${CAIRO_LIBRARIES} This one should be changed CAIRO_LIBRARIES with CAIRO_LDFLAGS. > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:50 > + gchar* uriString = soup_uri_to_string(uri, FALSE); Should we use gchar ? I think we avoid to use glib type in EFL port, if possible. > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:29 > + Nit: Unneeded line.

Created attachment 153188 [details] Patch Take Gyuyoung's feedback into consideration.

Comment on attachment 153188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153188&action=review One final consideration: I can see that you are using a lot of ASSERT_EQ. They are all "fatal" exceptions and will stop that particular test block in case it is hit. If the the tests on the block don't depend on each other, I recommend using EXPECT_* instead. If you did it on purpose, just ignore this comment. > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:51 > + CString ret = uriString; WebKit doesn't like variables named "ret". You should use something more descriptive. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:68 > + ASSERT(!error); Use gtest assertions (i.e. ASSERT_TRUE/ASSERT_FALSE) on the test. Otherwise you wont get this error when testing a release build. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:84 > + ASSERT(!error); Ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:120 > + g_type_init(); > + ecore_main_loop_glib_integrate(); This would be a nice addition for the fixture SetUp(). > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:126 > + ASSERT(cookieManager); Use gtest fatal assertion. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:218 > + ASSERT(cookieManager); Use gtest. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:224 > + ewk_cookie_manager_persistent_storage_set(cookieManager, TMP_COOKIE_FILE_TXT, EWK_COOKIE_PERSISTENT_STORAGE_TEXT); I'm concerned here because we always use the same file name for the cookie storage. This has the following implications: your test might be tainted by a previous run that crashed before the unlink and it will not work when running the same test parallel (i.e. you have 2 build bots on the same machine). A cheap solution for this is to use tempnam() at this test case. Ideally the TestEvironment should provide a temporary directory and cleanup this directory at its teardown. IMO it can be done in another bug.

Comment on attachment 153188 [details] Patch Clearing flags until I update the patch.

Well, the good news is that this patch revealed a regression in libsoup v2.39.3 (bumped yesterday). Thankfully, the issue has been resolved with v2.39.4.1 (confirmed with the unit tests) so I'm bumping libsoup again in bug 91741).

Created attachment 153258 [details] Patch Take Thiago's feedback into consideration.

Comment on attachment 153258 [details] Patch LGTM. Thanks!

Created attachment 153436 [details] Patch Rebase on master.

Comment on attachment 153436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153436&action=review LGTM except for trivial nit. > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:50 > + char* uri = soup_uri_to_string(soupURI, FALSE); nit(?) : is FALSE gtk style ? EFL port have used false so far except for public APIs.

(In reply to comment #12) > (From update of attachment 153436 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153436&action=review > > LGTM except for trivial nit. > > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:50 > > + char* uri = soup_uri_to_string(soupURI, FALSE); > > nit(?) : is FALSE gtk style ? EFL port have used false so far except for public APIs. Yes, I missed that one. Will fix it.

Created attachment 153443 [details] Patch Remove some glib'ism.

Comment on attachment 153443 [details] Patch LGTM, thanks.

Comment on attachment 153443 [details] Patch LGTM and the mini webserver is a great addition to the test suite. Thanks.

All the dependencies have landed, this patch is ready to land.

Comment on attachment 153443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153443&action=review > Source/WebKit2/ChangeLog:12 > + to indicate that only cookies for the main page are accepted by default. main frame? > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:58 > +} > + > + nit: extra line

Created attachment 153829 [details] Patch for landing Fix nits spotted by Antonio.

Comment on attachment 153829 [details] Patch for landing Clearing flags on attachment: 153829 Committed r123372: <http://trac.webkit.org/changeset/123372>

All reviewed patches have been landed. Closing bug.