Bug 93890 – [EFL] [WK2] Add unit tests for vibration_client_callbacks_set API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Sudarsana Nagineni (babu)]

Unit tests for vibration_client_callbacks_set API should be added.

[Sudarsana Nagineni (babu)]

Created attachment 158803 [details]
Patch

Patch covers unit testing of the Vibration API.

[Chris Dumez]

Comment on attachment 158803 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:78
> +    if (vibrationTime == *vibrationTimeExpected)

EXPECT_EQ(vibrationTime, *vibrationTimeExpected); ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:79
> +        vibrationEventReceived = true;

Should be set unconditionally I believe

[Sudarsana Nagineni (babu)]

Comment on attachment 158803 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:78
>> +    if (vibrationTime == *vibrationTimeExpected)
> 
> EXPECT_EQ(vibrationTime, *vibrationTimeExpected); ?

No. There is one case where I am passing an array of values to check alternating periods of time the vibrate method is called(eg., navigator.vibrate([200, 100, 500]). In this case the test should pass only when the callback received with vibrationTime 500 ms. Using EXPECT_EQ will cause a failure in this particular case.

[Chris Dumez]

Comment on attachment 158803 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:78
>>> +    if (vibrationTime == *vibrationTimeExpected)
>> 
>> EXPECT_EQ(vibrationTime, *vibrationTimeExpected); ?
> 
> No. There is one case where I am passing an array of values to check alternating periods of time the vibrate method is called(eg., navigator.vibrate([200, 100, 500]). In this case the test should pass only when the callback received with vibrationTime 500 ms. Using EXPECT_EQ will cause a failure in this particular case.

Ok, nevermind.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:87
> +static void loadVibrationHTMLString(Evas_Object* webView, const char* vibrationPattern, bool iterateLoop)

I think "iterateLoop" name is not explicit enough. Maybe something like "waitForVibrationEvent" ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:96
> +    ewk_view_html_string_load(webView, eina_strbuf_string_steal(buffer), 0, 0);

Leak? I believe you want to use eina_strbuf_string_get().

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:116
> +    ASSERT_TRUE(vibrationEventReceived);

How do you know that cancelVibrationCallback() got called and not vibrateCallback() since you're using the same boolean?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:119
> +    loadVibrationHTMLString(webView(), "[200, 100, 5000]", true);

How do you know your callback was called several times?

[Thiago Marcos P. Santos]

Comment on attachment 158803 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:97
> +    eina_strbuf_string_free(buffer);

This wont work because you called eina_strbuf_string_steal() before.

[Chris Dumez]

Comment on attachment 158803 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:97
>> +    eina_strbuf_string_free(buffer);
> 
> This wont work because you called eina_strbuf_string_steal() before.

Call eina_strbuf_free() instead.

[Sudarsana Nagineni (babu)]

Created attachment 159009 [details]
Patch

Add more test coverage for API by fixing review comments.

[Sudarsana Nagineni (babu)]

Comment on attachment 158803 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:87
>> +static void loadVibrationHTMLString(Evas_Object* webView, const char* vibrationPattern, bool iterateLoop)
> 
> I think "iterateLoop" name is not explicit enough. Maybe something like "waitForVibrationEvent" ?

Okay.

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:97
>>> +    eina_strbuf_string_free(buffer);
>> 
>> This wont work because you called eina_strbuf_string_steal() before.
> 
> Call eina_strbuf_free() instead.

fixed.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:116
>> +    ASSERT_TRUE(vibrationEventReceived);
> 
> How do you know that cancelVibrationCallback() got called and not vibrateCallback() since you're using the same boolean?

fixed.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:119
>> +    loadVibrationHTMLString(webView(), "[200, 100, 5000]", true);
> 
> How do you know your callback was called several times?

Added implementation to check this case now.

[Chris Dumez]

Comment on attachment 159009 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:73
> +    bool vibrateCb; /**< Whether the vibration event received */

boolean variable names should start with "is", "has", "was" or similar as per coding style. The name is not very clear. Should we use this comment style /**< */ for undocumented private code?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:122
> +    loadVibrationHTMLString(webView(), 0, true, &data);

Shouldn't it be "0" instead of 0? Now, it seems you pass 0 (null) to eina_strbuf_append_printf().

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:126
> +    data.vibrateCbCount = 0;

I guess you would do this in loadVibrationHTMLString() with the other resetting?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:145
> +    loadVibrationHTMLString(webView(), 0, false, &data);

is passing 0 really correct here?

[Sudarsana Nagineni (babu)]

Created attachment 159040 [details]
Patch

Take Chris review comments into consideration.

[Chris Dumez]

Comment on attachment 159040 [details]
Patch

LGTM.

[Thiago Marcos P. Santos]

Comment on attachment 159040 [details]
Patch

LGTM, thanks!

[Kenneth Rohde Christiansen]

Comment on attachment 159040 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:73
> +    bool isVibrateCbReceived; // Whether the vibration event received.

it is called 'was received', but you could say didReceiveVibrateCallback;

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:75
> +    int vibrateCbCount; // Vibrate callbacks count.

unsigned?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:95
> +    static const char* vibrationHTMLString =

This is more content than html, why not just const char* content; ? Why it is static?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:104
> +    eina_strbuf_append_printf(buffer, vibrationHTMLString, vibrationPattern);
> +    ewk_view_html_string_load(webView, eina_strbuf_string_get(buffer), 0, 0);

you could also just load the string ewk_view_load( ... using a data string "data:text/html,<html>..."

[Thiago Marcos P. Santos]

Comment on attachment 159040 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:104
>> +    ewk_view_html_string_load(webView, eina_strbuf_string_get(buffer), 0, 0);
> 
> you could also just load the string ewk_view_load( ... using a data string "data:text/html,<html>..."

Looks like the content is dynamically modified at "eina_strbuf_append_printf".

[Sudarsana Nagineni (babu)]

Created attachment 159088 [details]
Patch

Take Kenneth's feedback into consideration.

[Sudarsana Nagineni (babu)]

Comment on attachment 159040 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:73
>> +    bool isVibrateCbReceived; // Whether the vibration event received.
> 
> it is called 'was received', but you could say didReceiveVibrateCallback;

fixed.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:75
>> +    int vibrateCbCount; // Vibrate callbacks count.
> 
> unsigned?

fixed.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:95
>> +    static const char* vibrationHTMLString =
> 
> This is more content than html, why not just const char* content; ? Why it is static?

fixed.

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:104
>>> +    ewk_view_html_string_load(webView, eina_strbuf_string_get(buffer), 0, 0);
>> 
>> you could also just load the string ewk_view_load( ... using a data string "data:text/html,<html>..."
> 
> Looks like the content is dynamically modified at "eina_strbuf_append_printf".

yes, the content is dynamically modified every time the function is called with different value in vibration time.

[Kenneth Rohde Christiansen]

Comment on attachment 159088 [details]
Patch

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

Very slow test, is that really needed?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:75
> +    unsigned int vibrateCbCount; // Vibrate callbacks count.

Style guide says to just write "unsigned" and not "unsigned int".

unsigned vibrateCalledCount;

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:120
> +    // Vibrate for 5 seconds.
> +    loadVibrationHTMLString(webView(), "5000", true, &data);
> +    ASSERT_TRUE(data.didReceiveVibrateCallback);

Can this result in flakyness? 5 seconds is a long time for a test. Why not let the test run say 300 mseconds and wait 400 mseconds here?

[Thiago Marcos P. Santos]

Comment on attachment 159088 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:120
>> +    ASSERT_TRUE(data.didReceiveVibrateCallback);
> 
> Can this result in flakyness? 5 seconds is a long time for a test. Why not let the test run say 300 mseconds and wait 400 mseconds here?

The test won't wait for 5 seconds, but ask the embedder to vibrate for 5 seconds. He is checking here if the what the JavaScript asks and what the embedder is getting matches.

[Sudarsana Nagineni (babu)]

Comment on attachment 159088 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:75
>> +    unsigned int vibrateCbCount; // Vibrate callbacks count.
> 
> Style guide says to just write "unsigned" and not "unsigned int".
> 
> unsigned vibrateCalledCount;

I just followed the existing EFL style. I'm going to fix it. Thanks for pointing out.

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:120
>>> +    ASSERT_TRUE(data.didReceiveVibrateCallback);
>> 
>> Can this result in flakyness? 5 seconds is a long time for a test. Why not let the test run say 300 mseconds and wait 400 mseconds here?
> 
> The test won't wait for 5 seconds, but ask the embedder to vibrate for 5 seconds. He is checking here if the what the JavaScript asks and what the embedder is getting matches.

You are correct Thiago. I was about to write the same here. :)

[Sudarsana Nagineni (babu)]

Created attachment 159097 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 159097 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:75
> +    unsigned vibrateCbCount; // Vibrate callbacks count.

s/Cb/Called/

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:107
> +    eina_strbuf_free(buffer);
> +    if (!waitForVibrationEvent)
> +        return;

Add newline before if

[Sudarsana Nagineni (babu)]

Created attachment 159099 [details]
patch

[WebKit Review Bot]

Comment on attachment 159099 [details]
patch

Clearing flags on attachment: 159099

Committed r125893: <http://trac.webkit.org/changeset/125893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.