Bug 93890 - [EFL] [WK2] Add unit tests for vibration_client_callbacks_set API
Summary: [EFL] [WK2] Add unit tests for vibration_client_callbacks_set API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sudarsana Nagineni (babu)
URL:
Keywords:
Depends on:
Blocks: 90451 91077
  Show dependency treegraph
 
Reported: 2012-08-13 13:26 PDT by Sudarsana Nagineni (babu)
Modified: 2012-08-17 07:04 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.44 KB, patch)
2012-08-16 06:58 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2012-08-16 23:18 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (4.78 KB, patch)
2012-08-17 01:16 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2012-08-17 04:42 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2012-08-17 05:20 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
patch (4.85 KB, patch)
2012-08-17 05:28 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2012-08-13 13:26:29 PDT
Unit tests for vibration_client_callbacks_set API should be added.
Comment 1 Sudarsana Nagineni (babu) 2012-08-16 06:58:43 PDT
Created attachment 158803 [details]
Patch

Patch covers unit testing of the Vibration API.
Comment 2 Chris Dumez 2012-08-16 07:04:53 PDT
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
Comment 3 Sudarsana Nagineni (babu) 2012-08-16 07:37:08 PDT
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.
Comment 4 Chris Dumez 2012-08-16 07:51:53 PDT
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?
Comment 5 Thiago Marcos P. Santos 2012-08-16 09:38:31 PDT
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.
Comment 6 Chris Dumez 2012-08-16 09:45:16 PDT
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.
Comment 7 Sudarsana Nagineni (babu) 2012-08-16 23:18:52 PDT
Created attachment 159009 [details]
Patch

Add more test coverage for API by fixing review comments.
Comment 8 Sudarsana Nagineni (babu) 2012-08-16 23:24:40 PDT
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.
Comment 9 Chris Dumez 2012-08-16 23:56:28 PDT
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?
Comment 10 Sudarsana Nagineni (babu) 2012-08-17 01:16:44 PDT
Created attachment 159040 [details]
Patch

Take Chris review comments into consideration.
Comment 11 Chris Dumez 2012-08-17 01:19:23 PDT
Comment on attachment 159040 [details]
Patch

LGTM.
Comment 12 Thiago Marcos P. Santos 2012-08-17 02:51:30 PDT
Comment on attachment 159040 [details]
Patch

LGTM, thanks!
Comment 13 Kenneth Rohde Christiansen 2012-08-17 03:02:25 PDT
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>..."
Comment 14 Thiago Marcos P. Santos 2012-08-17 03:15:08 PDT
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".
Comment 15 Sudarsana Nagineni (babu) 2012-08-17 04:42:52 PDT
Created attachment 159088 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 16 Sudarsana Nagineni (babu) 2012-08-17 04:53:48 PDT
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.
Comment 17 Kenneth Rohde Christiansen 2012-08-17 04:54:35 PDT
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?
Comment 18 Thiago Marcos P. Santos 2012-08-17 05:12:44 PDT
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.
Comment 19 Sudarsana Nagineni (babu) 2012-08-17 05:16:33 PDT
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. :)
Comment 20 Sudarsana Nagineni (babu) 2012-08-17 05:20:36 PDT
Created attachment 159097 [details]
Patch
Comment 21 Kenneth Rohde Christiansen 2012-08-17 05:23:35 PDT
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
Comment 22 Sudarsana Nagineni (babu) 2012-08-17 05:28:30 PDT
Created attachment 159099 [details]
patch
Comment 23 WebKit Review Bot 2012-08-17 07:04:24 PDT
Comment on attachment 159099 [details]
patch

Clearing flags on attachment: 159099

Committed r125893: <http://trac.webkit.org/changeset/125893>
Comment 24 WebKit Review Bot 2012-08-17 07:04:32 PDT
All reviewed patches have been landed.  Closing bug.