Bug 97920 – [EFL][WK2] Change parameter and return type of loadUrlSync().

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

[Byungwoo Lee]

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.

[Byungwoo Lee]

Created attachment 166269 [details]
Patch

[Byungwoo Lee]

Created attachment 166365 [details]
Patch

[Byungwoo Lee]

Created attachment 167114 [details]
Patch

[Byungwoo Lee]

(In reply to comment #3)
> Created an attachment (id=167114) [details]
> Patch

Codes for checking return value is removed to simplify.

[Chris Dumez]

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)); ?

[Chris Dumez]

(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.

[Byungwoo Lee]

(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.

[Chris Dumez]

(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.

[Byungwoo Lee]

(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.

[Gyuyoung Kim]

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.

[Byungwoo Lee]

(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.

[Byungwoo Lee]

Created attachment 167750 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167750 [details]
Patch

LGTM. Please check if this patch won't influence on bots.

[Byungwoo Lee]

Comment on attachment 167750 [details]
Patch

I tested with the latest source and it works fine :)

[WebKit Review Bot]

Comment on attachment 167750 [details]
Patch

Clearing flags on attachment: 167750

Committed r130843: <http://trac.webkit.org/changeset/130843>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.