Bug 97920

Summary: [EFL][WK2] Change parameter and return type of loadUrlSync().
Product: WebKit Reporter: Byungwoo Lee <bw80.lee>
Component: WebKit EFLAssignee: Byungwoo Lee <bw80.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, laszlo.gombos, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Byungwoo Lee 2012-09-28 09:38:55 PDT
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.
Comment 1 Byungwoo Lee 2012-09-28 10:02:42 PDT
Created attachment 166269 [details]
Patch
Comment 2 Byungwoo Lee 2012-09-29 08:48:31 PDT
Created attachment 166365 [details]
Patch
Comment 3 Byungwoo Lee 2012-10-04 09:19:27 PDT
Created attachment 167114 [details]
Patch
Comment 4 Byungwoo Lee 2012-10-04 09:21:20 PDT
(In reply to comment #3)
> Created an attachment (id=167114) [details]
> Patch

Codes for checking return value is removed to simplify.
Comment 5 Chris Dumez 2012-10-04 09:22:48 PDT
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)); ?
Comment 6 Chris Dumez 2012-10-04 09:23:47 PDT
(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.
Comment 7 Byungwoo Lee 2012-10-04 17:41:15 PDT
(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.
Comment 8 Chris Dumez 2012-10-04 23:03:02 PDT
(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.
Comment 9 Byungwoo Lee 2012-10-05 04:02:13 PDT
(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 10 Gyuyoung Kim 2012-10-09 05:49:01 PDT
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.
Comment 11 Byungwoo Lee 2012-10-09 06:55:08 PDT
(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.
Comment 12 Byungwoo Lee 2012-10-09 07:18:29 PDT
Created attachment 167750 [details]
Patch
Comment 13 Gyuyoung Kim 2012-10-09 18:24:35 PDT
Comment on attachment 167750 [details]
Patch

LGTM. Please check if this patch won't influence on bots.
Comment 14 Byungwoo Lee 2012-10-09 21:14:46 PDT
Comment on attachment 167750 [details]
Patch

I tested with the latest source and it works fine :)
Comment 15 WebKit Review Bot 2012-10-09 21:49:49 PDT
Comment on attachment 167750 [details]
Patch

Clearing flags on attachment: 167750

Committed r130843: <http://trac.webkit.org/changeset/130843>
Comment 16 WebKit Review Bot 2012-10-09 21:49:54 PDT
All reviewed patches have been landed.  Closing bug.