Bug 147588 - Media Session: let UI clients query whether a media element is paused
Summary: Media Session: let UI clients query whether a media element is paused
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 145411
  Show dependency treegraph
 
Reported: 2015-08-03 13:46 PDT by Matt Rajca
Modified: 2015-08-04 10:10 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.92 KB, patch)
2015-08-03 13:52 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (7.31 KB, patch)
2015-08-03 16:39 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (5.96 KB, patch)
2015-08-03 17:18 PDT, Matt Rajca
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Rajca 2015-08-03 13:46:49 PDT
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.
Comment 1 Matt Rajca 2015-08-03 13:52:45 PDT
Created attachment 258107 [details]
Patch
Comment 2 Tim Horton 2015-08-03 14:03:05 PDT
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?
Comment 3 Matt Rajca 2015-08-03 16:39:47 PDT
Created attachment 258134 [details]
Patch
Comment 4 Tim Horton 2015-08-03 16:46:48 PDT
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.
Comment 5 Matt Rajca 2015-08-03 17:18:11 PDT
Created attachment 258136 [details]
Patch
Comment 6 Matt Rajca 2015-08-03 17:19:56 PDT
(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.
Comment 7 Tim Horton 2015-08-03 17:22:51 PDT
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!
Comment 8 Matt Rajca 2015-08-03 17:28:06 PDT
(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. :)
Comment 9 Matt Rajca 2015-08-03 17:31:15 PDT
Committed r187790: <http://trac.webkit.org/changeset/187790>
Comment 10 Eric Carlson 2015-08-04 09:13:12 PDT
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.
Comment 11 Matt Rajca 2015-08-04 10:10:59 PDT
(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.