Bug 97861 – [EFL][WK2] Move timeout callback to the CallbackDataTimer class.

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

[Byungwoo Lee]

Timeout callback implementation can be moved to the CallbackDataTimer class.

[Byungwoo Lee]

Created attachment 166150 [details]
Patch

[Byungwoo Lee]

Created attachment 166264 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 166264 [details]
Patch

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

Looks good refactoring.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:85
> +    CallbackDataTimer(double timeoutSeconds)

Missing explicit keyword

[Byungwoo Lee]

Created attachment 167036 [details]
Patch

[Byungwoo Lee]

(In reply to comment #3)
> (From update of attachment 166264 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166264&action=review
> 
> Looks good refactoring.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:85
> > +    CallbackDataTimer(double timeoutSeconds)
> 
> Missing explicit keyword
I added this~

[Gyuyoung Kim]

Comment on attachment 167036 [details]
Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 167036 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:117
> +    virtual void onTimeOut() { }

I think it would be good to add onTimeOut() when it is needed actually. Please remove this before landing.

[Byungwoo Lee]

(In reply to comment #7)
> (From update of attachment 167036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167036&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:117
> > +    virtual void onTimeOut() { }
> 
> I think it would be good to add onTimeOut() when it is needed actually. Please remove this before landing.

Ok~ I'll remove it.

[Kenneth Rohde Christiansen]

> Ok~ I'll remove it.

What is up with all those ~ signs? I have no idea what this might even mean.

[Gyuyoung Kim]

(In reply to comment #9)
> > Ok~ I'll remove it.
> 
> What is up with all those ~ signs? I have no idea what this might even mean.

Do you mean you can't  know what this patch mean ?  I understood this patch moves timeout callback   into the CallbackDataTimer class as member variable. Because waitUntilXXX functions should pass its callback function to  ctor of CallbackDataTimer. Beside all callback functions do same work. Do you think this patch is redundant ?

[Byungwoo Lee]

(In reply to comment #10)
> (In reply to comment #9)
> > > Ok~ I'll remove it.
> > 
> > What is up with all those ~ signs? I have no idea what this might even mean.
> 
> Do you mean you can't  know what this patch mean ?  I understood this patch moves timeout callback   into the CallbackDataTimer class as member variable. Because waitUntilXXX functions should pass its callback function to  ctor of CallbackDataTimer. Beside all callback functions do same work. Do you think this patch is redundant ?

Maybe kenneth asked about the '~' character that I used in the comment. (is it?)

It means nothing, just like dot character.
If you have some confusion about the character, I'll not use it :)

[Gyuyoung Kim]

Oops, if so, sorry about misunderstanding. :-)

[Byungwoo Lee]

Created attachment 167752 [details]
Patch

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> Oops, if so, sorry about misunderstanding. :-)

Exactly it is not really used in English so better not confuse people :-) It could be a "." for me or a ";" no idea.

[Byungwoo Lee]

(In reply to comment #14)
> (In reply to comment #12)
> > Oops, if so, sorry about misunderstanding. :-)
> 
> Exactly it is not really used in English so better not confuse people :-) It could be a "." for me or a ";" no idea.

Thanks for your kind comment. I'll remember that :-)

[WebKit Review Bot]

Comment on attachment 167752 [details]
Patch

Clearing flags on attachment: 167752

Committed r130836: <http://trac.webkit.org/changeset/130836>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.