Summary: | Media Session: let UI clients query whether a media element is paused | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Rajca <mrajca> | ||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | conrad_shultz, eric.carlson, mrajca, webkit-bug-importer | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 145411 | ||||||||||
Attachments: |
|
Description
Matt Rajca
2015-08-03 13:46:49 PDT
Created attachment 258107 [details]
Patch
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? Created attachment 258134 [details]
Patch
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. Created attachment 258136 [details]
Patch
(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 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! (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. :) Committed r187790: <http://trac.webkit.org/changeset/187790> 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.
(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. |