[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 #4 from Matt Rajca <mrajca at apple.com> ---
(In reply to comment #3)
> Comment on attachment 258277 [details]
> 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.

Removed it to simplify subsequent code (1).

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

Renamed.

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

Simplified this after fixing (1).

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

Simplified this after fixing (1).

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

Simplified this after fixing (1).

> 
> > 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).

I prefer just checking if the pointer is null (even if it's logically equivalent to checking for an invalid ID). Feels safer.

> 
> > Source/WebCore/page/MediaProducer.h:43
> > +#if ENABLE(MEDIA_SESSION)
> > +        IsSourcePlaying = 1 << 6,
> > +#endif
> 
> I don't think you need to #if this flag.

Removed.

> 
> > 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.

Refactored.

-- 
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/73d37570/attachment.html>