Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2426653002:
Adding mojo MediaSessionClient to support media controls (Closed)
|
Created:
4 years, 2 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, haraken, jam, mlamouri (slow - plz ping), qsr+mojo_chromium.org, Srirama, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding mojo MediaSessionClient to support media controls
This CL adds MediaSessionClient mojo interface to allow Blink
MediaSession and content::MediaSession communicate for media control
messages.
BUG=656563
Committed: https://crrev.com/649f9b868f6783ec9de71c123212b908bf3b232e
Cr-Commit-Position: refs/heads/master@{#426924}
Patch Set 1 : Still rough, depends on "blink media controls" CL #
Total comments: 6
Patch Set 2 : addressed Anton's comments #
Total comments: 6
Patch Set 3 : Addressed haraken's comments #
Total comments: 26
Patch Set 4 : addressing more comments #
Total comments: 8
Patch Set 5 : rebased and nits #Messages
Total messages: 37 (19 generated)
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org, mlamouri@chromium.org
https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html (right): https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html:56: runTests(); hm, does this really work? https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom (right): https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:29: DidReceivedAction(string action); Could you change action from string to enum? https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:39: DidSetEventHandlerForAction(string action, bool is_set); Could you change is_set to two methods? Maybe the method should not be about the event handler at all but rather something like: EnableDisableAction(Action action, bool enable) // again, consider having Enable and Disable methods
PTAL https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html (right): https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html:56: runTests(); On 2016/10/17 15:38:43, whywhat wrote: > hm, does this really work? Yes. It does work. Sadly I have to use async_test to do async setup here. Otherwise the test will quit early since no `async_test` is called when the script reaches the end. https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom (right): https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:29: DidReceivedAction(string action); On 2016/10/17 15:38:43, whywhat wrote: > Could you change action from string to enum? Done. However it seems mapping values enum (or int) from/to AtomicString is ugly in Blink. Also the tests are bit more unclear since it's using integer as enums. See the new patch. I'd prefer to revert the change and do the mapping in content/ in a later patch, WDYT? https://codereview.chromium.org/2426653002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:39: DidSetEventHandlerForAction(string action, bool is_set); On 2016/10/17 15:38:43, whywhat wrote: > Could you change is_set to two methods? > > Maybe the method should not be about the event handler at all but rather > something like: > > EnableDisableAction(Action action, bool enable) // again, consider having Enable > and Disable methods Done.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:23: Vector<AtomicString>* vec = new Vector<AtomicString>(); Use std::unique_ptr. However, why do you need to create the vector dynamically? It would be nicer to use a switch statement or a static array. https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:49: StringToActionEnumMap* map = new StringToActionEnumMap(); Ditto. https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.h:24: DEFINE_WRAPPERTYPEINFO(); You need to add USING_PRE_FINALIZER and clear m_clientBinding in the pre-finalizer. See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/nfc/NF... (This issue will be fixed soon so that developers don't need to worry about the pre-finalizer.)
zqzhang@chromium.org changed reviewers: + rickyz@chromium.org
PTAL +rickyz to review mojo changes. Addressed haraken's comments. https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:23: Vector<AtomicString>* vec = new Vector<AtomicString>(); On 2016/10/17 22:37:05, haraken wrote: > > Use std::unique_ptr. > > However, why do you need to create the vector dynamically? It would be nicer to > use a switch statement or a static array. > It is actually used in DEFINE_THREAD_SAFE_STATIC_LOCAL (see below). I've merged this function as a lambda into getActionEnumToEventNameVec(). https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:49: StringToActionEnumMap* map = new StringToActionEnumMap(); On 2016/10/17 22:37:05, haraken wrote: > > Ditto. Ditto as the other reply. https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/2426653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.h:24: DEFINE_WRAPPERTYPEINFO(); On 2016/10/17 22:37:05, haraken wrote: > > You need to add USING_PRE_FINALIZER and clear m_clientBinding in the > pre-finalizer. > > See > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/nfc/NF... > > (This issue will be fixed soon so that developers don't need to worry about the > pre-finalizer.) Thanks! Done.
Patchset #3 (id:40001) has been deleted
I see that avayvod@ and haraken@ are reviewing the CL already so I'm not going to do a full pass. Let me know if you want me to. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom (right): https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:20: LAST = SEEK_BACKWARD style: leave one empty line before "LAST" to make it more visible.
mlamouri@chromium.org changed reviewers: - mlamouri@chromium.org
https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:25: Vector<AtomicString>, actionEnumToEventNameVec, []() { I'm asking why we need to create the vector in the first place. Why is the switch statement not enough? switch (eventTypeName) { case EventTypeNames::play; return ...; case ...; } https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:48: StringToActionEnumMap, eventNameToActionEnumMap, []() { Ditto.
https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html (right): https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html:55: m.setClientCallback(t.step_func_done(_ => { it seems like you could just use t.step_func() here and pass t to runTests... or consider writing a separate test for each case: function runWithMediaSessionMock(test, callback) { mediaSessionServiceMock.then(m => { m.setClientCallback(test.step_func(_ => { callback(m); })); window.navigator.mediaSession.metadata = null; }); } async_test(function(t) { runWithMediaSessionMock(t, function(m) { window.navigator.mediaSession.onplay = t.step_func_done(); mock.getClient().didReceiveAction(0); }); }, "test that onplay handler is notified correctly"); ... https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-reset-handler-notifies-service.html (right): https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-reset-handler-notifies-service.html:11: [ 0, true ], nit: you could define constants for 0-6 to make it easier to read what actions are where. could be in a shared js file for the other test files. https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-reset-handler-notifies-service.html:59: window.navigator.mediaSession.onresize = _ => {}; maybe set them to null instead? https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html (right): https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html:44: }, "test that setting event handler notifies the mojo service"); what's the service of this test if the one above covers the same cases? https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-unset-handler-notifies-service.html (right): https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-unset-handler-notifies-service.html:62: }, "test that setting event handler notifies the mojo service"); I think you should only leave this one. https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:116: m_service->SetClient(m_clientBinding.CreateInterfacePtrAndBind()); hm, at this point, if document->frame() is null or interfaceProvider is null, you will call SetClient on an uninitialized m_service. perhaps you'd better rewrite it early return style? if (m_service) return m_service.get(); Document* document = ... if (!document->frame()) return nullptr; interfaceProvider = ... if (!interfaceProvider) return nullptr; m_service->SetClient(...); return m_service.get(); https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:126: EventTarget::addEventListenerInternal(eventType, listener, options); nit: do you have to do call the base class method before sending a message to the service? https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:143: bool result = nit: ditto https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:149: if (mojom::blink::MediaSessionService* service = nit: what if the service returned is nullptr? https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:160: const auto& vec = getActionEnumToEventNameVec(); do the opposite switch statement function (like mojomActionToEventName) here: switch (action) { case ...: return eventName::; ... } https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom (right): https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:41: DidReceivedAction(MediaSessionAction action); nit:s/Received/Receive https://chromiumcodereview.appspot.com/2426653002/diff/60001/third_party/WebK... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:45: // MediaSessionClient interface is to used to notify Blink MediaSession of nit: s/to used/used/
PTAL Addressed comments from avayvod@ and haraken@. * Using switch to translate between mojo action names and EventTypeNames. * Fixed tests https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html (right): https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html:55: m.setClientCallback(t.step_func_done(_ => { On 2016/10/18 21:02:57, whywhat wrote: > it seems like you could just use t.step_func() here and pass t to runTests... > or consider writing a separate test for each case: > > function runWithMediaSessionMock(test, callback) { > mediaSessionServiceMock.then(m => { > m.setClientCallback(test.step_func(_ => { callback(m); })); > window.navigator.mediaSession.metadata = null; > }); > } > > async_test(function(t) { > runWithMediaSessionMock(t, function(m) { > window.navigator.mediaSession.onplay = t.step_func_done(); > mock.getClient().didReceiveAction(0); > }); > }, "test that onplay handler is notified correctly"); > > ... I'm doing similar as the "set-handler-notifies-service" tests, which uses a test sequence. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-reset-handler-notifies-service.html (right): https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-reset-handler-notifies-service.html:11: [ 0, true ], On 2016/10/18 21:02:57, whywhat wrote: > nit: you could define constants for 0-6 to make it easier to read what actions > are where. could be in a shared js file for the other test files. Done. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html (right): https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html:44: }, "test that setting event handler notifies the mojo service"); On 2016/10/18 21:02:57, whywhat wrote: > what's the service of this test if the one above covers the same cases? Merged all the three into one. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-unset-handler-notifies-service.html (right): https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-unset-handler-notifies-service.html:62: }, "test that setting event handler notifies the mojo service"); On 2016/10/18 21:02:57, whywhat wrote: > I think you should only leave this one. Merged all three into one. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:25: Vector<AtomicString>, actionEnumToEventNameVec, []() { On 2016/10/18 19:42:37, haraken wrote: > > I'm asking why we need to create the vector in the first place. > > Why is the switch statement not enough? > > switch (eventTypeName) { > case EventTypeNames::play; > return ...; > case ...; > } OK, Done. Using switch is much cleaner. The cost is not super expensive :) https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:48: StringToActionEnumMap, eventNameToActionEnumMap, []() { On 2016/10/18 19:42:37, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:116: m_service->SetClient(m_clientBinding.CreateInterfacePtrAndBind()); On 2016/10/18 21:02:57, whywhat wrote: > hm, at this point, if document->frame() is null or interfaceProvider is null, > you will call SetClient on an uninitialized m_service. > perhaps you'd better rewrite it early return style? > > if (m_service) return m_service.get(); > > Document* document = ... > if (!document->frame()) return nullptr; > > interfaceProvider = ... > if (!interfaceProvider) return nullptr; > > m_service->SetClient(...); > > return m_service.get(); Done. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:126: EventTarget::addEventListenerInternal(eventType, listener, options); On 2016/10/18 21:02:57, whywhat wrote: > nit: do you have to do call the base class method before sending a message to > the service? Ahh, I was afraid of threading issue. Seems not a problem in our case. Done. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:143: bool result = On 2016/10/18 21:02:57, whywhat wrote: > nit: ditto Done. https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:149: if (mojom::blink::MediaSessionService* service = On 2016/10/18 21:02:57, whywhat wrote: > nit: what if the service returned is nullptr? Then it won't reach the `then` branch :) https://codereview.chromium.org/2426653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:160: const auto& vec = getActionEnumToEventNameVec(); On 2016/10/18 21:02:57, whywhat wrote: > do the opposite switch statement function (like mojomActionToEventName) here: > > switch (action) { > case ...: > return eventName::; > > ... > > } Done.
WebKit LGTM https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:45: return emptyString; Return WTF::emptyString() ?
lgtm https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html (right): https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html:32: window.navigator.mediaSession.onplay = t.step_func(checkExpectation.bind(null, t)); the order of parameters seems confusing: t should be the first argument to checkExpectation, right? is null reserved for this? https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:65: return MediaSessionAction::PLAY; I believe you need to return something different and check the return value (maybe use Optional?). One can add an event handler with any valid event name to any object AFAIK, we don't want this to result in enabling Play. https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom (right): https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:21: LAST = SEEK_BACKWARD This doesn't seem to be used. Do we really need it?
Hi, when doing security reviews mojom changes, we prefer to also see the browser-side implementation of the interface - without that, it's not easy to understand what extra functionality is exposed to renderers.
On 2016/10/20 05:56:49, rickyz wrote: > Hi, when doing security reviews mojom changes, we prefer to also see the > browser-side implementation of the interface - without that, it's not easy to > understand what extra functionality is exposed to renderers. Oh, yes. The browser-side changes are in media_session_service_impl.*, and the renderer-side changes are in MediaSession.* Basically, we are implementing a web API that allows web pages respond to media keys. The implementation on the browser side is still work in progress and will be done in follow-up CLs. I can explain a bit here. Hope it can help you review. A pages registers event handlers, and then blink tells browser that an action is enabled via mojom::MediaSessionService (implemented by content::MediaSessionServiceImpl). Then we show media buttons in media notification and register callbacks to the platform (e.g. listen to headset play/pause button). When the user press a button, the signal is passed to blink via mojom::MediaSessionClient (implemented by blink::MediaSession), and then we fire an event to tell the page a button has been clicked.
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:140001) has been deleted
Friendly ping to rickyz@ :) We have other works blocked by this CL. I had some explanation about the purpose of this CL in previous replies, and added some comments on the mojom file https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html (right): https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-action-reaches-client.html:32: window.navigator.mediaSession.onplay = t.step_func(checkExpectation.bind(null, t)); On 2016/10/19 21:59:00, whywhat wrote: > the order of parameters seems confusing: t should be the first argument to > checkExpectation, right? is null reserved for this? Yes, "null" is for this https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:45: return emptyString; On 2016/10/19 12:58:05, haraken wrote: > > Return WTF::emptyString() ? Actually WTF::emptyAtom :) https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:65: return MediaSessionAction::PLAY; On 2016/10/19 21:59:00, whywhat wrote: > I believe you need to return something different and check the return value > (maybe use Optional?). > One can add an event handler with any valid event name to any object AFAIK, we > don't want this to result in enabling Play. Done. https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom (right): https://codereview.chromium.org/2426653002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:21: LAST = SEEK_BACKWARD On 2016/10/19 21:59:00, whywhat wrote: > This doesn't seem to be used. Do we really need it? Thought it was a pattern, seems not :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the explanation - lgtm, but please also include a security reviewer on the CLs which implement the browser-side pieces of the IPCs, thanks!
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2426653002/#ps160001 (title: "rebased and nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Adding mojo MediaSessionClient to support media controls This CL adds MediaSessionClient mojo interface to allow Blink MediaSession and content::MediaSession communicate for media control messages. BUG=656563 ========== to ========== Adding mojo MediaSessionClient to support media controls This CL adds MediaSessionClient mojo interface to allow Blink MediaSession and content::MediaSession communicate for media control messages. BUG=656563 Committed: https://crrev.com/649f9b868f6783ec9de71c123212b908bf3b232e Cr-Commit-Position: refs/heads/master@{#426924} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/649f9b868f6783ec9de71c123212b908bf3b232e Cr-Commit-Position: refs/heads/master@{#426924} |
