Bug 130059 – Refactor Vibration algorithm to use only one timer.

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

[Jinwoo Jeong]

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.

[Jinwoo Jeong]

Created attachment 226363 [details]
Patch

[Jinwoo Song]

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.

[Darin Adler]

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);

[Jinwoo Jeong]

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

[Jinwoo Jeong]

Created attachment 226397 [details]
Patch

[Jinwoo Jeong]

Created attachment 226476 [details]
Patch

[Jinwoo Jeong]

Created attachment 226477 [details]
Patch

[Darin Adler]

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.

[Jinwoo Jeong]

Created attachment 226636 [details]
Patch

[Jinwoo Jeong]

Created attachment 226637 [details]
Patch

[Jinwoo Jeong]

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

[WebKit Commit Bot]

Comment on attachment 226637 [details]
Patch

Clearing flags on attachment: 226637

Committed r165598: <http://trac.webkit.org/changeset/165598>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[[e-mail]]

*** Bug 129598 has been marked as a duplicate of this bug. ***