Bug 144332 – [Mac] Simplify code to support media engines which do not support target playback

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Simplify the code added in r182240 to support media engines that do not support a playback target.

[Radar WebKit Bug Importer]

<rdar://problem/20727457>

[Eric Carlson]

Created attachment 251868 [details]
Proposed patch

[Eric Carlson]

Created attachment 251873 [details]
Rebased patch.

[WebKit Commit Bot]

Attachment 251873 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaSession.cpp:207:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 251873 [details]
Rebased patch.

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

r=me, with nits:

> Source/WebCore/html/HTMLMediaElement.cpp:748
> +    if (m_pendingActionFlags & StopPlayingToTarget) {
> +        if (m_player && m_player->isCurrentPlaybackTargetWireless() && !m_player->canPlayToWirelessPlaybackTarget()) {

No need to nest these if statements.

> Source/WebCore/html/HTMLMediaElement.cpp:4899
> +    RefPtr<Event> event = prpEvent;
> +    bool cancelled = HTMLElement::dispatchEvent(event);
> +
> +    if (event->type() != eventNames().webkitcurrentplaybacktargetiswirelesschangedEvent)
> +        return cancelled;
> +    if (!m_player || !m_player->isCurrentPlaybackTargetWireless() || m_player->canPlayToWirelessPlaybackTarget())
> +        return cancelled;
> +
> +    scheduleDelayedAction(StopPlayingToTarget);

Since we have to double check these have to be double-checked anyway inside of pendingActionTimerFired(), could we just re-write this to be?:

if (event->type() == eventNames().webkitcurrentplaybacktargetiswirelesschangedEvent)
    scheduleDelayedAction(StopPlayingToTarget);
return HTMLElement::dispatchEvent(event);

> Source/WebCore/html/HTMLMediaElement.cpp:5905
> -    return potentiallyPlaying() ? requestedPlaybackRate() : 0;
> +    double requestedPlaybackRate = this->requestedPlaybackRate();
> +    bool potentiallyPlaying = this->potentiallyPlaying();
> +    LOG(Media, "HTMLMediaElement::mediaPlayerRequestedPlaybackRate(%p) - potentiallyPlaying = %s, returning %f", this, boolString(potentiallyPlaying), requestedPlaybackRate);
> +    return potentiallyPlaying ? requestedPlaybackRate : 0;

Seems like this logging may potentially be very noisy.

> Source/WebCore/html/HTMLMediaSession.cpp:385
> -    setWirelessVideoPlaybackDisabled(element, m_restrictions & WirelessVideoPlaybackDisabled);
> +    if (m_restrictions & WirelessVideoPlaybackDisabled)
> +        setWirelessVideoPlaybackDisabled(element, true);

Is there some reason we're no longer setting this to false if the restriction has been lifted?

[Eric Carlson]

Created attachment 251888 [details]
Patch for landing

[Eric Carlson]

Committed r183509: https://trac.webkit.org/r183509