[Webkit-unassigned] [Bug 129048] Setting playback rate on video with media controller is not ignored

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=129048





--- Comment #6 from Eric Carlson <eric.carlson at apple.com>  2014-02-19 10:55:39 PST ---
(From update of attachment 224650)
View in context: https://bugs.webkit.org/attachment.cgi?id=224650&action=review

> LayoutTests/media/video-controller-child-rate.html:32
> +                setTimeout(halfSecLater,500);

Nit: space after the comma.

> LayoutTests/media/video-controller-child-rate.html:38
> +            function halfSecLater()
> +            {
> +                if (video.currentTime > currentTime + 1)
> +                        failTest("Playback rate was aplied directly to video with media controller.");

This is asking for occasional flakiness, and .5 seconds is a long time to wait for any one test. Instead, why don't you have a 'time update' event handler that triggers a small number of times (5-10?) which compares the .currentTime delta with the wall clock delta.

> LayoutTests/media/video-controller-child-rate.html:39
> +                    endTest();

Nit: odd indentation here.

> Source/WebCore/html/HTMLMediaElement.cpp:2590
> -    double effectiveRate = m_mediaController ? m_mediaController->playbackRate() : m_playbackRate;
> -    if (m_player && potentiallyPlaying() && m_player->rate() != effectiveRate)
> -        m_player->setRate(effectiveRate);
> +    if (m_player && potentiallyPlaying() && m_player->rate() != playbackRate())
> +        m_player->setRate(playbackRate());

This doesn't actually change anything, and it adds two method calls. Why change it?

> Source/WebCore/html/HTMLMediaElement.cpp:3884
> -        if (loop() && !m_mediaController && m_playbackRate > 0) {
> +        if (loop() && !m_mediaController && playbackRate() > 0) {
>              m_sentEndEvent = false;
>              // then seek to the earliest possible position of the media resource and abort these steps when the direction of
>              // playback is forwards,
>              if (now >= dur)
>                  seek(0);
> -        } else if ((now <= 0 && m_playbackRate < 0) || (now >= dur && m_playbackRate > 0)) {
> +        } else if ((now <= 0 && playbackRate() < 0) || (now >= dur && playbackRate() > 0)) {

It is probably worth caching playbackRate() in a local instead of calling it (potentially) three times.

> Source/WebCore/html/HTMLMediaElement.cpp:4181
> -    if (m_playbackRate > 0)
> +    if (playbackRate() > 0)
>          return dur > 0 && now >= dur && (!loop() || m_mediaController);
>  
>      // or the current playback position is the earliest possible position and the direction 
>      // of playback is backwards
> -    if (m_playbackRate < 0)
> +    if (playbackRate() < 0)

Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.