Latest Vibration API spec is changed. It is LC now. Therefore Vibration API need to align with the spec.
Created attachment 205883 [details] Patch
Comment on attachment 205883 [details] Patch Attachment 205883 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1015379
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
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.
(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.
(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.
(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.
Created attachment 205962 [details] Patch
Comment on attachment 205962 [details] Patch Attachment 205962 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1006921
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
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 ?
(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.
Created attachment 205981 [details] Patch
Comment on attachment 205981 [details] Patch Attachment 205981 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1012887
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
(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.
(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.
(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.
> 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 :)
Created attachment 206054 [details] Patch
Comment on attachment 206054 [details] Patch Attachment 206054 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1029062
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
Comment on attachment 206054 [details] Patch r=me as a step of adjusting latest vibration spec into WebKit.
When patch is landed, I will restart efl buildbot. But, we need to file a bug regarding this ews problem.
(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.
(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)
Comment on attachment 206054 [details] Patch Clearing flags on attachment: 206054 Committed r152441: <http://trac.webkit.org/changeset/152441>
All reviewed patches have been landed. Closing bug.