Created attachment 158803 [details] Patch Patch covers unit testing of the Vibration API.

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 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 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 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 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.

Created attachment 159009 [details] Patch Add more test coverage for API by fixing review comments.

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 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?

Created attachment 159040 [details] Patch Take Chris review comments into consideration.

Comment on attachment 159040 [details] Patch LGTM.

Comment on attachment 159040 [details] Patch LGTM, thanks!

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 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".

Created attachment 159088 [details] Patch Take Kenneth's feedback into consideration.

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 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 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 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. :)

Created attachment 159097 [details] Patch

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

Created attachment 159099 [details] patch

Comment on attachment 159099 [details] patch Clearing flags on attachment: 159099 Committed r125893: <http://trac.webkit.org/changeset/125893>

All reviewed patches have been landed. Closing bug.