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

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied 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 234900: Patch
https://bugs.webkit.org/attachment.cgi?id=234900&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234900&action=review


> Source/WebCore/html/HTMLMediaElement.cpp:3109
> +#if PLATFORM(IOS)
> +    if (!canPlayFastReverse() && rate  < 0)
> +	   rate = 0;
> +    if (!canPlayFastForward() && rate > 2)
> +	   rate = 2;
> +#endif

This is the wrong place to hard code maximum and minimum speeds, I doubt every
platform will have the same max/min. Instead of canPlayFastForward and
canPlayFastReverse, why don't you add something like maximumFastForwardRate and
minimumFastReverseRate?

Also, I think 0 and 2 are the wrong values even for AVFoundation. The header
docs for -[AVPlayerItem canPlayFastReverse] and -[AVPlayerItem
canPlayFastForward] they indicate if an item is limited to +1 and -1.

> Source/WebCore/html/HTMLMediaElement.cpp:4866
> +void HTMLMediaElement::mediaPlayerCanPlayFastReverseChanged(MediaPlayer*)
> +{
> +    scheduleEvent(eventNames().canplayfastreversechangeEvent);
> +}

We can't notify this way, this non-standard event will be visible to scripts.

> Source/WebCore/html/HTMLMediaElement.h:542
> +    virtual void mediaPlayerCanPlayFastReverseChanged(MediaPlayer*)
override;

Do we only care when the reverse rate minimum changes, what about max forward
rate?

I am not sure we need to a new callback (or two) just for this, couldn't you
use mediaPlayerCharacteristicChanged and check for a change?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:782
> +    [m_avPlayer.get() addObserver:m_objcObserver.get()
forKeyPath:@"canPlayFastReverse" options:NSKeyValueObservingOptionNew
context:(void *)MediaPlayerAVFoundationObservationContextPlayer];
> +    [m_avPlayer.get() addObserver:m_objcObserver.get()
forKeyPath:@"canPlayFastForward" options:NSKeyValueObservingOptionNew
context:(void *)MediaPlayerAVFoundationObservationContextPlayer];

These new observers must be removed in
MediaPlayerPrivateAVFoundationObjC::cancelLoad.