Bug 147588 – Media Session: let UI clients query whether a media element is paused

• Component: Media
• Status: RESOLVED FIXED

[Matt Rajca]

As part of implementing support for keeping track of the focused 'Content' media element, we need to be able to query whether a given element is paused.

[Matt Rajca]

Created attachment 258107 [details]
Patch

[Tim Horton]

Comment on attachment 258107 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3895
> +    m_process->sendSync(Messages::WebPage::IsMediaElementPaused(elementID), Messages::WebPage::IsMediaElementPaused::Reply(paused), m_pageID);

Let's try to come up with a plan that doesn't involve sync IPC. Can you have a callback that is called once the reply comes in?

[Matt Rajca]

Created attachment 258134 [details]
Patch

[Tim Horton]

Comment on attachment 258134 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4072
> +    send(Messages::WebPageProxy::DidGetIsMediaElementPaused(callbackID, paused));

You could/should use the GenericCallback (specifically UnsignedCallback) mechanism here. Look around for examples. And probably your API (which oddly, I don't see here) should let people provide a block/function to call back (look at WKPageForceRepaint for an example, though that one doesn't return a value).

> Source/WebKit2/WebProcess/WebPage/WebPage.h:753
> +    void getIsMediaElementPaused(uint64_t, uint64_t);

No "get" here; we usually reserve that for functions with out arguments.

[Matt Rajca]

Created attachment 258136 [details]
Patch

[Matt Rajca]

(In reply to comment #4)
> Comment on attachment 258134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258134&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4072
> > +    send(Messages::WebPageProxy::DidGetIsMediaElementPaused(callbackID, paused));
> 
> You could/should use the GenericCallback (specifically UnsignedCallback)
> mechanism here. Look around for examples. And probably your API (which
> oddly, I don't see here) should let people provide a block/function to call
> back (look at WKPageForceRepaint for an example, though that one doesn't
> return a value).

I do use GenericCallback. I didn't realize I could just use UnsignedCallback instead of writing custom callbacks. Sending out a new revision.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:753
> > +    void getIsMediaElementPaused(uint64_t, uint64_t);
> 
> No "get" here; we usually reserve that for functions with out arguments.

Renamed.

[Tim Horton]

Comment on attachment 258136 [details]
Patch

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

> Source/WebCore/page/Page.cpp:1233
> +    return true;

This might deserve some sort of assert? Is there any reasonable way to get here? Is true-by-default right?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3889
> +void WebPageProxy::isMediaElementPaused(uint64_t elementID, uint64_t callbackID)

This patch is maybe a smaller than expected slice of the real patch, because nothing calls this, but OK!

[Matt Rajca]

(In reply to comment #7)
> Comment on attachment 258136 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258136&action=review
> 
> > Source/WebCore/page/Page.cpp:1233
> > +    return true;
> 
> This might deserve some sort of assert? Is there any reasonable way to get
> here? Is true-by-default right?

Yup, I added a ASSERT_NOT_REACHED.

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:3889
> > +void WebPageProxy::isMediaElementPaused(uint64_t elementID, uint64_t callbackID)
> 
> This patch is maybe a smaller than expected slice of the real patch, because
> nothing calls this, but OK!

That's coming soon. :)

[Matt Rajca]

Committed r187790: <http://trac.webkit.org/changeset/187790>

[Eric Carlson]

Comment on attachment 258136 [details]
Patch

If you push media element state changes from the web process when they happen, the UI process won't have to query at all. We already do this for AirPlay and we *need* to consolidate that code with this, so I would prefer if you use that approach here as well.

[Matt Rajca]

(In reply to comment #10)
> Comment on attachment 258136 [details]
> Patch
> 
> If you push media element state changes from the web process when they
> happen, the UI process won't have to query at all. We already do this for
> AirPlay and we *need* to consolidate that code with this, so I would prefer
> if you use that approach here as well.

I'll send out a separate patch that pushes the paused state rather than polls.