loadUrlSync function uses waitUntilLoadFinished() function. waitUntilLoadFinished has timeout parameter and bool return value. The function also need timeout parameter for the waitUntilLoadFinished() and should return the result of it.
Created attachment 166269 [details] Patch
Created attachment 166365 [details] Patch
Created attachment 167114 [details] Patch
(In reply to comment #3) > Created an attachment (id=167114) [details] > Patch Codes for checking return value is removed to simplify.
Comment on attachment 167114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167114&action=review > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:74 > +bool EWK2UnitTestBase::loadUrlSync(const char* url, double timeoutSeconds) Shouldn't we update the tests using loadUrlSync() with something like ASSERT_TRUE(loadUrlSync(xxx)); ?
(In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=167114) [details] [details] > > Patch > > Codes for checking return value is removed to simplify. Why did you remove it? I think we need it.
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > Created an attachment (id=167114) [details] [details] [details] > > > Patch > > > > Codes for checking return value is removed to simplify. > > Why did you remove it? I think we need it. Yes, I added those initially, but it makes continuous re-base because of conflict. So I removed it from this patch and will apply it after this is landed.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > Created an attachment (id=167114) [details] [details] [details] [details] > > > > Patch > > > > > > Codes for checking return value is removed to simplify. > > > > Why did you remove it? I think we need it. > > Yes, I added those initially, but it makes continuous re-base because of conflict. > So I removed it from this patch and will apply it after this is landed. I don't think we can review this change without updating the callers of this function. I don't understand why you need continuous rebasing either. Just upload a full patch to the bug tracker and rebase it only once after the patch gets r+ in order to cq+ it.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #4) > > > > (In reply to comment #3) > > > > > Created an attachment (id=167114) [details] [details] [details] [details] [details] > > > > > Patch > > > > > > > > Codes for checking return value is removed to simplify. > > > > > > Why did you remove it? I think we need it. > > > > Yes, I added those initially, but it makes continuous re-base because of conflict. > > So I removed it from this patch and will apply it after this is landed. > > I don't think we can review this change without updating the callers of this function. I don't understand why you need continuous rebasing either. Just upload a full patch to the bug tracker and rebase it only once after the patch gets r+ in order to cq+ it. I cannot understand why this cannot be reviewed without the callers. Currently, loadUrlSync function internally uses waitUntilLoadFinished() function with default timeout(10 seconds). Without timeout parameter, timeout interval for loadUrlSync() will be always 10 seconds, and there is no way to change this. And without return value, there is no way to check whether the loadUrlSync() function is finished successfully or with timeout. So I added parameter and return value, just like the other waitUntilXXX functions. I removed the code for more easy review. But if there is no overhead to review with those, I can surely add it.
Comment on attachment 167114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167114&action=review >> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:74 >> +bool EWK2UnitTestBase::loadUrlSync(const char* url, double timeoutSeconds) > > Shouldn't we update the tests using loadUrlSync() with something like ASSERT_TRUE(loadUrlSync(xxx)); ? If you can't guarantee loadUrlSync() always return true, I think caller of loadUrlSync needs to use ASSERT_TRUE or EXPECT_TRUE. However, waitUntilLoadFinished() is not guaranteed to return true always.
(In reply to comment #10) > (From update of attachment 167114 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167114&action=review > > >> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:74 > >> +bool EWK2UnitTestBase::loadUrlSync(const char* url, double timeoutSeconds) > > > > Shouldn't we update the tests using loadUrlSync() with something like ASSERT_TRUE(loadUrlSync(xxx)); ? > > If you can't guarantee loadUrlSync() always return true, I think caller of loadUrlSync needs to use ASSERT_TRUE or EXPECT_TRUE. However, waitUntilLoadFinished() is not guaranteed to return true always. Ok~ then I'll add those.
Created attachment 167750 [details] Patch
Comment on attachment 167750 [details] Patch LGTM. Please check if this patch won't influence on bots.
Comment on attachment 167750 [details] Patch I tested with the latest source and it works fine :)
Comment on attachment 167750 [details] Patch Clearing flags on attachment: 167750 Committed r130843: <http://trac.webkit.org/changeset/130843>
All reviewed patches have been landed. Closing bug.