Vibration can not cancel during pattern vibration is working. If resting time which are even numbers of pattern m_isVibraing is false, therefore if cancel is called it is returned. In addition, m_timerStart need to stop in the cancelVibration() with m_timerStop.stop(). If cancelVibration is called right after m_timerStart is fired, timerStartFired function can be called even if vibration is already canceled because of timing issue of timer.
Created attachment 205061 [details] Patch
Comment on attachment 205061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205061&action=review > Source/WebCore/Modules/vibration/Vibration.cpp:-94 > - cancelVibration(); Isn't it better to call cancelVibration() ? I think cancelVibration() does same work. > Source/WebCore/Modules/vibration/Vibration.cpp:128 > + if (!m_pattern.size()) Why not use m_pattern.isEmpty() ? > LayoutTests/vibration/cancelVibrationDuringPatternVibrating.html:14 > + navigator.vibrate([100, 100, 100, 100, 100]); Looks wrong indentation. > LayoutTests/vibration/cancelVibrationDuringPatternVibrating.html:24 > + finishJSTest(); ditto.
(In reply to comment #2) > (From update of attachment 205061 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205061&action=review > > > Source/WebCore/Modules/vibration/Vibration.cpp:-94 > > - cancelVibration(); > > Isn't it better to call cancelVibration() ? I think cancelVibration() does same work. m_pattern is cleared in the cancelVibration but suspenVibration need to add left time to the pattern. therefore It's little bit different. > > > Source/WebCore/Modules/vibration/Vibration.cpp:128 > > + if (!m_pattern.size()) > > Why not use m_pattern.isEmpty() ? I'll change to isEmpty().
Created attachment 205466 [details] Patch
Comment on attachment 205466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205466&action=review LGTM but a few nits: > Source/WebCore/ChangeLog:8 > + Vibration can not cancel during pattern vibration is working. If resting time which are "Vibration cannot be canceled during pattern vibration." > Source/WebCore/ChangeLog:9 > + even numbers of pattern m_isVibraing is false, therefore if cancel is called it is returned. "... m_isVibrating will be false and cancel() will thus return early." > Source/WebCore/ChangeLog:10 > + In addition, m_timerStart need to stop in the cancelVibration() with m_timerStop.stop(). "In addition, m_timerStart needs to be stopped in the cancelVibration()." > Source/WebCore/ChangeLog:11 > + If cancelVibration is called right after m_timerStart is fired, timerStartFired "cancelVibration()" > LayoutTests/ChangeLog:3 > + Vibration can not cancel during pattern is working "Vibration cannot be canceled during pattern vibration" > LayoutTests/ChangeLog:8 > + Add a test case for cancel vibtion during pattern vibration is working. "cancel vibtion during pattern vibration is working" -> "vibration cancellation during pattern vibration." > LayoutTests/ChangeLog:9 > + In addition, restore visibilityState before excuting cancelVibration-after-pagevisibility-changed-to-hidden.html. "executing"
Created attachment 205470 [details] Patch
Comment on attachment 205470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205470&action=review >> m_pattern is cleared in the cancelVibration but suspenVibration need to add left time to the pattern. therefore It's little bit different. How about adding new private function in order to process this logic ? m_timerStart.stop(); m_timerStop.stop(); m_vibrationClient->cancelVibration(); m_isVibrating = false; > Source/WebCore/ChangeLog:8 > + Vibration can not cancel during pattern vibration is working. If resting time which are Still can not -> cannot.
Comment on attachment 205470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205470&action=review >> Source/WebCore/ChangeLog:8 >> + Vibration can not cancel during pattern vibration is working. If resting time which are > > Still can not -> cannot. Technically I believe both are valid. I agree that 'cannot' seems more commonly used though.
(In reply to comment #7) > (From update of attachment 205470 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205470&action=review > > >> m_pattern is cleared in the cancelVibration but suspenVibration need to add left time to the pattern. therefore It's little bit different. > > How about adding new private function in order to process this logic ? > > m_timerStart.stop(); > m_timerStop.stop(); > m_vibrationClient->cancelVibration(); > m_isVibrating = false; > No problem. Thanks chris and gyuyoung. :)
Created attachment 205478 [details] Patch
Comment on attachment 205478 [details] Patch LGTM. Please land after finishing all ews.
Comment on attachment 205478 [details] Patch Clearing flags on attachment: 205478 Committed r152070: <http://trac.webkit.org/changeset/152070>
All reviewed patches have been landed. Closing bug.