Bug 117822 – Vibration cannot be canceled during pattern vibration.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kihong Kwon]

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.

[Kihong Kwon]

Created attachment 205061 [details]
Patch

[Gyuyoung Kim]

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.

[Kihong Kwon]

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

[Kihong Kwon]

Created attachment 205466 [details]
Patch

[Chris Dumez]

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"

[Kihong Kwon]

Created attachment 205470 [details]
Patch

[Gyuyoung Kim]

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.

[Chris Dumez]

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.

[Kihong Kwon]

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

[Kihong Kwon]

Created attachment 205478 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 205478 [details]
Patch

LGTM. Please land after finishing all ews.

[WebKit Commit Bot]

Comment on attachment 205478 [details]
Patch

Clearing flags on attachment: 205478

Committed r152070: <http://trac.webkit.org/changeset/152070>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.