[Webkit-unassigned] [Bug 147633] Media Session: push paused state to the media session focus manager instead of polling

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 258277
  --> https://bugs.webkit.org/attachment.cgi?id=258277
Patch

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

This looks good modulo my minor comments, but I am now a WK2 reviewer so someone else will have to give the official r+.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:344
> +#if ENABLE(MEDIA_SESSION)
> +    document()->updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +    document()->updateIsPlayingMedia(0);
> +#endif

Nit: if you don't guard HTMLMediaElementUnknownID with "ENABLE(MEDIA_SESSION)" you won't need the #if here.

Also, the name "HTMLMediaElementUnknownID" is not quite correct because it isn't an HTMLMediaElement at all. Maybe HTMLMediaElementInvalidID?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:1085
> +#if ENABLE(MEDIA_SESSION)
> +            strongThis->document()->updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +            strongThis->document()->updateIsPlayingMedia(0);
> +#endif

Ditto.

> Source/WebCore/dom/Document.cpp:3505
> +#if ENABLE(MEDIA_SESSION)
> +    updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +    updateIsPlayingMedia(0);
> +#endif

Ditto.

> Source/WebCore/dom/Document.cpp:3516
> +#if ENABLE(MEDIA_SESSION)
> +    updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +    updateIsPlayingMedia(0);
> +#endif

Ditto.

> Source/WebCore/dom/Document.cpp:3529
> +    if (HTMLMediaElement* sourceElement = HTMLMediaElement::elementWithID(sourceElementID)) {
> +        if (sourceElement->isPlaying())
> +            state |= MediaProducer::IsSourcePlaying;
> +    }

Nit: you can prevent the failed lookup by checking for HTMLMediaElementUnknownID (or HTMLMediaElementInvalidID, or whatever you use).

> Source/WebCore/page/MediaProducer.h:43
> +#if ENABLE(MEDIA_SESSION)
> +        IsSourcePlaying = 1 << 6,
> +#endif

I don't think you need to #if this flag.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5947
> +    if (focusManager->focusedMediaElementPage() == this && focusManager->focusedMediaElementID() == sourceElementID)
> +        focusManager->setFocusedMediaElementIsPlaying(state & MediaProducer::IsSourcePlaying);

This seems like the wrong place for this logic. Why not pass the element ID into the focus manager so you can keep all of the focus logic there.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150805/0a6d4536/attachment-0001.html>