Bug 118288 – vibrate function have to return boolean value.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kihong Kwon]

Latest Vibration API spec is changed. It is LC now.
Therefore Vibration API need to align with the spec.

[Kihong Kwon]

Created attachment 205883 [details]
Patch

[EFL EWS Bot]

Comment on attachment 205883 [details]
Patch

Attachment 205883 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1015379

[EFL EWS Bot]

Comment on attachment 205883 [details]
Patch

Attachment 205883 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1012584

[Gyuyoung Kim]

Comment on attachment 205883 [details]
Patch

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

r- because of build error and my comment.

> Source/WebCore/ChangeLog:11
> +        In addition, add maxVibrationDuration and maxVibrationPatternLength.

I think we need to handle this new variables on new bug because this patch is not trivial.

[Kihong Kwon]

(In reply to comment #4)
> (From update of attachment 205883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205883&action=review
> 
> r- because of build error and my comment.

I think efl doesn't do clean build in this case, It's same error with my local build.
After remove WebKitBuild direcory, there is no error.

> 
> > Source/WebCore/ChangeLog:11
> > +        In addition, add maxVibrationDuration and maxVibrationPatternLength.
> 
> I think we need to handle this new variables on new bug because this patch is not trivial.

I agree with this. I will make a new bug for it.

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 205883 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=205883&action=review
> > 
> > r- because of build error and my comment.
> 
> I think efl doesn't do clean build in this case, It's same error with my local build.
> After remove WebKitBuild direcory, there is no error.

If so, I will check it soon.

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 205883 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=205883&action=review
> > 
> > r- because of build error and my comment.

Kihong, I also check there is no build error locally. Please submit patch corresponding with the title.

[Kihong Kwon]

Created attachment 205962 [details]
Patch

[EFL EWS Bot]

Comment on attachment 205962 [details]
Patch

Attachment 205962 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1006921

[EFL EWS Bot]

Comment on attachment 205962 [details]
Patch

Attachment 205962 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/938948

[Gyuyoung Kim]

Comment on attachment 205962 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        According to latest Vibration API which is LC, vibrate() need to be return

need -> needs ? return -> returned ?

> Source/WebCore/Modules/vibration/NavigatorVibration.cpp:45
> +    return NavigatorVibration::vibrate(navigator, pattern);

I don't prefer to use unneeded local variables. Why not we use NavigatorVibration::vibrate(navigator, VibrationPattern(time)); ?

> Source/WebCore/Modules/vibration/Vibration.cpp:52
> +    if (length && !(length % 2))

Could you explain why this condition is needed ?

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

I think we need to keep existing comment for this condition.

> Source/WebCore/Modules/vibration/Vibration.cpp:56
>          cancelVibration();

Why you don't return false ?

> LayoutTests/ChangeLog:9
> +        cancelVibration-after-pagevisibility-changed-to-hidden.html is not proper.

Why ? There is still definition in the spec.

"When the visibilitychange event [PAGE-VISIBILITY] is dispatched at the Document in a browsing context, the user agent must cancel the pre-existing instance of the processing vibration patterns algorithm, if any."

> LayoutTests/ChangeLog:10
> +        And navigator-vibration.html need to be changed to align to the spec.

need -> needs ?

[Kihong Kwon]

(In reply to comment #11)
> (From update of attachment 205962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205962&action=review
> 
> > Source/WebCore/Modules/vibration/NavigatorVibration.cpp:45
> > +    return NavigatorVibration::vibrate(navigator, pattern);
> 
> I don't prefer to use unneeded local variables. Why not we use NavigatorVibration::vibrate(navigator, VibrationPattern(time)); ?

No problem.
> 
> > Source/WebCore/Modules/vibration/Vibration.cpp:52
> > +    if (length && !(length % 2))
> 
> Could you explain why this condition is needed ?

According to the spec
- If the length of pattern is even and is not zero, then remove the last entry in pattern.
> 
> > Source/WebCore/Modules/vibration/Vibration.cpp:55
> > +    if (m_isVibrating || (m_pattern.size() == 1 && !m_pattern[0]))
> 
> I think we need to keep existing comment for this condition.

OK.
> 
> > Source/WebCore/Modules/vibration/Vibration.cpp:56
> >          cancelVibration();
> 
> Why you don't return false ?

According to the spec, vibraiton cancel is also return true.
> 
> > LayoutTests/ChangeLog:9
> > +        cancelVibration-after-pagevisibility-changed-to-hidden.html is not proper.
> 
> Why ? There is still definition in the spec.
> 
> "When the visibilitychange event [PAGE-VISIBILITY] is dispatched at the Document in a browsing context, the user agent must cancel the pre-existing instance of the processing vibration patterns algorithm, if any."
> 

This test case is for checking that vibration can be canceled or not, even if visibility state is hidden. But according to the spec, when visibilitychange event is dispatched, pre-exsiting vibration have to be canceled automatically.

IMHO, we need to add new test case for page-visibility status when we add that part, but is's not included in this patch.

[Kihong Kwon]

Created attachment 205981 [details]
Patch

[EFL EWS Bot]

Comment on attachment 205981 [details]
Patch

Attachment 205981 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1012887

[EFL EWS Bot]

Comment on attachment 205981 [details]
Patch

Attachment 205981 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1025037

[Gyuyoung Kim]

(In reply to comment #12)

> This test case is for checking that vibration can be canceled or not, even if visibility state is hidden. But according to the spec, when visibilitychange event is dispatched, pre-exsiting vibration have to be canceled automatically.
> 
> IMHO, we need to add new test case for page-visibility status when we add that part, but is's not included in this patch.

This removal isn't related to this bug. I think you need to keep this until you're ready to add new test cases.

[Kihong Kwon]

(In reply to comment #16)
> (In reply to comment #12)
> 
> > This test case is for checking that vibration can be canceled or not, even if visibility state is hidden. But according to the spec, when visibilitychange event is dispatched, pre-exsiting vibration have to be canceled automatically.
> > 
> > IMHO, we need to add new test case for page-visibility status when we add that part, but is's not included in this patch.
> 
> This removal isn't related to this bug. I think you need to keep this until you're ready to add new test cases.

I think this test cases is related to this patch, because I align vibration api to the latest spec, and according to the spec, this TC is wrong.
- When page visibility is changed, vibration have to be canceled.
But this TC suppose to vibrating is not canceled when page visibility is changed to the hidden state.

[Gyuyoung Kim]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #12)
> > 
> > > This test case is for checking that vibration can be canceled or not, even if visibility state is hidden. But according to the spec, when visibilitychange event is dispatched, pre-exsiting vibration have to be canceled automatically.
> > > 
> > > IMHO, we need to add new test case for page-visibility status when we add that part, but is's not included in this patch.
> > 
> > This removal isn't related to this bug. I think you need to keep this until you're ready to add new test cases.
> 
> I think this test cases is related to this patch, because I align vibration api to the latest spec, and according to the spec, this TC is wrong.
> - When page visibility is changed, vibration have to be canceled.
> But this TC suppose to vibrating is not canceled when page visibility is changed to the hidden state.

If possible, IMHO, we need to land a patch corresponding with bug title. I feel the removal is away from this bug's aim. Of course, we sometimes have landed patches with trivial changes on previous bugs though, I would recommend to keep this test until you're ready to replace with new tests.

[Kihong Kwon]

> If possible, IMHO, we need to land a patch corresponding with bug title. I feel the removal is away from this bug's aim. Of course, we sometimes have landed patches with trivial changes on previous bugs though, I would recommend to keep this test until you're ready to replace with new tests.

I will this TC to the TestExpectations until I fix it.
Thanks :)

[Kihong Kwon]

Created attachment 206054 [details]
Patch

[EFL EWS Bot]

Comment on attachment 206054 [details]
Patch

Attachment 206054 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1029062

[EFL EWS Bot]

Comment on attachment 206054 [details]
Patch

Attachment 206054 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1027229

[Gyuyoung Kim]

Comment on attachment 206054 [details]
Patch

r=me as a step of adjusting latest vibration spec into WebKit.

[Gyuyoung Kim]

When patch is landed, I will restart efl buildbot. But, we need to file a bug regarding this ews problem.

[Kihong Kwon]

(In reply to comment #24)
> When patch is landed, I will restart efl buildbot. But, we need to file a bug regarding this ews problem.

I will file a bug about this issue.

[Kihong Kwon]

(In reply to comment #25)
> (In reply to comment #24)
> > When patch is landed, I will restart efl buildbot. But, we need to file a bug regarding this ews problem.
> 
> I will file a bug about this issue.

Bug is filed for this issue.(Bug 118390)

[WebKit Commit Bot]

Comment on attachment 206054 [details]
Patch

Clearing flags on attachment: 206054

Committed r152441: <http://trac.webkit.org/changeset/152441>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.