[webkit-reviews] review granted: [Bug 134894] Disable ff/rw based on canPlayFastForward and canPlayFastRewind. : [Attachment 235195] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jeremy Jones <jeremyj-wk at apple.com>'s
request for review:
Bug 134894: Disable ff/rw based on canPlayFastForward and canPlayFastRewind.
https://bugs.webkit.org/show_bug.cgi?id=134894

Attachment 235195: Patch
https://bugs.webkit.org/attachment.cgi?id=235195&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235195&action=review


While I’m not an expert on our media code, I think I understand this well
enough to say review+

> Source/WebCore/html/HTMLMediaElement.h:192
> +    double minFastReverseRate();
> +    double maxFastForwardRate();

Should be const, I think.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:898
> +    return m_private ? m_private->maxFastForwardRate() : DBL_MAX;

In C++ code we’d normally use std::numeric_limits instead of old school numeric
limit MACROS. This one would be std::numeric_limits<double>::max(). Or you
could use an infinity instead. Also available from numeric_limits.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:903
> +    return m_private ? m_private->minFastReverseRate() : -DBL_MAX;

This one would be -std::numeric_limits<double>::max(). Or -infinity.

> Source/WebCore/platform/graphics/MediaPlayer.h:484
> +    double minFastReverseRate();
> +    double maxFastForwardRate();

Should be const, I think.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.h:115
> +    virtual double maxFastForwardRate() const override { return
m_cachedCanPlayFastForward ? DBL_MAX : 2.0; }
> +    virtual double minFastReverseRate() const override { return
m_cachedCanPlayFastReverse ? -DBL_MAX : 0.0; }

Same comments about DBL_MAX. Also, new overrides can and should be private
unless they need to be called non-polymorphically.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2698
> +	   } else if ([keyPath isEqualToString:@"canPlayFastReverse"])
> +	       function =
WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastReverseDidChange,
m_callback, [newValue boolValue]);
> +	   else if ([keyPath isEqualToString:@"canPlayFastForward"])
> +	       function =
WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastForwardDidChange,
m_callback, [newValue boolValue]);

In the future we should consider using lambdas all throughout the function
instead of WTF::bind.