Bug 96910 – [EFL][WK2] waitUntilTitleChangedTo and waitUntilLoadFinished needs timeout.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

Currently, the waitUntilTitleChangedTo() and waitUntilLoadFinished() functions doesn't handle timeout by itself.
And if there are some failed cases that loading is not finished or title is not changed to the expected string,
test case just stopped with timeout and there is no information about this (line number.. etc)
And when this happens, below test cases also doesn't work.

So adding timeout parameter for these functions will be better to handle and test timeout cases.

[Byungwoo Lee]

Created attachment 164376 [details]
Patch

[Byungwoo Lee]

return value of those function can be used as below.

when test met timeout at below line,
EXPECT_TRUE(waitUntilTitleChangedTo("Foo", 10));


log can show this status as below.
WebKit/Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:76: Failure
Value of: waitUntilTitleChangedTo("Foo", 10)
  Actual: false
Expected: true

[Chris Dumez]

Comment on attachment 164376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164376&action=review

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
> +struct LoadFinishedData {

Should have a constructor and initialize its members.
Should also have a destructor (to delete the timer if non null)

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:86
> +    bool timerFired;

"timedOut" would be clearer in my opinion.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:98
> +static bool timerFiredForWaitUntilLoadFinished(void *userData)

Star on wrong side.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:113
> +bool EWK2UnitTestBase::waitUntilLoadFinished(double timeout)

The variable name should contain the unit, for clarity.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:133
>  struct TitleChangedData {

I think we should add a constructor/destructor here as well.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:148
> +        ecore_timer_del(data->timer);

Already deleted in waitUntilTitleChangedTo(), I don't think you need this here.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:155
> +static bool timerFiredForWaitUntilTitleChangedTo(void *userData)

Star on wrong side.

[Byungwoo Lee]

Comment on attachment 164376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164376&action=review

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
>> +struct LoadFinishedData {
> 
> Should have a constructor and initialize its members.
> Should also have a destructor (to delete the timer if non null)

Do you think that this should be a class type? or just structure with constructor or destructor?

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:86
>> +    bool timerFired;
> 
> "timedOut" would be clearer in my opinion.

Ok~ I'll change it.

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:98
>> +static bool timerFiredForWaitUntilLoadFinished(void *userData)
> 
> Star on wrong side.

Oops Thanks :)

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:113
>> +bool EWK2UnitTestBase::waitUntilLoadFinished(double timeout)
> 
> The variable name should contain the unit, for clarity.

I cannot understand. timeOut?

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:148
>> +        ecore_timer_del(data->timer);
> 
> Already deleted in waitUntilTitleChangedTo(), I don't think you need this here.

I think, if onTitleChanged() is called before the timer is fired, then removing timer will be better not to be fired.
But null assigning will be required. for double delete.

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:155
>> +static bool timerFiredForWaitUntilTitleChangedTo(void *userData)
> 
> Star on wrong side.

Ok~

[Chris Dumez]

Comment on attachment 164376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164376&action=review

>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
>>> +struct LoadFinishedData {
>> 
>> Should have a constructor and initialize its members.
>> Should also have a destructor (to delete the timer if non null)
> 
> Do you think that this should be a class type? or just structure with constructor or destructor?

I think you can keep it as struct since all members are public. We already do it this way in the rest of WebKit2 EFL.

>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:113
>>> +bool EWK2UnitTestBase::waitUntilLoadFinished(double timeout)
>> 
>> The variable name should contain the unit, for clarity.
> 
> I cannot understand. timeOut?

timeoutMsecs ? We have no idea what the time unit is currently. Seconds? milliseconds? If it is in milliseconds, does it really need to be a double type or can we simply use an integer type?

>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:148
>>> +        ecore_timer_del(data->timer);
>> 
>> Already deleted in waitUntilTitleChangedTo(), I don't think you need this here.
> 
> I think, if onTitleChanged() is called before the timer is fired, then removing timer will be better not to be fired.
> But null assigning will be required. for double delete.

I see you point but then again, you're not doing the same thing in onLoadFinished :) In the current state, it is inconsistent. Maybe do the same in onLoadFinished() then?

[Byungwoo Lee]

Comment on attachment 164376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164376&action=review

>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
>>>> +struct LoadFinishedData {
>>> 
>>> Should have a constructor and initialize its members.
>>> Should also have a destructor (to delete the timer if non null)
>> 
>> Do you think that this should be a class type? or just structure with constructor or destructor?
> 
> I think you can keep it as struct since all members are public. We already do it this way in the rest of WebKit2 EFL.

Ok~ I'll apply it.

>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:113
>>>> +bool EWK2UnitTestBase::waitUntilLoadFinished(double timeout)
>>> 
>>> The variable name should contain the unit, for clarity.
>> 
>> I cannot understand. timeOut?
> 
> timeoutMsecs ? We have no idea what the time unit is currently. Seconds? milliseconds? If it is in milliseconds, does it really need to be a double type or can we simply use an integer type?

Ok I understand your point :).

As my searching, webkit uses double type for timer interval, So I followed it.
And interval for ecore_timer_add is also double type so using double type looks ok.

And is there any misunderstanding of unit with using double type as timer interval or timeout?
If it is integer type, there can be a misunderstanding issue.
But I think that double type for timer interval or timeout generally uses as second unit.
(1.0 means 1 second, and 2.5 means 2.5 seconds)
WebKit also doesn't have a unit for the name for interval parameter.
I think it is ok to use just timeout.

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:133
>>  struct TitleChangedData {
> 
> I think we should add a constructor/destructor here as well.

Ok~

>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:148
>>>> +        ecore_timer_del(data->timer);
>>> 
>>> Already deleted in waitUntilTitleChangedTo(), I don't think you need this here.
>> 
>> I think, if onTitleChanged() is called before the timer is fired, then removing timer will be better not to be fired.
>> But null assigning will be required. for double delete.
> 
> I see you point but then again, you're not doing the same thing in onLoadFinished :) In the current state, it is inconsistent. Maybe do the same in onLoadFinished() then?

Oops~ I missed it. Thanks :)

[Chris Dumez]

Comment on attachment 164376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164376&action=review

>>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:113
>>>>> +bool EWK2UnitTestBase::waitUntilLoadFinished(double timeout)
>>>> 
>>>> The variable name should contain the unit, for clarity.
>>> 
>>> I cannot understand. timeOut?
>> 
>> timeoutMsecs ? We have no idea what the time unit is currently. Seconds? milliseconds? If it is in milliseconds, does it really need to be a double type or can we simply use an integer type?
> 
> Ok I understand your point :).
> 
> As my searching, webkit uses double type for timer interval, So I followed it.
> And interval for ecore_timer_add is also double type so using double type looks ok.
> 
> And is there any misunderstanding of unit with using double type as timer interval or timeout?
> If it is integer type, there can be a misunderstanding issue.
> But I think that double type for timer interval or timeout generally uses as second unit.
> (1.0 means 1 second, and 2.5 means 2.5 seconds)
> WebKit also doesn't have a unit for the name for interval parameter.
> I think it is ok to use just timeout.

No, I don' think it is OK to use just "timeout". Please use unit in the argument name: e.g. "timeoutMsecs". Otherwise, we have no idea what unit is needed.

[Chris Dumez]

Comment on attachment 164376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164376&action=review

>>>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:113
>>>>>> +bool EWK2UnitTestBase::waitUntilLoadFinished(double timeout)
>>>>> 
>>>>> The variable name should contain the unit, for clarity.
>>>> 
>>>> I cannot understand. timeOut?
>>> 
>>> timeoutMsecs ? We have no idea what the time unit is currently. Seconds? milliseconds? If it is in milliseconds, does it really need to be a double type or can we simply use an integer type?
>> 
>> Ok I understand your point :).
>> 
>> As my searching, webkit uses double type for timer interval, So I followed it.
>> And interval for ecore_timer_add is also double type so using double type looks ok.
>> 
>> And is there any misunderstanding of unit with using double type as timer interval or timeout?
>> If it is integer type, there can be a misunderstanding issue.
>> But I think that double type for timer interval or timeout generally uses as second unit.
>> (1.0 means 1 second, and 2.5 means 2.5 seconds)
>> WebKit also doesn't have a unit for the name for interval parameter.
>> I think it is ok to use just timeout.
> 
> No, I don' think it is OK to use just "timeout". Please use unit in the argument name: e.g. "timeoutMsecs". Otherwise, we have no idea what unit is needed.

I checked the ecore doc, it should actually be "timeoutSeconds" since the timeout is in seconds, not milliseconds. See that it is not obvious to someone using your API.

[Byungwoo Lee]

Comment on attachment 164376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164376&action=review

>>>>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:113
>>>>>>> +bool EWK2UnitTestBase::waitUntilLoadFinished(double timeout)
>>>>>> 
>>>>>> The variable name should contain the unit, for clarity.
>>>>> 
>>>>> I cannot understand. timeOut?
>>>> 
>>>> timeoutMsecs ? We have no idea what the time unit is currently. Seconds? milliseconds? If it is in milliseconds, does it really need to be a double type or can we simply use an integer type?
>>> 
>>> Ok I understand your point :).
>>> 
>>> As my searching, webkit uses double type for timer interval, So I followed it.
>>> And interval for ecore_timer_add is also double type so using double type looks ok.
>>> 
>>> And is there any misunderstanding of unit with using double type as timer interval or timeout?
>>> If it is integer type, there can be a misunderstanding issue.
>>> But I think that double type for timer interval or timeout generally uses as second unit.
>>> (1.0 means 1 second, and 2.5 means 2.5 seconds)
>>> WebKit also doesn't have a unit for the name for interval parameter.
>>> I think it is ok to use just timeout.
>> 
>> No, I don' think it is OK to use just "timeout". Please use unit in the argument name: e.g. "timeoutMsecs". Otherwise, we have no idea what unit is needed.
> 
> I checked the ecore doc, it should actually be "timeoutSeconds" since the timeout is in seconds, not milliseconds. See that it is not obvious to someone using your API.

Yes, I already told that the unit is second but you still regard it as msec.
Maybe you want to tell me that someone can misunderstand the unit, and you are the example. :)
I'll change it.

[Byungwoo Lee]

Created attachment 164554 [details]
Patch

[Chris Dumez]

Comment on attachment 164554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164554&action=review

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
> +    LoadFinishedData()

Why not pass the timeout to the constructor and create the timer in the constructor as well?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:149
> +    TitleChangedData(const CString& title)

I think we would avoid an extra copy if we used const char* instead of CString as constructor argument type.

[Kenneth Rohde Christiansen]

Comment on attachment 164554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164554&action=review

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
> +    LoadFinishedData()

LoadFinishedData(int timeoutSeconds, Ecore_Task_Cb callback) ?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:87
> +        , timeOut(false)

didTimeOut?

[Byungwoo Lee]

Comment on attachment 164554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164554&action=review

>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
>>> +    LoadFinishedData()
>> 
>> Why not pass the timeout to the constructor and create the timer in the constructor as well?
> 
> LoadFinishedData(int timeoutSeconds, Ecore_Task_Cb callback) ?

@Christophe
Now i'm confused about those.
I think this struct have no need to maintain the timeoutSeconds or callback.
Please check the usage.

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:87
>> +        , timeOut(false)
> 
> didTimeOut?

yes it might be more clear. :)

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:149
>> +    TitleChangedData(const CString& title)
> 
> I think we would avoid an extra copy if we used const char* instead of CString as constructor argument type.

Ok. good point. I'll change it to const char*

[Kenneth Rohde Christiansen]

Comment on attachment 164554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164554&action=review

>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
>>>> +    LoadFinishedData()
>>> 
>>> Why not pass the timeout to the constructor and create the timer in the constructor as well?
>> 
>> LoadFinishedData(int timeoutSeconds, Ecore_Task_Cb callback) ?
> 
> @Christophe
> Now i'm confused about those.
> I think this struct have no need to maintain the timeoutSeconds or callback.
> Please check the usage.

it could do

LoadFinishedData(int timeoutSeconds, Ecore_Task_Cb callback) {
    timer = ecore_timer_add(timeoutSeconds, reinterpret_cast<Ecore_Task_Cb>(callback), this);
}

[Byungwoo Lee]

Comment on attachment 164554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164554&action=review

>>>>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
>>>>> +    LoadFinishedData()
>>>> 
>>>> Why not pass the timeout to the constructor and create the timer in the constructor as well?
>>> 
>>> LoadFinishedData(int timeoutSeconds, Ecore_Task_Cb callback) ?
>> 
>> @Christophe
>> Now i'm confused about those.
>> I think this struct have no need to maintain the timeoutSeconds or callback.
>> Please check the usage.
> 
> it could do
> 
> LoadFinishedData(int timeoutSeconds, Ecore_Task_Cb callback) {
>     timer = ecore_timer_add(timeoutSeconds, reinterpret_cast<Ecore_Task_Cb>(callback), this);
> }

Oh, its a better solution!
Thanks for the clarification :)

[Byungwoo Lee]

Created attachment 164589 [details]
Patch

[Chris Dumez]

Comment on attachment 164589 [details]
Patch

LGTM. Thanks.

[Kenneth Rohde Christiansen]

Comment on attachment 164589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164589&action=review

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:115
> +
> +    if (data->timer) {
> +        ecore_timer_del(data->timer);
> +        data->timer = 0;
> +    }

Is the LoadFinishedData going to be reused? or is there any need for the above? just wondering

[Gyuyoung Kim]

Comment on attachment 164589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164589&action=review

> Source/WebKit2/ChangeLog:3
> +        waitUntilTitleChangedTo() and waitUntilLoadFinished() needs timeout.

Missing to add [EFL][WK2] prefix.

[Gyuyoung Kim]

Comment on attachment 164589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164589&action=review

>> Source/WebKit2/ChangeLog:3
>> +        waitUntilTitleChangedTo() and waitUntilLoadFinished() needs timeout.
> 
> Missing to add [EFL][WK2] prefix.

And also, please use @samsung.com account.

[Byungwoo Lee]

Comment on attachment 164589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164589&action=review

>>> Source/WebKit2/ChangeLog:3
>>> +        waitUntilTitleChangedTo() and waitUntilLoadFinished() needs timeout.
>> 
>> Missing to add [EFL][WK2] prefix.
> 
> And also, please use @samsung.com account.

Ok~ I'll change it. Thank you :)

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:115
>> +    }
> 
> Is the LoadFinishedData going to be reused? or is there any need for the above? just wondering

If the 'onLoadFinished' is invoked before the 'timeoutWhileWaitingUntilLoadFinished',
the timer is explicitly destroyed for preventing additional timeout callback.

Actualy the timeout callback also has some status checking whether the load is finished or not.
But I thought that this is more clear :)

[Byungwoo Lee]

Created attachment 164678 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164678 [details]
Patch

Clearing flags on attachment: 164678

Committed r128975: <http://trac.webkit.org/changeset/128975>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.