Bug 129598 – Second execution of navigator.vibrate() does not wok.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit Misc.

[[e-mail]]

Vibration works on requests of odd-number times and does not work on requests of even-number times.

[Jinwoo Jeong]

Created attachment 225632 [details]
Patch

[Ryuan Choi]

Comment on attachment 225632 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

I think that you need to add test case.

[Jinwoo Song]

Comment on attachment 225632 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        m_isVibrate should be set as false, when m_pattern has no element.

Bug title is different from this.

>> Source/WebCore/ChangeLog:7
>> +
> 
> I think that you need to add test case.

It would be good to add explanation for this patch.

> Source/WebCore/Modules/vibration/Vibration.cpp:53
> +    if (m_isVibrating || (length == 1 && !m_pattern[0]))

s/m_pattern/pattern?

[Jinwoo Jeong]

Created attachment 225737 [details]
Patch

[Jinwoo Jeong]

(In reply to comment #2)
> (From update of attachment 225632 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225632&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> I think that you need to add test case.

Add a unit test case.

[Jinwoo Jeong]

Created attachment 225742 [details]
Patch

[Jinwoo Jeong]

Created attachment 225923 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 225923 [details]
Patch

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

> Source/WebCore/ChangeLog:-1099
> -

Do not touch unrelated line.

> Source/WebKit2/ChangeLog:8
> +        Add a vibration unit test regression after r164266.

I don't see why r164266 was influenced on vibration feature.

[CoordinatedGraphics][EFL] Remove view_source functions.
http://trac.webkit.org/changeset/164266

> Source/WebKit2/ChangeLog:9
> +        Tighten condition of existing tests.

Add a new line.

[Gyuyoung Kim]

Comment on attachment 225923 [details]
Patch

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

> Source/WebCore/Modules/vibration/Vibration.cpp:70
> +    stopVibration();

Why should we call stopVibration() when m_isVibrating is false ?

[Jinwoo Song]

Comment on attachment 225923 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        If the vibrating starts with the VibrationPattern that has single element.

s/the/a, s/./,

> Source/WebCore/ChangeLog:9
> +        m_isVibrating could not be set as false on Vibration::timerStopFired,

s/,/.

> Source/WebCore/Modules/vibration/Vibration.cpp:55
>          return true;

You should not return here after canceling vibration. If a new vibration pattern is set, then existing vibration is canceled and the new vibration should work.

[[e-mail]]

(In reply to comment #9)
> (From update of attachment 225923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225923&action=review
> 
> > Source/WebCore/Modules/vibration/Vibration.cpp:70
> > +    stopVibration();
> 
> Why should we call stopVibration() when m_isVibrating is false ?

Yes, I will add a checking statement before calling stopVibration().

[Jinwoo Jeong]

(In reply to comment #10)
> (From update of attachment 225923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225923&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        If the vibrating starts with the VibrationPattern that has single element.
> 
> s/the/a, s/./,
Done.
> 
> > Source/WebCore/ChangeLog:9
> > +        m_isVibrating could not be set as false on Vibration::timerStopFired,
> 
> s/,/.
Done.
> 
> > Source/WebCore/Modules/vibration/Vibration.cpp:55
> >          return true;
> 
> You should not return here after canceling vibration. If a new vibration pattern is set, then existing vibration is canceled and the new vibration should work.

Yes, new vibration should work.

[Jinwoo Jeong]

Created attachment 225962 [details]
Patch

[Jinwoo Jeong]

Created attachment 226270 [details]
Patch

[Jinwoo Song]

Comment on attachment 226270 [details]
Patch

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

> Source/WebCore/Modules/vibration/Vibration.cpp:48
>  {

Spec. describes the algorithm for vibrate() method, so it would be better to follow the sequences.
http://www.w3.org/TR/2014/WD-vibration-20140211/

> Source/WebCore/Modules/vibration/Vibration.cpp:51
> +    // And if pattern is an empty list or time is 0, vibration have to be canceled.

s/And if/If

> Source/WebCore/Modules/vibration/Vibration.cpp:57
> +    // Pre-exsiting instance need to be canceled when vibrate() is called also.

Remove 'also'.

[[e-mail]]

*** This bug has been marked as a duplicate of bug 130059 ***