[webkit-reviews] review granted: [Bug 170250] AX: Modern Media Controls Timeline slider should be operable : [Attachment 307395] Patch

[bugzilla-daemon at webkit.org]

Antoine Quint <graouts at apple.com> has granted Aaron Chu <aaron_chu at apple.com>'s
request for review:
Bug 170250: AX: Modern Media Controls Timeline slider should be operable
https://bugs.webkit.org/show_bug.cgi?id=170250

Attachment 307395: Patch

https://bugs.webkit.org/attachment.cgi?id=307395&action=review




--- Comment #18 from Antoine Quint <graouts at apple.com> ---
Comment on attachment 307395
  --> https://bugs.webkit.org/attachment.cgi?id=307395
Patch

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

> Source/WebCore/ChangeLog:55
> +	   (formatTimeByUnit):

Make sure this double ChangeLog entry gets fixed.

> Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:85
> +    _unitizeTime(value, unit)

While it's OK as-is, this method really needn't be on the Scrubber prototype,
it could just be a global function.

> Source/WebCore/Modules/modern-media-controls/controls/time-control.js:93
> +	   if (this.scrubber)

Is this ever not true?

> Source/WebCore/Modules/modern-media-controls/controls/time-label.js:59
> +	       if (this._timeControl)

Is this ever not true?

> Source/WebCore/Modules/modern-media-controls/main.js:57
> +    let result = {};
> +    const time = value || 0;
> +    const absTime = Math.abs(time);
> +    result.intSeconds = Math.floor(absTime % 60).toFixed(0);
> +    result.intMinutes = Math.floor((absTime / 60) % 60).toFixed(0);
> +    result.intHours = Math.floor(absTime / (60 * 60)).toFixed(0);
> +    return result;

Probably more idiomatic to write this as:

return {
    seconds: …,
    minutes: …,
    hours: …
};

> LayoutTests/ChangeLog:20
> +

Double ChangeLog entry here as well.