Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1416123005:
Implement setSinkId() for media elements without src. (Closed)
|
Created:
5 years, 1 month ago by Guido Urdaneta Modified:
5 years, 1 month ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, hta - Chromium, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement setSinkId() for media elements without src.
BUG=543087
Committed: https://crrev.com/3906a72aa1ee5da057879d7e233304101586e8d9
Cr-Commit-Position: refs/heads/master@{#359405}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rename checkAudioSink and fix #includes #Patch Set 3 : More jochen's comments #
Total comments: 12
Patch Set 4 : philipj's comments #
Total comments: 1
Patch Set 5 : More jochen's comments #
Total comments: 8
Patch Set 6 : Peter's comments #Patch Set 7 : rebase #Patch Set 8 : rebase #
Total comments: 21
Patch Set 9 : Peter's comments #Patch Set 10 : rebase #Messages
Total messages: 45 (15 generated)
guidou@chromium.org changed reviewers: + jochen@chromium.org
Hi, PTAL. This is the second CL from the split of crrev.com/1415343004 in two CLs.
jochen@chromium.org changed reviewers: + cwilso@chromium.org
+cwilso@ who asked to be more involved in these changes on the intent to implement https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h:21: class AudioOutputDeviceClient : public WillBeHeapSupplement<LocalFrame> { why do you need this interface? all usages of the impl class appear to be internal to blink? https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebFrameClient.h:682: virtual void checkAudioSink(const WebString& sinkId, const WebSecurityOrigin&, WebSetSinkIdCallbacks*) { } why does this not just return the result? It seems like the callback is executed synchronously anyway? can we call the method something like checkIfAudioSinkExistsAndIsAuthorized?
Description was changed from ========== Implement setSinkId() for media elements without src. BUG=543087 ========== to ========== Implement setSinkId() for media elements without src. BUG=543087 ==========
https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h:21: class AudioOutputDeviceClient : public WillBeHeapSupplement<LocalFrame> { On 2015/10/28 14:18:49, jochen wrote: > why do you need this interface? all usages of the impl class appear to be > internal to blink? I see the following choices: a) Implement in web/: Not possible because modules/audio_output_devices/HTMLMediaElementAudioOutputDevices.cpp needs access to checkIfAudioSinkExistsAndIsAuthorized() and it is forbidden to include web/ from modules/. b) Implement in modules/audio_output_devices/: Not possible because the implementation of checkIfAudioSinkExistsAndIsAuthorized() needs to include web/ and public/web to access WebLocalFrameImpl and WebFrameClient, and it is forbidden to include web/ and public/web/ from modules/. c) Put the .h in modules/ and the .cpp in web/, but I don't think that is allowed either. Is there another way? https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebFrameClient.h:682: virtual void checkAudioSink(const WebString& sinkId, const WebSecurityOrigin&, WebSetSinkIdCallbacks*) { } On 2015/10/28 14:18:49, jochen wrote: > why does this not just return the result? It seems like the callback is executed > synchronously anyway? It's simpler to leave the callback for two reasons: 1. This is used only in a context where the callback is available, as that is the existing mechanism to communicate the results to the upper layer. 2. Error results are in an enum that contains only error codes with no value for success. Thus, we would need to return two values: a bool to indicate success/failure, and an enum to indicate the error code in case of failures. I think this would look less clean than the callback. 3. We would need extra logic in RenderFrameImpl to translate the chromium status code to the bool/enum combination, and also extra logic in Blink to decide whether to call onSuccess() or onError() to communicate the result upstream. Using the callback allows us to use the already existing logic in Chromium to do all this with just a couple of statements. If you think these reasons are not good enough I can of course change it. > can we call the method something like checkIfAudioSinkExistsAndIsAuthorized? Done.
jochen@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h:21: class AudioOutputDeviceClient : public WillBeHeapSupplement<LocalFrame> { that looks a bit like hiding a layering violation to me... adding haraken for his input https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebFrameClient.h:682: virtual void checkAudioSink(const WebString& sinkId, const WebSecurityOrigin&, WebSetSinkIdCallbacks*) { } On 2015/10/28 at 15:58:44, Guido Urdaneta wrote: > On 2015/10/28 14:18:49, jochen wrote: > > why does this not just return the result? It seems like the callback is executed > > synchronously anyway? > > It's simpler to leave the callback for two reasons: > 1. This is used only in a context where the callback is available, as that is the existing mechanism to communicate the results to the upper layer. > 2. Error results are in an enum that contains only error codes with no value for success. Thus, we would need to return two values: a bool to indicate success/failure, and an enum to indicate the error code in case of failures. I think this would look less clean than the callback. > 3. We would need extra logic in RenderFrameImpl to translate the chromium status code to the bool/enum combination, and also extra logic in Blink to decide whether to call onSuccess() or onError() to communicate the result upstream. Using the callback allows us to use the already existing logic in Chromium to do all this with just a couple of statements. > > If you think these reasons are not good enough I can of course change it. > ok, then at least postTask the callback so nobody accidentally can rely on it being sync
https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClient.h:21: class AudioOutputDeviceClient : public WillBeHeapSupplement<LocalFrame> { On 2015/10/28 16:06:47, jochen wrote: > that looks a bit like hiding a layering violation to me... adding haraken for > his input This is a common pattern to bypass a disallowed dependency from modules/ to web/. So looks good.
> ok, then at least postTask the callback so nobody accidentally can rely on it > being sync Done. I PostTasked the whole thing, except for Blink/Chromium type conversions.
https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebFrameClient.h:682: virtual void checkAudioSink(const WebString& sinkId, const WebSecurityOrigin&, WebSetSinkIdCallbacks*) { } On 2015/10/28 16:06:47, jochen wrote: > On 2015/10/28 at 15:58:44, Guido Urdaneta wrote: > > On 2015/10/28 14:18:49, jochen wrote: > > > why does this not just return the result? It seems like the callback is > executed > > > synchronously anyway? > > > > It's simpler to leave the callback for two reasons: > > 1. This is used only in a context where the callback is available, as that is > the existing mechanism to communicate the results to the upper layer. > > 2. Error results are in an enum that contains only error codes with no value > for success. Thus, we would need to return two values: a bool to indicate > success/failure, and an enum to indicate the error code in case of failures. I > think this would look less clean than the callback. > > 3. We would need extra logic in RenderFrameImpl to translate the chromium > status code to the bool/enum combination, and also extra logic in Blink to > decide whether to call onSuccess() or onError() to communicate the result > upstream. Using the callback allows us to use the already existing logic in > Chromium to do all this with just a couple of statements. > > > > If you think these reasons are not good enough I can of course change it. > > > ok, then at least postTask the callback so nobody accidentally can rely on it > being sync > Done. I PostTasked the whole logic, except for type conversions.
guidou@chromium.org changed reviewers: + philipj@opera.com
Adding philipj, who showed interest about this case in the Intent to Ship.
Friendly ping :)
I've only looked at the tests so far. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html (right): https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:9: <audio id="testAudio" src="../resources/media-source/webm/test-a-128k-44100Hz-1ch.webm"></audio> Since the individual tests change this object, I don't think it's a good idea to share it. Better to createElement("audio") for every test and explicitly get it into the state you need for testing. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:26: // do nothing Looking at the definition of promise_test(), it looks like you can just return audio.setSinkId(''), as both then() and catch() are hooked up in the harness. You can do the same for any test that expects the promise to resolve. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:36: promise_test(function() { Looks like you can use promise_rejects(function() { ... }, ) for this and similar tests. Here's an example of how to use it: https://github.com/w3c/web-platform-tests/pull/1490/files https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:59: audio.src="" I think it would be better to test this with a new audio element that has never had a src attribute. If there's value in also testing this case, a separate test would be good.
https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp (right): https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:45: webMediaPlayer->setSinkId(sinkId, WebSecurityOrigin(context->securityOrigin()), callbacks.leakPtr()); Is it correct that it's callbacks.leakPtr() in one branch but callbacks.release() in the other? Without a comment, it looks like a mistake. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:50: // The context has been detached. The promise will never settle. When will this happen? I assume that the spec doesn't allow the promise to remain unsettled in any situation, so if this is the only sensible behavior to implement then the spec should change to somehow allow this.
https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html (right): https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:9: <audio id="testAudio" src="../resources/media-source/webm/test-a-128k-44100Hz-1ch.webm"></audio> On 2015/11/02 12:58:33, philipj wrote: > Since the individual tests change this object, I don't think it's a good idea to > share it. Better to createElement("audio") for every test and explicitly get it > into the state you need for testing. Done. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:26: // do nothing On 2015/11/02 12:58:33, philipj wrote: > Looking at the definition of promise_test(), it looks like you can just return > audio.setSinkId(''), as both then() and catch() are hooked up in the harness. > You can do the same for any test that expects the promise to resolve. Done. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:36: promise_test(function() { On 2015/11/02 12:58:33, philipj wrote: > Looks like you can use promise_rejects(function() { ... }, ) for this and > similar tests. Here's an example of how to use it: > https://github.com/w3c/web-platform-tests/pull/1490/files Done. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/audio_output_devices/setsinkid.html:59: audio.src="" On 2015/11/02 12:58:33, philipj wrote: > I think it would be better to test this with a new audio element that has never > had a src attribute. If there's value in also testing this case, a separate test > would be good. Done. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp (right): https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:45: webMediaPlayer->setSinkId(sinkId, WebSecurityOrigin(context->securityOrigin()), callbacks.leakPtr()); On 2015/11/02 13:04:07, philipj wrote: > Is it correct that it's callbacks.leakPtr() in one branch but > callbacks.release() in the other? Without a comment, it looks like a mistake. Added comment explaining why leakPtr(). I can add another one explaining release() if you consider it helps clarify intent further. https://codereview.chromium.org/1416123005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:50: // The context has been detached. The promise will never settle. On 2015/11/02 13:04:07, philipj wrote: > When will this happen? I assume that the spec doesn't allow the promise to > remain unsettled in any situation, so if this is the only sensible behavior to > implement then the spec should change to somehow allow this. I changed it to return a SecurityError, which seems appropriate given that the problem in this case is that there is no way to check permissions. Hitting this case should be very rare.
One comment wrt letting promises hang around forever, based on discussions at TPAC: It seems that at least one school of thought with regard to Promises is that when the promise-operation no longer makes sense, the right choice is to neither resolve nor reject the promise - it's not a success, and it's not an error. See Domenic's comments on the issue: https://lists.w3.org/Archives/Public/public-script-coord/2015OctDec/0037.html cited in https://github.com/w3c/webrtc-pc/issues/150 I personally think this is behavior that is likely to surprise developers, but I'm all in favor of documenting the behavior and writing tests for it whenever we actually program something like that. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
One comment wrt letting promises hang around forever, based on discussions at TPAC: It seems that at least one school of thought with regard to Promises is that when the promise-operation no longer makes sense, the right choice is to neither resolve nor reject the promise - it's not a success, and it's not an error. See Domenic's comments on the issue: https://lists.w3.org/Archives/Public/public-script-coord/2015OctDec/0037.html cited in https://github.com/w3c/webrtc-pc/issues/150 I personally think this is behavior that is likely to surprise developers, but I'm all in favor of documenting the behavior and writing tests for it whenever we actually program something like that. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Friendly ping to reviewers :)
ping
lgtm from my pov in general, we need to identify a reviewer that actually understands WebRTC (in contrast to me) - I have a really hard time evaluating the changes. Having said that, we should probably wait for positive feedback form either Philip or Chris before landing this. https://codereview.chromium.org/1416123005/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1416123005/diff/60001/content/renderer/render... content/renderer/render_frame_impl.h:890: void checkIfAudioSinkExistsAndIsAuthorized( this function should be in the block of overrides of WebFrameImpl
guidou@chromium.org changed reviewers: + peter@chromium.org
Adding peter@, who has reviewed a lot of WebRTC-related Blink code lately.
Just a quick note to not block this on me. I think the functionality itself makes sense, so if another reviewer has taken a deep dive then that's fine.
https://codereview.chromium.org/1416123005/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1416123005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:5305: base::Bind(&RenderFrameImpl::DoCheckIfAudioSinkExistsAndIsAuthorized, There is a really subtle case in which this PostTask matters (even though the implementation of the check is synchronous) - while resolving the promise will use a microtask and thus appear asynchronous to the developer, not having this PostTask would cause HTMLMediaElementAudioOutputDevice.m_sinkId to be synchronously updated, which *can* be observed. Firstly, I think it would be worth documenting this somewhere - perhaps add a comment here briefly explaining why the PostTask to the same thread matters, it may confuse a reader. Secondly, this would be a good case to test in the layout test, which now doesn't check the `sinkId` property at all. (1) Assert that element.sinkId == '' (2) Call element.setSinkId('somevalue') (3) Assert that element.sinkId == '' (4) Once the promise resolves, element.sinkId == 'somevalue' (Are there keywords we can use for this kind of pass-through?) https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp (right): https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:35: ASSERT(scriptState); note: We seem to be missing step 1 of the algorithm here. 1. If sinkId and the sinkId attribute are the same object, return a resolved promise. (http://www.w3.org/TR/audio-output/#methods) |sinkId| is defined to be the empty string as the default value, so most of the cases in the layout test (lines 24, 30, 50, 52, 63 and 66) would actually hit this step. https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:52: resolver->reject(); nit: why not reject the resolver with the dom error? Then you can just fall-through to the return on line 57. resolver->reject(DOMException::create(SecurityError, "Impossible to authorize device for detached context")); https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:682: virtual void checkIfAudioSinkExistsAndIsAuthorized(const WebString& sinkId, const WebSecurityOrigin&, WebSetSinkIdCallbacks*) { } nit: document that ownership of |callbacks| is passed. Not having an implementation of this method means that we'd leak the callbacks. Can we BLINK_ASSERT_NOT_REACHED() in the default implementation's body?
https://codereview.chromium.org/1416123005/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1416123005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:5305: base::Bind(&RenderFrameImpl::DoCheckIfAudioSinkExistsAndIsAuthorized, On 2015/11/09 20:05:06, Peter Beverloo wrote: > There is a really subtle case in which this PostTask matters (even though the > implementation of the check is synchronous) - while resolving the promise will > use a microtask and thus appear asynchronous to the developer, not having this > PostTask would cause HTMLMediaElementAudioOutputDevice.m_sinkId to be > synchronously updated, which *can* be observed. > > Firstly, I think it would be worth documenting this somewhere - perhaps add a > comment here briefly explaining why the PostTask to the same thread matters, it > may confuse a reader. > > Secondly, this would be a good case to test in the layout test, which now > doesn't check the `sinkId` property at all. > > (1) Assert that element.sinkId == '' > (2) Call element.setSinkId('somevalue') > (3) Assert that element.sinkId == '' > (4) Once the promise resolves, element.sinkId == 'somevalue' > > (Are there keywords we can use for this kind of pass-through?) I changed the implementation so that it is made asynchronous in Blink. The reason is that when I added the mock version of checkIfAudioSinkExistsAndIsAuthorized() to WebFrameTestProxy, the promise was resolved/rejected too quickly and the layout test was getting undefined as the result of setSinkId() instead of a Promise object. I don't know if this is a feature or a bug, but it did not happen in tests when the real implementation was used. The sinkId property check was added to the test. https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp (right): https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:35: ASSERT(scriptState); On 2015/11/09 20:05:06, Peter Beverloo wrote: > note: We seem to be missing step 1 of the algorithm here. > > 1. If sinkId and the sinkId attribute are the same object, return a > resolved promise. (http://www.w3.org/TR/audio-output/#methods) > > |sinkId| is defined to be the empty string as the default value, so most of the > cases in the layout test (lines 24, 30, 50, 52, 63 and 66) would actually hit > this step. Done. https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:52: resolver->reject(); On 2015/11/09 20:05:06, Peter Beverloo wrote: > nit: why not reject the resolver with the dom error? Then you can just > fall-through to the return on line 57. > > resolver->reject(DOMException::create(SecurityError, "Impossible to authorize > device for detached context")); Done. Note that the implementation is slightly different now as it is asynchronous in Blink. https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:682: virtual void checkIfAudioSinkExistsAndIsAuthorized(const WebString& sinkId, const WebSecurityOrigin&, WebSetSinkIdCallbacks*) { } On 2015/11/09 20:05:06, Peter Beverloo wrote: > nit: document that ownership of |callbacks| is passed. > > Not having an implementation of this method means that we'd leak the callbacks. > Can we BLINK_ASSERT_NOT_REACHED() in the default implementation's body? Done.
lgtm % comments https://codereview.chromium.org/1416123005/diff/140001/components/test_runner... File components/test_runner/web_frame_test_proxy.h (right): https://codereview.chromium.org/1416123005/diff/140001/components/test_runner... components/test_runner/web_frame_test_proxy.h:282: web_callback); nit: web_callbacks (elsewhere too) https://codereview.chromium.org/1416123005/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1416123005/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.h:29: #include "media/base/output_device.h" nit: no need to include this header https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html:33: return Promise.resolve(); nit: no need, that propagates automagically. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html:69: return Promise.resolve(); dito https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html:72: }, 'setSinkId("valid") followed by setSinkId("").'); q: have you tested the behaviour of: let element = document.createElement('audio'); element.setSinkId('valid').then(() => { element.src = 'mediafile.oga'; // which sink is being used? }); https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:17: class SetSinkIdResolver : public ScriptPromiseResolver { note: you've split out the smaller SetSinkIdCallbacks to its own file, but define this in-file. You may want to consider moving it to its own file, but I'm OK with keeping it here too, whatever you prefer. If you keep it in this file, please do put it in an anonymous namespace. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:21: ~SetSinkIdResolver() = default; +override https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:53: if (m_sinkId == HTMLMediaElementAudioOutputDevice::sinkId(*m_element)) { nit: since we can do this check synchronously, we should do it in HTMLMediaElementAudioOutputDevice::setSinkId(). This (1) matches the spec, (2) remove the overhead of starting a timer and (3) remove the need to use the static accessor. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:67: client->checkIfAudioSinkExistsAndIsAuthorized(context, m_sinkId, callbacks.release()); Note that there will still be a gap between updating the value (which happens during the timer execution) and resolving the promise (which happens using a microtask at the end of the event loop), but since neither are in the developer's control this should be OK. Alex (scheduling team) tells me that it's effectively indeterminable. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h:32: friend class SetSinkIdResolver; Why do we need this? https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:52: #include "public/platform/WebSetSinkIdCallbacks.h" nit: we can forward declare this
https://codereview.chromium.org/1416123005/diff/140001/components/test_runner... File components/test_runner/web_frame_test_proxy.h (right): https://codereview.chromium.org/1416123005/diff/140001/components/test_runner... components/test_runner/web_frame_test_proxy.h:282: web_callback); On 2015/11/12 16:45:42, Peter Beverloo wrote: > nit: web_callbacks (elsewhere too) Done. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html:33: return Promise.resolve(); On 2015/11/12 16:45:42, Peter Beverloo wrote: > nit: no need, that propagates automagically. Done. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html:69: return Promise.resolve(); On 2015/11/12 16:45:42, Peter Beverloo wrote: > dito Done. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html:72: }, 'setSinkId("valid") followed by setSinkId("").'); On 2015/11/12 16:45:42, Peter Beverloo wrote: > q: have you tested the behaviour of: > > let element = document.createElement('audio'); > element.setSinkId('valid').then(() => { > element.src = 'mediafile.oga'; > // which sink is being used? > }); I've tested it manually and it works as expected. 'valid' would be used. I did not add a unit test for that case because there are a few issues: 1. element.sinkId will return 'valid' regardless of whether 'mediafile.oga' plays on that sink or not, as it is a property of the element. 2. at the moment, 'mediafile.oga' will not play because there is no MockWebMediaPlayer that can play on the 'valid' device. The real implementation of WebMediaPlayer will log an error message as 'valid' does not exist and will play nothing. 3. WebMediaPlayer doesn't provide a way to detect that it failed to play on the selected sink on startup, or to report which sink it is actually using. One way to address this would be to add a getSinkId() method to WebMediaPlayer that reports the sink being used (or an error). I added a TODO referencing crbug.com/546566, which talks about this problem. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:17: class SetSinkIdResolver : public ScriptPromiseResolver { On 2015/11/12 16:45:42, Peter Beverloo wrote: > note: you've split out the smaller SetSinkIdCallbacks to its own file, but > define this in-file. > > You may want to consider moving it to its own file, but I'm OK with keeping it > here too, whatever you prefer. If you keep it in this file, please do put it in > an anonymous namespace. Done. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:21: ~SetSinkIdResolver() = default; On 2015/11/12 16:45:42, Peter Beverloo wrote: > +override Done. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:53: if (m_sinkId == HTMLMediaElementAudioOutputDevice::sinkId(*m_element)) { On 2015/11/12 16:45:42, Peter Beverloo wrote: > nit: since we can do this check synchronously, we should do it in > HTMLMediaElementAudioOutputDevice::setSinkId(). > > This (1) matches the spec, (2) remove the overhead of starting a timer and (3) > remove the need to use the static accessor. Done. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:67: client->checkIfAudioSinkExistsAndIsAuthorized(context, m_sinkId, callbacks.release()); On 2015/11/12 16:45:42, Peter Beverloo wrote: > Note that there will still be a gap between updating the value (which happens > during the timer execution) and resolving the promise (which happens using a > microtask at the end of the event loop), but since neither are in the > developer's control this should be OK. > > Alex (scheduling team) tells me that it's effectively indeterminable. Acknowledged. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h:32: friend class SetSinkIdResolver; On 2015/11/12 16:45:42, Peter Beverloo wrote: > Why do we need this? We don't. An early version of SetSinkIdResolver required it, and I didn't realize that the final version does not require it. Fixed. https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/1416123005/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:52: #include "public/platform/WebSetSinkIdCallbacks.h" On 2015/11/12 16:45:42, Peter Beverloo wrote: > nit: we can forward declare this We can't because WebSetSinkIdCallbacks is a typedef.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/1416123005/#ps160001 (title: "Peter's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416123005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416123005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by guidou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416123005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416123005/160001
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...)
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1416123005/#ps180001 (title: "rebase")
The CQ bit was unchecked by guidou@chromium.org
The CQ bit was checked by guidou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416123005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416123005/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3906a72aa1ee5da057879d7e233304101586e8d9 Cr-Commit-Position: refs/heads/master@{#359405} |
