Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1370453002:
Introduce WebMediaSession (Closed)
|
Created:
5 years, 3 months ago by davve Modified:
5 years, 2 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce WebMediaSession
WebMediaSession is the API enabling web exposed MediaSession objects
to control platform implementations of media session related
functionality.
Implementation will begin with Android and other platforms will be
added later.
BUG=497735
Committed: https://crrev.com/9b97db48d95635e9c5d48676df8dc51c9e5eef2a
Cr-Commit-Position: refs/heads/master@{#355058}
Patch Set 1 #Patch Set 2 : Remove some stale includes #Patch Set 3 : Expect no platform implementation on !ANDROID #
Total comments: 40
Patch Set 4 : Address review comments #
Total comments: 2
Patch Set 5 : Rebased and MediaSession changes in case of no impl #Patch Set 6 : Revert throwing in the constructor; would needlessly complicate existing tests #
Total comments: 36
Patch Set 7 : Address review comments #
Total comments: 18
Patch Set 8 : Adress further more review comments #Patch Set 9 : Re-upload #
Total comments: 2
Patch Set 10 : Address resolver is unused #
Total comments: 3
Patch Set 11 : Anchor scoped_ptr properly #Patch Set 12 : Moar scope #
Total comments: 1
Patch Set 13 : Add virtual default destructor #Messages
Total messages: 79 (26 generated)
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
davve@opera.com changed reviewers: + avayvod@chromium.org, mlamouri@chromium.org, philipj@opera.com
Attempt at getting the ball rolling (again). Some code is lifted from https://codereview.chromium.org/1259643002/ and I've address Antons comments about an explicit constructor. There is still missing documentation for MediaSession that I plan to do at some point. WebMediaSession is not moved to the Oilpan heap, the lifespans are linked so keeping it as an OwnPtr seems fine to me. There is the TypeError controversy too. This patch now uses TypeError and has to add a scope to CallbackPromiseAdapter to make it work. We might want to reconsider this. Oh well, something for you to look at when you have the chance. I'll be on vacation next week so no hurry.
Thanks for doing this! :) Please see comments below. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.cc:22: NOTREACHED(); Could you resolve the callback and add a NOT_IMPLEMENTED() call here? NOTREACHED() will crash and the callback will never be called. It seems unnecessary. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.cc:30: NOTREACHED(); ditto https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:13: class RendererMediaSessionManager; nit: not needed. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:21: virtual void deactivate(blink::WebMediaSessionCommandCallback*); = override; for both. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:24: base::ThreadChecker main_thread_checker_; For a feature that is only available from a Window context, this seems a bit too much on the careful side. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1370453002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:5065: WebMediaSession* RenderFrameImpl::CreateAndroidWebMediaSession() { Do we need this level of indirection? https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:27: MediaSession* MediaSession::create(ScriptState* scriptState) The ExecutionContext should be enough. It will give you the Document from which you should be able to get all you need. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:43: ScriptPromise promise = resolver->promise(); Do you need to create the |promise| here? You could probably |return resolver->promise();| at the end, couldn't you? https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:45: #if OS(ANDROID) Could we avoid the ifdef and let that happen at a higher level? Or maybe fail if m_webMediaSession is nullptr? https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:50: resolver->reject(v8::Exception::Error(v8String(isolate, "Missing platform implementation."))); Could you fail with a NotSupportedError? https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:59: ScriptPromise promise = resolver->promise(); ditto https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:61: #if OS(ANDROID) ditto https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:66: resolver->reject(v8::Exception::Error(v8String(isolate, "Missing platform implementation."))); ditto https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.idl:9: ConstructorCallWith=ScriptState, s/ScriptState/ExecutionContext/? https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:18: return v8::Exception::TypeError(v8String(isolate, "The media session activation failed.")); TypeError... Let's see how the discussion on the spec goes, worse case, we can change that afterward. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionError.h:14: class DOMException; nit: doesn't seem needed. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:45: TEST_F(MediaSessionTest, Activate) Can you follow the pattern we usually do in Chromium: #if OS(ANDROID) #define MAYBE_Activate Activate #define MAYBE_Deactivate Deactivate #else #define MAYBE_Activate DISABLED_Activate #define MAYBE_Deactivate DISABLED_Deactivate #endif TEST_F(MediaSessionTest, MAYBE_Activate) { ... } TEST_F(MediaSessionTest, MAYBE_Deactivate) { ... } https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:52: EXPECT_CALL(*mockWebMediaSession, activate(_)); nit: leave empty line before, remove the one after. Arguably these tests don't tests much butI guess they could be used for better things later. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:65: EXPECT_CALL(*mockWebMediaSession, deactivate(_)); nit: leave empty line before. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:17: virtual ~WebMediaSession() {} {} => "= default;"
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/60001
Thanks for the detailed review, Mounir! https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.cc:22: NOTREACHED(); On 2015/09/28 15:16:02, Mounir Lamouri wrote: > Could you resolve the callback and add a NOT_IMPLEMENTED() call here? > NOTREACHED() will crash and the callback will never be called. It seems > unnecessary. Done. I'll let the activate promise fail. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.cc:30: NOTREACHED(); On 2015/09/28 15:16:02, Mounir Lamouri wrote: > ditto Done. Since activate can't succeed I'll make deactivate instantly succeed. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:13: class RendererMediaSessionManager; On 2015/09/28 15:16:02, Mounir Lamouri wrote: > nit: not needed. Done. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:21: virtual void deactivate(blink::WebMediaSessionCommandCallback*); On 2015/09/28 15:16:02, Mounir Lamouri wrote: > = override; for both. Done. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:24: base::ThreadChecker main_thread_checker_; On 2015/09/28 15:16:02, Mounir Lamouri wrote: > For a feature that is only available from a Window context, this seems a bit too > much on the careful side. Removed. https://codereview.chromium.org/1370453002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1370453002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:5065: WebMediaSession* RenderFrameImpl::CreateAndroidWebMediaSession() { On 2015/09/28 15:16:03, Mounir Lamouri wrote: > Do we need this level of indirection? Probably not, no. If we ever get CreateAndroidWebMediaPlayer level complexity (which I doubt) we could add it back later. Removed. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:27: MediaSession* MediaSession::create(ScriptState* scriptState) On 2015/09/28 15:16:03, Mounir Lamouri wrote: > The ExecutionContext should be enough. It will give you the Document from which > you should be able to get all you need. Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:43: ScriptPromise promise = resolver->promise(); On 2015/09/28 15:16:03, Mounir Lamouri wrote: > Do you need to create the |promise| here? You could probably |return > resolver->promise();| at the end, couldn't you? Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:45: #if OS(ANDROID) On 2015/09/28 15:16:03, Mounir Lamouri wrote: > Could we avoid the ifdef and let that happen at a higher level? Or maybe fail if > m_webMediaSession is nullptr? Using m_webMediaSession != NULL wfm. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:50: resolver->reject(v8::Exception::Error(v8String(isolate, "Missing platform implementation."))); On 2015/09/28 15:16:03, Mounir Lamouri wrote: > Could you fail with a NotSupportedError? Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:59: ScriptPromise promise = resolver->promise(); On 2015/09/28 15:16:03, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:61: #if OS(ANDROID) On 2015/09/28 15:16:03, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:66: resolver->reject(v8::Exception::Error(v8String(isolate, "Missing platform implementation."))); On 2015/09/28 15:16:03, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.idl:9: ConstructorCallWith=ScriptState, On 2015/09/28 15:16:03, Mounir Lamouri wrote: > s/ScriptState/ExecutionContext/? Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:18: return v8::Exception::TypeError(v8String(isolate, "The media session activation failed.")); On 2015/09/28 15:16:03, Mounir Lamouri wrote: > TypeError... Let's see how the discussion on the spec goes, worse case, we can > change that afterward. Yes. I'll leave it be for now. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionError.h:14: class DOMException; On 2015/09/28 15:16:03, Mounir Lamouri wrote: > nit: doesn't seem needed. Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:45: TEST_F(MediaSessionTest, Activate) On 2015/09/28 15:16:03, Mounir Lamouri wrote: > Can you follow the pattern we usually do in Chromium: > > #if OS(ANDROID) > #define MAYBE_Activate Activate > #define MAYBE_Deactivate Deactivate > #else > #define MAYBE_Activate DISABLED_Activate > #define MAYBE_Deactivate DISABLED_Deactivate > #endif > > TEST_F(MediaSessionTest, MAYBE_Activate) > { > ... > } > > TEST_F(MediaSessionTest, MAYBE_Deactivate) > > { > ... > } Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:52: EXPECT_CALL(*mockWebMediaSession, activate(_)); On 2015/09/28 15:16:03, Mounir Lamouri wrote: > nit: leave empty line before, remove the one after. > > Arguably these tests don't tests much butI guess they could be used for better > things later. Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:65: EXPECT_CALL(*mockWebMediaSession, deactivate(_)); On 2015/09/28 15:16:03, Mounir Lamouri wrote: > nit: leave empty line before. Done. https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:17: virtual ~WebMediaSession() {} On 2015/09/28 15:16:03, Mounir Lamouri wrote: > {} => "= default;" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1370453002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:35: return new MediaSession(client->createWebMediaSession()); Philip suggested that we might throw an exception here already in case the platform has no implementation. That way we can assume a m_webMediaSession in activate()/deactivate().
https://codereview.chromium.org/1370453002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:50: return resolver->promise(); Creating the promise this late doesn't work well with synchronously resolving the promise in the stub implementation. I'll move it back for now.
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1370453002/diff/100001/content/renderer/media... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1370453002/diff/100001/content/renderer/media... content/renderer/media/android/webmediasession_android.h:8: #include "base/threading/thread_checker.h" nit: I don't think you need that #include anymore. https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:5122: nit: remove that change. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:50: resolver->reject(DOMException::create(NotSupportedError, "Missing platform implementation.")); I would argue that rejecting with DOMException and TypeError is a bit odd but I guess this DOMException isn't meant to stay forever. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:9: #include "bindings/core/v8/ScriptPromiseResolver.h" I don't think you need the Resolver header in here. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:13: #include "public/platform/modules/mediasession/WebMediaSession.h" Can you fwd declare WebMediaSession? (Not sure what would happen with PassOwnPtr) https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:27: static MediaSession* createForTesting(PassOwnPtr<WebMediaSession>); nit: I would prefer to have the test marked as a friend in order to be able to call the private ctor instead of adding a "createForTesting" method but it might be a matter of test so I will leave it up to you. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:18: return v8::Exception::TypeError(v8String(isolate, "The media session activation failed.")); I will not block this CL for that but I'm still against using TypeError. Proof that it's not even used much is that you had to fix a bug in CallbackPromiseAdapter to allow it to work in Blink. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.h:9: #include "public/platform/modules/mediasession/WebMediaSessionError.h" You might be able to forward declare this one. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/OWNERS (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/OWNERS:4: philipj@opera.com nit: you can do: file://third_party/WebKit/Source/modules/mediasession/OWNERS so it will use that other OWNERS file, it will prevent having to change multiple OWNERS file if we want to modify ownership. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; Why are we using a custom callback for deactivate() while we do not expect an error? Should we have two type of callbacks: - WebMediaSessionActivateCallback which will be <void, const WebMediaSessionError&> - WebMediaSessionDeactivateCallback which will be <void, void> https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h:11: enum ErrorType { I think it would be beneficial to move this outside of the struct and make it a "class enum". Two benefits are that you will then be able to do: blink::WebMediaSessionErrorType::Activate; instead of: blink::WebMediaSessionError::ErrorTypeActivate; Also, that will allow you to forward declare the enum (and the struct) instead of including the header when you only need the enum.
https://codereview.chromium.org/1370453002/diff/100001/content/renderer/media... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1370453002/diff/100001/content/renderer/media... content/renderer/media/android/webmediasession_android.h:8: #include "base/threading/thread_checker.h" No longer needed? https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2133: blink::WebLocalFrame* frame) { I suppose remove this argument and add it back if needed later. https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:5122: Stray whitespace. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:35: return new MediaSession(client->createWebMediaSession()); If we throw an exception when client->createWebMediaSession() returns nullptr, so that there can be no MediaSession instance without a backend. Now we only have two places where this is handled, but there may be more. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:17: if (webError.errorType == WebMediaSessionError::ErrorTypeActivate) Maybe just assert this so that there's just one error below? https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:44: #if OS(ANDROID) This ought not be necessary in this test, we have a MockWebMediaSession and the platform-specific implementations should never be created here.
https://codereview.chromium.org/1370453002/diff/100001/content/renderer/media... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1370453002/diff/100001/content/renderer/media... content/renderer/media/android/webmediasession_android.h:8: #include "base/threading/thread_checker.h" On 2015/10/07 14:19:30, Mounir Lamouri wrote: > nit: I don't think you need that #include anymore. Done. https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2133: blink::WebLocalFrame* frame) { On 2015/10/07 14:37:12, philipj wrote: > I suppose remove this argument and add it back if needed later. Done. https://codereview.chromium.org/1370453002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:5122: On 2015/10/07 14:19:30, Mounir Lamouri wrote: > nit: remove that change. Done. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:35: return new MediaSession(client->createWebMediaSession()); On 2015/10/07 14:37:12, philipj wrote: > If we throw an exception when client->createWebMediaSession() returns nullptr, > so that there can be no MediaSession instance without a backend. Now we only > have two places where this is handled, but there may be more. Done. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:50: resolver->reject(DOMException::create(NotSupportedError, "Missing platform implementation.")); On 2015/10/07 14:19:30, Mounir Lamouri wrote: > I would argue that rejecting with DOMException and TypeError is a bit odd but I > guess this DOMException isn't meant to stay forever. By moving the exception throwing to the constructor we at least doesn't mix and match in the same promise. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:9: #include "bindings/core/v8/ScriptPromiseResolver.h" On 2015/10/07 14:19:31, Mounir Lamouri wrote: > I don't think you need the Resolver header in here. Done. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:13: #include "public/platform/modules/mediasession/WebMediaSession.h" On 2015/10/07 14:19:31, Mounir Lamouri wrote: > Can you fwd declare WebMediaSession? (Not sure what would happen with > PassOwnPtr) As you suspected, PassOwnPtr seems to not like that: ../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:51:29: error: invalid application of 'sizeof' to incomplete type 'blink::WebMediaSession' static_assert(sizeof(T) > 0, "type must be complete"); ^ ../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:52:9: error: possible problem detected in invocation of delete operator: [-Werror=delete-incomplete] delete ptr; ^ ../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:48:30: error: 'ptr' has incomplete type [-Werror] static void deletePtr(T* ptr) ^ In file included from ../../third_party/WebKit/Source/modules/mediasession/HTMLMediaElementMediaSession.h:10:0, from ../../third_party/WebKit/Source/modules/mediasession/HTMLMediaElementMediaSession.cpp:6: ../../third_party/WebKit/Source/modules/mediasession/MediaSession.h:17:7: error: forward declaration of 'class blink::WebMediaSession' [-Werror] class WebMediaSession; ^ https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:27: static MediaSession* createForTesting(PassOwnPtr<WebMediaSession>); On 2015/10/07 14:19:31, Mounir Lamouri wrote: > nit: I would prefer to have the test marked as a friend in order to be able to > call the private ctor instead of adding a "createForTesting" method but it might > be a matter of test so I will leave it up to you. That would require one to add a FRIEND_TEST_ALL_PREFIXES invocation to the header for each test one adds? I don't have a strong feeling about it, but it feels a bit cumbersome. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:17: if (webError.errorType == WebMediaSessionError::ErrorTypeActivate) On 2015/10/07 14:37:12, philipj wrote: > Maybe just assert this so that there's just one error below? Done. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:18: return v8::Exception::TypeError(v8String(isolate, "The media session activation failed.")); On 2015/10/07 14:19:31, Mounir Lamouri wrote: > I will not block this CL for that but I'm still against using TypeError. Proof > that it's not even used much is that you had to fix a bug in > CallbackPromiseAdapter to allow it to work in Blink. Point well taken. I'll make it a priority to follow up on this later. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.h:9: #include "public/platform/modules/mediasession/WebMediaSessionError.h" On 2015/10/07 14:19:31, Mounir Lamouri wrote: > You might be able to forward declare this one. Done. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:44: #if OS(ANDROID) On 2015/10/07 14:37:12, philipj wrote: > This ought not be necessary in this test, we have a MockWebMediaSession and the > platform-specific implementations should never be created here. Done. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/OWNERS (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/OWNERS:4: philipj@opera.com On 2015/10/07 14:19:31, Mounir Lamouri wrote: > nit: you can do: > file://third_party/WebKit/Source/modules/mediasession/OWNERS > so it will use that other OWNERS file, it will prevent having to change multiple > OWNERS file if we want to modify ownership. Good point. Didn't know that. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; On 2015/10/07 14:19:31, Mounir Lamouri wrote: > Why are we using a custom callback for deactivate() while we do not expect an > error? Should we have two type of callbacks: As the code stands here, there's no probably no good reason. But when building on this, I had a reason for going this way. For each activate call, the plan is to generate an activate number for the IPC. When the activate finishes, there is IPC back with the corresponding activate number so that the render process can resolve the correct promise. We also need the corresponding infrastructure for deactivate(). No, deactivate() can't fail but we still need to resolve the promise when it has completed. If the callback API for resolving activate() and deactivate() is the same, we can use the same map. Something like in WebMediaSessionAndroid: std::map<int, blink::WebMediaSessionCommandCallback*> commands_; for outstanding activate and deactivate calls on the renderer side. If the API is different we need a separate map for activate() and deactivate(). It not super important but made the code a bit simpler later on. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h:11: enum ErrorType { On 2015/10/07 14:19:31, Mounir Lamouri wrote: > I think it would be beneficial to move this outside of the struct and make it a > "class enum". Two benefits are that you will then be able to do: > blink::WebMediaSessionErrorType::Activate; > instead of: > blink::WebMediaSessionError::ErrorTypeActivate; > > Also, that will allow you to forward declare the enum (and the struct) instead > of including the header when you only need the enum. Good point. Seems like I only need the enum really so I'll just ditch the struct. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/android/virtual/mediasession/media/mediasession/mediasession-constructor-expected.txt (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/android/virtual/mediasession/media/mediasession/mediasession-constructor-expected.txt:1: CONSOLE WARNING: line 9: dummy log entry to avoid an empty -expected.txt in virtual/ Currently these are made in the blind. When running the tests on an android device, they got detected as pixel tests (expected .png files). Probably some missing infrastructure there for testharness.js tests (only a guess though).
https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:27: static MediaSession* createForTesting(PassOwnPtr<WebMediaSession>); On 2015/10/08 09:17:35, David Vest wrote: > On 2015/10/07 14:19:31, Mounir Lamouri wrote: > > nit: I would prefer to have the test marked as a friend in order to be able to > > call the private ctor instead of adding a "createForTesting" method but it > might > > be a matter of test so I will leave it up to you. > > That would require one to add a FRIEND_TEST_ALL_PREFIXES invocation to the > header for each test one adds? I don't have a strong feeling about it, but it > feels a bit cumbersome. FWIW, there's quite a few fooForTesting in Blink, so at least this wouldn't be some outlier hack.
My issues have been addressed, LGTM.
Anton had a few comments on https://codereview.chromium.org/1259643002/ that I never addressed.
https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:27: static MediaSession* createForTesting(PassOwnPtr<WebMediaSession>); On 2015/10/08 at 09:34:57, philipj wrote: > On 2015/10/08 09:17:35, David Vest wrote: > > On 2015/10/07 14:19:31, Mounir Lamouri wrote: > > > nit: I would prefer to have the test marked as a friend in order to be able to > > > call the private ctor instead of adding a "createForTesting" method but it > > might > > > be a matter of test so I will leave it up to you. > > > > That would require one to add a FRIEND_TEST_ALL_PREFIXES invocation to the > > header for each test one adds? I don't have a strong feeling about it, but it > > feels a bit cumbersome. > > FWIW, there's quite a few fooForTesting in Blink, so at least this wouldn't be some outlier hack. I replied in the new patchset. There is no need to use FRIEND_TEST_ALL_PREFIXES and even if there are some fooForTest, having code specific for tests isn't a great idea in general. I would prefer to allow test classes to access private method as long as we can. https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; On 2015/10/08 at 09:17:35, David Vest wrote: > On 2015/10/07 14:19:31, Mounir Lamouri wrote: > > Why are we using a custom callback for deactivate() while we do not expect an > > error? Should we have two type of callbacks: > > As the code stands here, there's no probably no good reason. But when > building on this, I had a reason for going this way. For each activate > call, the plan is to generate an activate number for the IPC. > > When the activate finishes, there is IPC back with the corresponding > activate number so that the render process can resolve the correct > promise. > > We also need the corresponding infrastructure for deactivate(). No, > deactivate() can't fail but we still need to resolve the promise when > it has completed. > > If the callback API for resolving activate() and deactivate() is the > same, we can use the same map. Something like in > WebMediaSessionAndroid: > > std::map<int, blink::WebMediaSessionCommandCallback*> commands_; > > for outstanding activate and deactivate calls on the renderer side. If > the API is different we need a separate map for activate() and > deactivate(). > > It not super important but made the code a bit simpler later on. I replied in the new patchset. I don't think we should have oddities because of future plans. The change will be trivial when the requirements you are talking about will apply. I would prefer to do that change when needed instead of having something that doesn't sound useful now. https://codereview.chromium.org/1370453002/diff/120001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1370453002/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:14: callback->onError(blink::WebMediaSessionError::Activate); nit: use scoped_ptr<>? https://codereview.chromium.org/1370453002/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:22: callback->onSuccess(); ditto https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:34: return nullptr; I'm sceptical of that change. It means that if we tell websites to try the feature, on Desktop the call te |var session = new MediaSession()| will throw. That will break their JS code. I will not block the CL on that but please, consider this. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:35: return MediaSession::createForTesting(adoptPtr(webMediaSession)); I would still prefer to have MediaSessionTest a friend of MediaSession if that means we can avoid adding a FooForTest() method. I think you could move MediaSessionTest class outside of the anonymous namespace. Then declare this class as friend of MediaSession in the MediaSession class. You should then be able to call new MediaSession(adoptPtr(webMediaSession)) here. No need to use FRIEND_* macros. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; I still don't think you need WebMediaSessionCommandCallback in deactivate(). https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h:10: enum class WebMediaSessionError { nit: this is seen as a copy-paste but it's not the case. You can fix that with --no-find-copies in git cl upload (if there are really no copies in the CL). Also, FTR, it's interesting how you can easily do what you are doing because the repositories have merged. Otherwise, I would have recommended to keep the struct because you might need to add a |message| later and moving to as struct would have been a nightmare :)
https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:34: return nullptr; On 2015/10/09 13:01:28, Mounir Lamouri wrote: > I'm sceptical of that change. It means that if we tell websites to try the > feature, on Desktop the call te |var session = new MediaSession()| will throw. > That will break their JS code. I will not block the CL on that but please, > consider this. Neither this nor rejecting in activate() and deactivate() as before is something that could actually be shipped, as neither failure mode is per spec. Until we can assert that we have a backend, a single point of failure seems nice.
On 2015/10/09 at 15:05:39, philipj wrote: > https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right): > > https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:34: return nullptr; > On 2015/10/09 13:01:28, Mounir Lamouri wrote: > > I'm sceptical of that change. It means that if we tell websites to try the > > feature, on Desktop the call te |var session = new MediaSession()| will throw. > > That will break their JS code. I will not block the CL on that but please, > > consider this. > > Neither this nor rejecting in activate() and deactivate() as before is something that could actually be shipped, as neither failure mode is per spec. Until we can assert that we have a backend, a single point of failure seems nice. I agree. It's definitely not blocking the CL from landing. I should have made that clearer.
lgtm given Mounir's comments are addressed https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/CallbackPromiseAdapter.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/CallbackPromiseAdapter.h:188: ScriptState::Scope scope(resolver->scriptState()); nit: is this needed? shoulnd't this be part of a separate change? or is it a bad merge/rebase? https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; On 2015/10/09 at 13:01:28, Mounir Lamouri wrote: > I still don't think you need WebMediaSessionCommandCallback in deactivate(). It's needed to resolve the promise, isn't it?
https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; On 2015/10/13 at 15:59:05, whywhat wrote: > On 2015/10/09 at 13:01:28, Mounir Lamouri wrote: > > I still don't think you need WebMediaSessionCommandCallback in deactivate(). > > It's needed to resolve the promise, isn't it? True, but WebMedioSessionCommandCallback is a WebCallbacks<void, const WebMediaSessionError&> and we need a WebCallbacks<void,void> for deactivate().
We've been in Opio in Provence for an engineering seminar, so that's why we've been so silent this week. David's system disk had the bad taste of breaking while we were away, but I'm confident that next week will be the week when we can finally land this :)
Anton, I don't suppose you have an opinion on the DOMException vs. TypeError thing? Depending on how I look at it I can convince myself either way, and I'm eager to be swayed either way so we can move on to more interesting questions :)
On 2015/10/16 at 13:25:08, philipj wrote: > Anton, I don't suppose you have an opinion on the DOMException vs. TypeError thing? Depending on how I look at it I can convince myself either way, and I'm eager to be swayed either way so we can move on to more interesting questions :) We can land this CL and resolve this problem in the spec. I think it is easier to land with DOMException because it doesn't require touching CallbackPromiseAdapter but if you can get approval for that change, this is fine by me.
Philip, let's say I trust Mounir on the DOMException vs TypeError issue :)
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/140001
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/160001
https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; On 2015/10/09 13:01:28, Mounir Lamouri wrote: > On 2015/10/08 at 09:17:35, David Vest wrote: > > On 2015/10/07 14:19:31, Mounir Lamouri wrote: > > > Why are we using a custom callback for deactivate() while we do not expect > an > > > error? Should we have two type of callbacks: > > > > As the code stands here, there's no probably no good reason. But when > > building on this, I had a reason for going this way. For each activate > > call, the plan is to generate an activate number for the IPC. > > > > When the activate finishes, there is IPC back with the corresponding > > activate number so that the render process can resolve the correct > > promise. > > > > We also need the corresponding infrastructure for deactivate(). No, > > deactivate() can't fail but we still need to resolve the promise when > > it has completed. > > > > If the callback API for resolving activate() and deactivate() is the > > same, we can use the same map. Something like in > > WebMediaSessionAndroid: > > > > std::map<int, blink::WebMediaSessionCommandCallback*> commands_; > > > > for outstanding activate and deactivate calls on the renderer side. If > > the API is different we need a separate map for activate() and > > deactivate(). > > > > It not super important but made the code a bit simpler later on. > > I replied in the new patchset. I don't think we should have oddities because of > future plans. The change will be trivial when the requirements you are talking > about will apply. I would prefer to do that change when needed instead of having > something that doesn't sound useful now. Done. https://codereview.chromium.org/1370453002/diff/120001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1370453002/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:14: callback->onError(blink::WebMediaSessionError::Activate); On 2015/10/09 13:01:28, Mounir Lamouri wrote: > nit: use scoped_ptr<>? Done. https://codereview.chromium.org/1370453002/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:22: callback->onSuccess(); On 2015/10/09 13:01:28, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/CallbackPromiseAdapter.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/CallbackPromiseAdapter.h:188: ScriptState::Scope scope(resolver->scriptState()); On 2015/10/13 15:59:04, whywhat wrote: > nit: is this needed? shoulnd't this be part of a separate change? or is it a bad > merge/rebase? Gone now we're heading for DOMException instead. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:19: return v8::Exception::TypeError(v8String(isolate, "The media session activation failed.")); Let's go for DOMException InvalidStateError for now. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionTest.cpp:35: return MediaSession::createForTesting(adoptPtr(webMediaSession)); On 2015/10/09 13:01:28, Mounir Lamouri wrote: > I would still prefer to have MediaSessionTest a friend of MediaSession if that > means we can avoid adding a FooForTest() method. > > I think you could move MediaSessionTest class outside of the anonymous > namespace. Then declare this class as friend of MediaSession in the MediaSession > class. You should then be able to call new > MediaSession(adoptPtr(webMediaSession)) here. No need to use FRIEND_* macros. WFM. Thanks. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:20: virtual void deactivate(WebMediaSessionCommandCallback*) = 0; On 2015/10/14 11:02:43, Mounir Lamouri wrote: > On 2015/10/13 at 15:59:05, whywhat wrote: > > On 2015/10/09 at 13:01:28, Mounir Lamouri wrote: > > > I still don't think you need WebMediaSessionCommandCallback in deactivate(). > > > > It's needed to resolve the promise, isn't it? > > True, but WebMedioSessionCommandCallback is a WebCallbacks<void, const > WebMediaSessionError&> and we need a WebCallbacks<void,void> for deactivate(). WebMediaSessionDeactivateCallback as WebCallbacks<void,void> is now a thing. https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h (right): https://codereview.chromium.org/1370453002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/mediasession/WebMediaSessionError.h:10: enum class WebMediaSessionError { On 2015/10/09 13:01:28, Mounir Lamouri wrote: > nit: this is seen as a copy-paste but it's not the case. You can fix that with > --no-find-copies in git cl upload (if there are really no copies in the CL). > > Also, FTR, it's interesting how you can easily do what you are doing because the > repositories have merged. Otherwise, I would have recommended to keep the struct > because you might need to add a |message| later and moving to as struct would > have been a nightmare :) Acknowledged.
https://codereview.chromium.org/1370453002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:16: DOMException* MediaSessionError::take(ScriptPromiseResolver* resolver, const WebMediaSessionError& webError) resolver argument is unused
https://codereview.chromium.org/1370453002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp (right): https://codereview.chromium.org/1370453002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSessionError.cpp:16: DOMException* MediaSessionError::take(ScriptPromiseResolver* resolver, const WebMediaSessionError& webError) On 2015/10/19 13:23:08, philipj wrote: > resolver argument is unused Done.
I have no further issues. Mounir, you get the final say :) https://codereview.chromium.org/1370453002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1370453002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:789: return nullptr; Note to the future: If this is actually reachable, then even when all platforms have a WebMediaSession implementation, we can't assume that we can create one.
https://codereview.chromium.org/1370453002/diff/180001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1370453002/diff/180001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:15: scoped_ptr<blink::WebMediaSessionActivateCallback>(callback) Add variable so that the scoped_ptr doesn't go out of scope too early. https://codereview.chromium.org/1370453002/diff/180001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:23: scoped_ptr<blink::WebMediaSessionDeactivateCallback>(callback)->onSuccess(); Add variable so that the scoped_ptr doesn't go out of scope too early.
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/220001
Description was changed from ========== Introduce WebMediaSession WebMediaSession is the API enabling web exposed MediaSession objects to control platform implementations of media session related functionality. Implementation will begin with Android and other platforms will be added later. Until there are other platform implementations the tests will only be enabled for Android. BUG=497735 ========== to ========== Introduce WebMediaSession WebMediaSession is the API enabling web exposed MediaSession objects to control platform implementations of media session related functionality. Implementation will begin with Android and other platforms will be added later. BUG=497735 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlamouri@chromium.org
lgtm Thank you so much for applying the comments!
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1370453002/#ps220001 (title: "Moar scope")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mlamouri@chromium.org changed reviewers: + jochen@chromium.org
+jochen@ for content/renderer/
lgtm
https://codereview.chromium.org/1370453002/diff/220001/content/renderer/media... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1370453002/diff/220001/content/renderer/media... content/renderer/media/android/webmediasession_android.h:15: WebMediaSessionAndroid() = default; please add a dtor
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by davve@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, philipj@opera.com, jochen@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1370453002/#ps240001 (title: "Add virtual default destructor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370453002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370453002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/9b97db48d95635e9c5d48676df8dc51c9e5eef2a Cr-Commit-Position: refs/heads/master@{#355058}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/1412933005/ by davve@opera.com. The reason for reverting is: Tests leak under address sanitizer. See https://code.google.com/p/chromium/issues/detail?id=546394.. |
