Currently Vibration has used two timers, But they does not work in same time. And in Vibration, there are some difference between algorithm defined in specification and implementation.
Created attachment 226363 [details] Patch
Comment on attachment 226363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226363&action=review > Source/WebCore/ChangeLog:3 > + Refactor Vibration algorithm to use only 1 timer. nit: s/1/one > Source/WebCore/ChangeLog:8 > + Currently Vibration has used 2 timer, Writing suggestion: 'Currently Vibration is using two timers,' > Source/WebCore/ChangeLog:13 > + Indeed this patch applies the algorithm and restrictions defined on specification. Writing suggestion: 'Also, this patch implement the missing part of the algorithm, which check the maximum length of the vibration pattern and the maximum duration of the vibration. > Source/WebCore/Modules/vibration/Vibration.cpp:32 > +const unsigned MaxVibrationDuration = 10000; Where these magic values come from? > Source/WebCore/Modules/vibration/Vibration.cpp:87 > return true; These lines can be removed as it is already checked. > Source/WebCore/Modules/vibration/Vibration.cpp:110 > + m_state = Idle; You may return here. > Source/WebCore/Modules/vibration/Vibration.cpp:111 > + else { Why don't you use switch-case statement here? > Source/WebCore/Modules/vibration/Vibration.h:-45 > - // FIXME : Add suspendVibration() and resumeVibration() to the page visibility feature, when the document.hidden attribute is changed. It's okay to remove these unused methods but It would be better to comment 'FIXME' for implementing page visibility feature.
Comment on attachment 226363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226363&action=review > Source/WebCore/Modules/vibration/Vibration.cpp:72 > + for (size_t i = 0; i < length; ++i) { > + if (sanitized[i] > MaxVibrationDuration) > + sanitized[i] = MaxVibrationDuration; > + } I suggest writing it like this: for (auto& duration : sanitized) duration = std::min(duration, MaxVibrationDuration);
> > Source/WebCore/Modules/vibration/Vibration.cpp:32 > > +const unsigned MaxVibrationDuration = 10000; > > Where these magic values come from? I referred to blink. And others is done.
Created attachment 226397 [details] Patch
Created attachment 226476 [details] Patch
Created attachment 226477 [details] Patch
Comment on attachment 226477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226477&action=review > Source/WebCore/Modules/vibration/Vibration.cpp:72 > + // Pre-exsiting instance need to be canceled when vibrate() is called. Typo: "exsiting". > Source/WebCore/Modules/vibration/Vibration.h:43 > + // FIXME : When visibilitychange event is dispatched, if a vibration is working, it should be canceled. No space before the ":" in "FIXME:". I don’t understand the meaning of the phrase “a vibration is working”. > Source/WebCore/Modules/vibration/Vibration.h:58 > + enum class State { > + Idle, > + Vibrating, > + Waiting > + }; I’d suggest putting this on a single line instead of 5 lines.
Created attachment 226636 [details] Patch
Created attachment 226637 [details] Patch
(In reply to comment #8) > (From update of attachment 226477 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226477&action=review > > > Source/WebCore/Modules/vibration/Vibration.cpp:72 > > + // Pre-exsiting instance need to be canceled when vibrate() is called. > > Typo: "exsiting". > > > Source/WebCore/Modules/vibration/Vibration.h:43 > > + // FIXME : When visibilitychange event is dispatched, if a vibration is working, it should be canceled. > > No space before the ":" in "FIXME:". > > I don’t understand the meaning of the phrase “a vibration is working”. > > > Source/WebCore/Modules/vibration/Vibration.h:58 > > + enum class State { > > + Idle, > > + Vibrating, > > + Waiting > > + }; > > I’d suggest putting this on a single line instead of 5 lines. Applied Darin's comment.
Comment on attachment 226637 [details] Patch Clearing flags on attachment: 226637 Committed r165598: <http://trac.webkit.org/changeset/165598>
All reviewed patches have been landed. Closing bug.
*** Bug 129598 has been marked as a duplicate of this bug. ***