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.
Created attachment 164376 [details] Patch
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
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.
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~
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?
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 :)
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.
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.
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.
Created attachment 164554 [details] Patch
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.
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?
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*
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); }
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 :)
Created attachment 164589 [details] Patch
Comment on attachment 164589 [details] Patch LGTM. Thanks.
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
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.
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.
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 :)
Created attachment 164678 [details] Patch
Comment on attachment 164678 [details] Patch Clearing flags on attachment: 164678 Committed r128975: <http://trac.webkit.org/changeset/128975>
All reviewed patches have been landed. Closing bug.