Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2424163004:
Factor out authorization from AudioRendererHost. (Closed)
|
Created:
4 years, 2 months ago by Max Morin Modified:
4 years, 1 month ago CC:
audio-mojo-cl_google.com, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor out hadndling of the authorization IPC from AudioRendererHost.
This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations.
BUG=656923
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/b08e842c3fab03521e0c1fb954c39453c96edeab
Cr-Commit-Position: refs/heads/master@{#433160}
Patch Set 1 #Patch Set 2 : Rename. #Patch Set 3 : Add null check. Add comments. #Patch Set 4 : Remove unnecessary weak pointer use. #
Total comments: 21
Patch Set 5 : . #
Total comments: 19
Patch Set 6 : Unit tests. Address other comments. #
Total comments: 4
Patch Set 7 : Guidos comments. #
Total comments: 16
Patch Set 8 : Dale's comments. #
Total comments: 33
Patch Set 9 : Threaded unit tests. More unit tests. #Patch Set 10 : Minor fixes. #
Total comments: 7
Patch Set 11 : . #Patch Set 12 : Don't use separate UI thread for test. #
Total comments: 21
Patch Set 13 : . #
Total comments: 20
Patch Set 14 : Rename things. #
Total comments: 11
Patch Set 15 : . #
Total comments: 1
Patch Set 16 : const& #Messages
Total messages: 64 (21 generated)
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
maxmorin@chromium.org changed reviewers: + olka@chromium.org
Olga: WDYT? This code is exercised by the current tests of ARH, and this class is internal to ARH, so I don't think more specific unit tests for this are needed yet.
Description was changed from ========== Factor out authorization from AudioRendererHost. BUG=656923 ========== to ========== Factor out authorization from AudioRendererHost. BUG=656923 ==========
Description was changed from ========== Factor out authorization from AudioRendererHost. BUG=656923 ========== to ========== Factor out authorization from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. BUG=656923 ==========
maxmorin@chromium.org changed reviewers: + guidou@chromium.org
Guido: Could you take a look at this? Basically, the device authorization checking flow is factored out into its own class.
Description was changed from ========== Factor out authorization from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. BUG=656923 ========== to ========== Factor out authorization from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations. BUG=656923 ==========
Nice! https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:18: device_id == media::AudioDeviceDescription::kDefaultDeviceId || l.17 and 18 are AudioDeviceDescription::IsDefaultDevice() https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:118: AuthorizationCompletedCallback cb, Everywhere in this class: Usually callback is the last parameter, is there a specific reason you put it at the beginning? https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:218: media::AudioParameters output_params) { const ref, and make a copy only if needed (as in the original code)? https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:47: AuthorizationCompletedCallback cb); everywhere: pass the callback around as const reference? https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:397: authorization_manager_.RequestDeviceAuthorization( |authorization_handler_|? https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:406: UMALogDeviceAuthorizationTime(auth_start_time); Move it to be called right before Send() https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:407: auto auth_data = authorizations_.find(stream_id); thread check? https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:634: return authorizations_.find(stream_id) != authorizations_.end(); thread check?
guidou@ - a question to you https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (left): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:273: std::map<int, std::pair<bool, std::string>> authorizations_; Guido, why do we actually need a stream id for authorization? Why not (device id, security origin) pair? Why should we re-authorize the same pair for a new stream? (curious)
https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (left): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:273: std::map<int, std::pair<bool, std::string>> authorizations_; On 2016/10/28 08:12:20, o1ka wrote: > Guido, why do we actually need a stream id for authorization? Why not (device > id, security origin) pair? Why should we re-authorize the same pair for a new > stream? (curious) Because the user can revoke permissions at any time.
https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:18: device_id == media::AudioDeviceDescription::kDefaultDeviceId || On 2016/10/27 13:58:09, o1ka wrote: > l.17 and 18 are AudioDeviceDescription::IsDefaultDevice() Done. https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:118: AuthorizationCompletedCallback cb, On 2016/10/27 13:58:09, o1ka wrote: > Everywhere in this class: Usually callback is the last parameter, is there a > specific reason you put it at the beginning? I was not aware of this convention. Making it the very last won't work (because unbound parameters have to be last), but I could make it second last or somewhat late in the parameter list. https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:218: media::AudioParameters output_params) { On 2016/10/27 13:58:08, o1ka wrote: > const ref, and make a copy only if needed (as in the original code)? This specific function needs to mutate the parameters (call to MaybeFixAudioParameters), so I leave this one as passed by value. I changed to const ref everywhere else. https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:47: AuthorizationCompletedCallback cb); On 2016/10/27 13:58:09, o1ka wrote: > everywhere: pass the callback around as const reference? How would we guarantee that it's alive in that case? It is created by the caller, but since the reply is async the caller will return from the method creating the callback before we call it, usually. https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:397: authorization_manager_.RequestDeviceAuthorization( On 2016/10/27 13:58:09, o1ka wrote: > |authorization_handler_|? Done. https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:406: UMALogDeviceAuthorizationTime(auth_start_time); On 2016/10/27 13:58:09, o1ka wrote: > Move it to be called right before Send() Done. https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:407: auto auth_data = authorizations_.find(stream_id); On 2016/10/27 13:58:09, o1ka wrote: > thread check? Done. https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:634: return authorizations_.find(stream_id) != authorizations_.end(); On 2016/10/27 13:58:09, o1ka wrote: > thread check? Done.
https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:118: AuthorizationCompletedCallback cb, On 2016/10/28 11:16:07, Max Morin Chromium wrote: > On 2016/10/27 13:58:09, o1ka wrote: > > Everywhere in this class: Usually callback is the last parameter, is there a > > specific reason you put it at the beginning? > > I was not aware of this convention. Making it the very last won't work (because > unbound parameters have to be last), but I could make it second last or somewhat > late in the parameter list. Ok, it's fine. "Usually" means just "usually" :) https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:218: media::AudioParameters output_params) { On 2016/10/28 11:16:07, Max Morin Chromium wrote: > On 2016/10/27 13:58:08, o1ka wrote: > > const ref, and make a copy only if needed (as in the original code)? > > This specific function needs to mutate the parameters (call to > MaybeFixAudioParameters), so I leave this one as passed by value. I changed to > const ref everywhere else. Ok, if this is your preference - just make a comment in the header. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:60: AudioOutputAuthorizationHandler::AudioOutputAuthorizationHandler( I guess we need some unit tests for it? https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:393: // Unretained is ok here since this owns the authorization_handler. nit: |this| and |authorization_handler_|
Most of my comments are about documentation, but I also recommend to get rid of the badmessage handler. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:129: bad_message_handler_.Run(bad_message::ARH_UNAUTHORIZED_URL); why not kill the renderer process immediately here? Do you expect to do something different in the mojo-based version? https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:24: class AudioOutputAuthorizationHandler { Is this class really necessary in the context of migrating to Mojo? My first impression is that this is actually harder to read than the original, with the new callback types, bad-message handler, and so on. You should have a comment explaining what this class does (handle the logic for the authorization IPC). The name of the class (and the IPC message) suggests that this is only about authorization, but it actually also involves translating the device ID and reading hardware parameters. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:26: using AuthorizationCompletedCallback = This callback type looks not trivial. You should have a comment explaining what the semantics are, what each of the result parameters mean, etc. I understand that this will be the replacement of the authorization reply message in the mojo-based version. Since you are using std::move for this callback, I think you should change it to OnceCallback. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:31: using BadMessageHandler = Why is a BadMessageHandler needed? Can't you just call ReceivedBadMessage directly at the point where you detect the bad message? You only need the process ID, which you already have in a member variable. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:49: // In case an invalid request is recieved, this handler will be called. typo: recieved -> received https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:50: void SetBadMessageHandler(BadMessageHandler handler) { If you're going to use a handler, why not pass it in the constructor? https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:393: // Unretained is ok here since this owns the authorization_handler. I think the explanation is not complete. You also have to clarify that the the callback is invoked only by authorization_handler. Since ARH outlives AH, when AH runs the callback, ARH (this) is guaranteed to be valid. If AH did something else with the callback (like posting it as a task to another thread) it would not be ok. Maybe you should write that guarantee in the documentation for AH.
PTAL. Unit tests are added, and all comments should be resolved (otherwise, tell me). https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:406: UMALogDeviceAuthorizationTime(auth_start_time); On 2016/10/28 11:16:07, Max Morin Chromium wrote: > On 2016/10/27 13:58:09, o1ka wrote: > > Move it to be called right before Send() > > Done. This will actually change the meaning of the stat slightly, but I think it'll be ok. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:60: AudioOutputAuthorizationHandler::AudioOutputAuthorizationHandler( On 2016/10/28 13:18:40, o1ka wrote: > I guess we need some unit tests for it? Done. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:129: bad_message_handler_.Run(bad_message::ARH_UNAUTHORIZED_URL); On 2016/10/28 15:17:28, Guido Urdaneta wrote: > why not kill the renderer process immediately here? > Do you expect to do something different in the mojo-based version? Sure, I'll just get rid of the bad message handler to make it cleaner. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:24: class AudioOutputAuthorizationHandler { On 2016/10/28 15:17:29, Guido Urdaneta wrote: > Is this class really necessary in the context of migrating to Mojo? > My first impression is that this is actually harder to read than the original, > with the new callback types, bad-message handler, and so on. > > You should have a comment explaining what this class does (handle the logic for > the authorization IPC). The name of the class (and the IPC message) suggests > that this is only about authorization, but it actually also involves translating > the device ID and reading hardware parameters. The class isn't strictly necessary, but I want to break AudioRendererHost into smaller pieces, and this is one way to move a lot of self-contained logic out of there. It could also be in the future OutputManager which will take more of ARHs logic and own an instance of this class, but I think having smaller classes is generally preferable. It's also true that it does many things, which is not ideal, but the current alternative is to have it all in the ARH, so this change makes concerns more separated, even if it's not completely separated. It also eliminates some sources of errors (i.e. it's now easy to verify that UMALogDeviceAuthorizationTime is called once, before it was called from seven different places, and similarly it's nice to not have Send(new AudioMsg_NotifyDeviceAuthorized...) in several places). Added documentation. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:26: using AuthorizationCompletedCallback = On 2016/10/28 15:17:29, Guido Urdaneta wrote: > This callback type looks not trivial. You should have a comment explaining what > the semantics are, what each of the result parameters mean, etc. > I understand that this will be the replacement of the authorization reply > message in the mojo-based version. > > Since you are using std::move for this callback, I think you should change it to > OnceCallback. OnceCallback makes sense, but it is not possible to switch yet since many dependencies (at least PostTaskAndReply and MediaStreamUIProxy::CheckAccess) take RepeatableCallbacks and OnceCallbacks cannot be bound into RepeatableCallbacks. I could maybe fix MediaStreamUIProxy, but I definitely think PostTaskAndReply should be left to the people in crbug.com/554299. Added documentation. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:31: using BadMessageHandler = On 2016/10/28 15:17:28, Guido Urdaneta wrote: > Why is a BadMessageHandler needed? > Can't you just call ReceivedBadMessage directly at the point where you detect > the bad message? > You only need the process ID, which you already have in a member variable. Done. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:49: // In case an invalid request is recieved, this handler will be called. On 2016/10/28 15:17:29, Guido Urdaneta wrote: > typo: recieved -> received Done. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:50: void SetBadMessageHandler(BadMessageHandler handler) { On 2016/10/28 15:17:28, Guido Urdaneta wrote: > If you're going to use a handler, why not pass it in the constructor? Removed handler. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:393: // Unretained is ok here since this owns the authorization_handler. On 2016/10/28 15:17:29, Guido Urdaneta wrote: > I think the explanation is not complete. > You also have to clarify that the the callback is invoked only by > authorization_handler. Since ARH outlives AH, when AH runs the callback, ARH > (this) is guaranteed to be valid. > If AH did something else with the callback (like posting it as a task to another > thread) it would not be ok. > Maybe you should write that guarantee in the documentation for AH. Done.
Looking good. Still a few minor comments, but feel free to add owners to the review now. https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:24: class AudioOutputAuthorizationHandler { Now that I understand it, I like it. Maybe you should be more explicit in the comment about saying that this handles all the logic related to the authorization/device request IPC. https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:210: const media::AudioParameters& output_params) { Actually, I think having the parameter by value here like you had in a previous patchset is actually better. Having it by value, means the temporary object is used directly as the parameter due to compiler optimizations. Having it by const-ref means you have to copy the temporary to the nonconst object (this also applies to the bogus std::move in the original code, which also does a copy since you can't move a const). If you leave it by value and explain in a comment why, I think that would be better. However, if OWNERs tell you otherwise, do what the owners say. https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:114: loop.RunUntilIdle(); you can make this shorter by avoiding the variable and doing base::RunLoop().RunUntilIdle()
Description was changed from ========== Factor out authorization from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations. BUG=656923 ========== to ========== Factor out hadndling of the authorization IPC from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations. BUG=656923 ==========
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org
maxmorin@chromium.org changed reviewers: + avi@chromium.org
Thanks for all the reviews so far! Dale: PTAL at everything as owner. Avi: PTAL at content/browser/BUILD.gn https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:24: class AudioOutputAuthorizationHandler { On 2016/11/01 13:49:16, Guido Urdaneta wrote: > Now that I understand it, I like it. > Maybe you should be more explicit in the comment about saying that this handles > all the logic related to the authorization/device request IPC. Done. https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:210: const media::AudioParameters& output_params) { On 2016/11/01 13:49:16, Guido Urdaneta wrote: > Actually, I think having the parameter by value here like you had in a previous > patchset is actually better. > Having it by value, means the temporary object is used directly as the parameter > due to compiler optimizations. > Having it by const-ref means you have to copy the temporary to the nonconst > object (this also applies to the bogus std::move in the original code, which > also does a copy since you can't move a const). > If you leave it by value and explain in a comment why, I think that would be > better. However, if OWNERs tell you otherwise, do what the owners say. Done. https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/100001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:114: loop.RunUntilIdle(); On 2016/11/01 13:49:16, Guido Urdaneta wrote: > you can make this shorter by avoiding the variable and doing > base::RunLoop().RunUntilIdle() Done.
lgtm stamp with nit https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:90: #endif // CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_AUTHORIZATION_HANDLER_H_ nit: two blank lines, one after the closing of the class, one after the closing of the namespace.
Nice cleanup! I think this has helped me see where our renderer hangs may be coming from too. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:17: static const std::string::size_type kValidLength = 64; No need for static const? Just use constexpr? Seems like this should actually be a property of the HMAC generation code. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:39: if (params->channels() > media::limits::kMaxChannels) Probably this should DCHECK that the channel layout is actually discrete in this case. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:53: if (media::AudioDeviceDescription::IsDefaultDevice(unique_id)) Prefer ternary? Up to you though. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:83: if (!IsValidDeviceId(device_id)) { Seems like this should be BadMessage? https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:139: permission_checker_.CheckPermission( How does this interplay with the device authorization timeout on Windows/OSX? It seems like this is blocking for user permission which may result in hanging the render thread. +olka. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:52: AuthorizationCompletedCallback cb); "Pass `Callback` objects by value if ownership is transferred; otherwise, pass it by const-reference." https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:402: media::AudioParameters params, const& We don't have a move constructor for params do we?
https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:139: permission_checker_.CheckPermission( On 2016/11/01 22:01:13, DaleCurtis wrote: > How does this interplay with the device authorization timeout on Windows/OSX? It > seems like this is blocking for user permission which may result in hanging the > render thread. +olka. This introduces some delay with nondefault devices, but it does not block waiting for user input. CheckPermission only checks if permission has already been granted but does not prompt the user.
Thanks guidou@. lgtm % comments then.
Avi: Please also take a look at content/browser/bad_message.h. The UNAUTHORIZED_URL message has the same meaning as before, it was renamed from ARH to AOAH since the place the message is reported from changed. Sorry about the two review rounds. Holte: PTAL at histograms.xml, also see the above note about a renamed message. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:17: static const std::string::size_type kValidLength = 64; On 2016/11/01 22:01:13, DaleCurtis wrote: > No need for static const? Just use constexpr? Seems like this should actually be > a property of the HMAC generation code. I was lazy and just moved this code over from ARH, but it doesn't belong here at all. I'll put it in media_device_id and media_stream_manager with the other hashing-related functions. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:39: if (params->channels() > media::limits::kMaxChannels) On 2016/11/01 22:01:13, DaleCurtis wrote: > Probably this should DCHECK that the channel layout is actually discrete in this > case. Done. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:53: if (media::AudioDeviceDescription::IsDefaultDevice(unique_id)) On 2016/11/01 22:01:13, DaleCurtis wrote: > Prefer ternary? Up to you though. Done. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:83: if (!IsValidDeviceId(device_id)) { On 2016/11/01 22:01:13, DaleCurtis wrote: > Seems like this should be BadMessage? Done. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:52: AuthorizationCompletedCallback cb); On 2016/11/01 22:01:13, DaleCurtis wrote: > "Pass `Callback` objects by value if ownership is transferred; otherwise, pass > it > by const-reference." By value makes sense here. Since we bind it in another callback, we don't have to reference-count if we move it, but we do if we bind it to a function taking a const& (unless we use base::ConstRef, but base::ConstRef would leave a dangling reference when we make thread hops). An added bonus is that migration to OnceCallback is easy once the surrounding code is ready. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:90: #endif // CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_AUTHORIZATION_HANDLER_H_ On 2016/11/01 15:06:34, Avi wrote: > nit: two blank lines, one after the closing of the class, one after the closing > of the namespace. Done. https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:402: media::AudioParameters params, On 2016/11/01 22:01:13, DaleCurtis wrote: > const& We don't have a move constructor for params do we? Ah, we actually don't. I'll change it back then.
maxmorin@chromium.org changed reviewers: + holte@chromium.org
Bad message file LGTM. No worries about multiple reviews.
lgtm histograms lgtm
https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:89: cb.Run(media::OUTPUT_DEVICE_STATUS_OK, std::move(output_params), remove "move"? https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: device_info.device_id)); +guidou@ - I'm a bit confused: shouldn't we return |device_id| here instead of raw id, which |device_info.device_id| is? Suspiciously looks like a bug that we missed before? (this is inherited from the original code) Overall, I find all that device id naming quite misleading (I know it's inherited from the original code) What is unique_id we expect to receive in DeviceParametersReceived? https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:192: const std::string& unique_id, we need to name this appropriately (see my comment above). https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:198: media::AudioParameters::UnavailableDeviceParams(), ""); std::string()? https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:32: using AuthorizationCompletedCallback = Please explain parameters meaning in the comment - it's really unclear, specially what to expect in "string" part. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:48: void RequestDeviceAuthorization(int render_frame_id, const method? https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:54: MediaDevicesPermissionChecker& GetMediaDevicesPermissionCheckerForTesting() { I feel like it should be const ref. Probably fix MediaDevicesPermissionChecker to have all methods as const? https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:59: void CheckOutputDeviceAccess(AuthorizationCompletedCallback cb, all methods const? https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:64: void AccessChecked(AuthorizationCompletedCallback cb, We need to explain each of the methods here. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:25: const int kRenderProcessId = 1; hide statics in an unnamed namespace https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:28: const GURL kSecurityOriginGURL(kSecurityOriginString); only PODs can be static: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:61: TestBrowserThreadBundle thread_bundle_; All the browser threads are emulated with a single thread here, so technically you run it all on UI thread. https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_... Probably we need to set up multi-threaded testing? WDYT? https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:185: EXPECT_EQ(RPH.bad_msg_count(), 1); nit: expect it to be 0 at the beginning of the test https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:1973: bool MediaStreamManager::IsValidDeviceId(const std::string& device_id) { What is the reason for MediaStreaMgr to expose it? Isn't it enough oi have it in media_device_id? https://codereview.chromium.org/2424163004/diff/140001/content/public/browser... File content/public/browser/media_device_id.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/public/browser... content/public/browser/media_device_id.cc:40: return MediaStreamManager::IsValidDeviceId(device_id); Move the entire implementation?
https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: device_info.device_id)); On 2016/11/03 10:07:22, o1ka wrote: > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here instead of > raw id, which |device_info.device_id| is? Suspiciously looks like a bug that we > missed before? (this is inherited from the original code) > > Overall, I find all that device id naming quite misleading (I know it's > inherited from the original code) > > What is unique_id we expect to receive in DeviceParametersReceived? We should use device_id instead of device_info.device-id. Nice catch! Maybe we need a unit test to verify this?
On 2016/11/03 10:11:52, Guido Urdaneta wrote: > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > File content/browser/renderer_host/media/audio_output_authorization_handler.cc > (right): > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: > device_info.device_id)); > On 2016/11/03 10:07:22, o1ka wrote: > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here instead of > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug that > we > > missed before? (this is inherited from the original code) > > > > Overall, I find all that device id naming quite misleading (I know it's > > inherited from the original code) > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > We should use device_id instead of device_info.device-id. Nice catch! > Maybe we need a unit test to verify this? Agree, we need it.
On 2016/11/03 10:11:52, Guido Urdaneta wrote: > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > File content/browser/renderer_host/media/audio_output_authorization_handler.cc > (right): > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: > device_info.device_id)); > On 2016/11/03 10:07:22, o1ka wrote: > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here instead of > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug that > we > > missed before? (this is inherited from the original code) > > > > Overall, I find all that device id naming quite misleading (I know it's > > inherited from the original code) > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > We should use device_id instead of device_info.device-id. Nice catch! > Maybe we need a unit test to verify this? maxmorin@ we may need to wait with landing this CL until guidou@ lands the fix and merges it to m55 (looks like the bug got there)
On 2016/11/03 10:19:35, o1ka wrote: > On 2016/11/03 10:11:52, Guido Urdaneta wrote: > > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > > File content/browser/renderer_host/media/audio_output_authorization_handler.cc > > (right): > > > > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > > content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: > > device_info.device_id)); > > On 2016/11/03 10:07:22, o1ka wrote: > > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here instead > of > > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug that > > we > > > missed before? (this is inherited from the original code) > > > > > > Overall, I find all that device id naming quite misleading (I know it's > > > inherited from the original code) > > > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > > > We should use device_id instead of device_info.device-id. Nice catch! > > Maybe we need a unit test to verify this? > > maxmorin@ we may need to wait with landing this CL until guidou@ lands the fix > and merges it to m55 (looks like the bug got there) m55 is good. We send the empty string there. The bug is in this CL.
On 2016/11/03 10:27:12, Guido Urdaneta wrote: > On 2016/11/03 10:19:35, o1ka wrote: > > On 2016/11/03 10:11:52, Guido Urdaneta wrote: > > > > > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > > > File > content/browser/renderer_host/media/audio_output_authorization_handler.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > > > > content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: > > > device_info.device_id)); > > > On 2016/11/03 10:07:22, o1ka wrote: > > > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here > instead > > of > > > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug > that > > > we > > > > missed before? (this is inherited from the original code) > > > > > > > > Overall, I find all that device id naming quite misleading (I know it's > > > > inherited from the original code) > > > > > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > > > > > We should use device_id instead of device_info.device-id. Nice catch! > > > Maybe we need a unit test to verify this? > > > > maxmorin@ we may need to wait with landing this CL until guidou@ lands the fix > > and merges it to m55 (looks like the bug got there) > > m55 is good. We send the empty string there. The bug is in this CL. o1ka: See AudioRendererHost::DeviceParametersReceived in the original. max: Make sure you have a unit test to check this.
Description was changed from ========== Factor out hadndling of the authorization IPC from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations. BUG=656923 ========== to ========== Factor out hadndling of the authorization IPC from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations. BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
PTAL https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:89: cb.Run(media::OUTPUT_DEVICE_STATUS_OK, std::move(output_params), On 2016/11/03 10:07:22, o1ka wrote: > remove "move"? Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: device_info.device_id)); On 2016/11/03 10:11:52, Guido Urdaneta wrote: > On 2016/11/03 10:07:22, o1ka wrote: > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here instead of > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug that > we > > missed before? (this is inherited from the original code) > > > > Overall, I find all that device id naming quite misleading (I know it's > > inherited from the original code) > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > We should use device_id instead of device_info.device-id. Nice catch! > Maybe we need a unit test to verify this? ARH actually needs the translated id. I'll make the ARH hash the id again before sending it to the renderer. Test is CreateUnifiedStream in audio_renderer_host_unittest.cc, assertion is on line 194. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:192: const std::string& unique_id, On 2016/11/03 10:07:22, o1ka wrote: > we need to name this appropriately (see my comment above). I changed this to translated_device_id. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:198: media::AudioParameters::UnavailableDeviceParams(), ""); On 2016/11/03 10:07:22, o1ka wrote: > std::string()? Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:32: using AuthorizationCompletedCallback = On 2016/11/03 10:07:22, o1ka wrote: > Please explain parameters meaning in the comment - it's really unclear, > specially what to expect in "string" part. I added this to the comment on RequestDeviceAuthorization. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:48: void RequestDeviceAuthorization(int render_frame_id, On 2016/11/03 10:07:22, o1ka wrote: > const method? Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:54: MediaDevicesPermissionChecker& GetMediaDevicesPermissionCheckerForTesting() { On 2016/11/03 10:07:22, o1ka wrote: > I feel like it should be const ref. Probably fix MediaDevicesPermissionChecker > to have all methods as const? The purpose was actually to allow the caller to modify MediaDevicesPermissionChecker (add override), I added a simpler method instead of this one. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:59: void CheckOutputDeviceAccess(AuthorizationCompletedCallback cb, On 2016/11/03 10:07:22, o1ka wrote: > all methods const? Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:64: void AccessChecked(AuthorizationCompletedCallback cb, On 2016/11/03 10:07:22, o1ka wrote: > We need to explain each of the methods here. I added an overview of how the implementation of RequestDeviceAuthorization works. The individual methods doesn't make sense in isolation, it's more like the entire class is one long function that had to be divided into pieces due to async calls. It could be divided into functions in a way that makes sense, but as discussed offline, it's not a priority right now. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:25: const int kRenderProcessId = 1; On 2016/11/03 10:07:22, o1ka wrote: > hide statics in an unnamed namespace Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:28: const GURL kSecurityOriginGURL(kSecurityOriginString); On 2016/11/03 10:07:22, o1ka wrote: > only PODs can be static: > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:61: TestBrowserThreadBundle thread_bundle_; On 2016/11/03 10:07:22, o1ka wrote: > All the browser threads are emulated with a single thread here, so technically > you run it all on UI thread. > https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_... > > Probably we need to set up multi-threaded testing? WDYT? Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:185: EXPECT_EQ(RPH.bad_msg_count(), 1); On 2016/11/03 10:07:22, o1ka wrote: > nit: expect it to be 0 at the beginning of the test Done. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:1973: bool MediaStreamManager::IsValidDeviceId(const std::string& device_id) { On 2016/11/03 10:07:22, o1ka wrote: > What is the reason for MediaStreaMgr to expose it? Isn't it enough oi have it in > media_device_id? I figured I'd put it with the rest of the hashing-related functions, but I've moved it to media_device_id now. https://codereview.chromium.org/2424163004/diff/140001/content/public/browser... File content/public/browser/media_device_id.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/public/browser... content/public/browser/media_device_id.cc:40: return MediaStreamManager::IsValidDeviceId(device_id); On 2016/11/03 10:07:22, o1ka wrote: > Move the entire implementation? Done.
Will check the tests later, although they look comprehensive at first glance. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: device_info.device_id)); On 2016/11/10 14:59:52, Max Morin Chromium wrote: > On 2016/11/03 10:11:52, Guido Urdaneta wrote: > > On 2016/11/03 10:07:22, o1ka wrote: > > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here instead > of > > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug that > > we > > > missed before? (this is inherited from the original code) > > > > > > Overall, I find all that device id naming quite misleading (I know it's > > > inherited from the original code) > > > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > > > We should use device_id instead of device_info.device-id. Nice catch! > > Maybe we need a unit test to verify this? > > ARH actually needs the translated id. I'll make the ARH hash the id again before > sending it to the renderer. Test is CreateUnifiedStream in > audio_renderer_host_unittest.cc, assertion is on line 194. Actually you can provide the empty string, as in the code you are replacing. The translated ID is a requirement only for the case where you use the session_id. https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:192: const std::string& unique_id, On 2016/11/10 14:59:52, Max Morin Chromium wrote: > On 2016/11/03 10:07:22, o1ka wrote: > > we need to name this appropriately (see my comment above). > > I changed this to translated_device_id. Note that in the original code, |unique_id| was used for the original (unstranslated) ID. https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:16: media::AudioParameters MaybeFixAudioParameters( Maybe I missed another review comment, but why force a copy in the case where it's unnecessary? https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:48: // will be translated, i.e. not hashed. remove "i.e. not hashed". "translated" is precise and clear. the "not hashed" part adds confusion because the translated ID is actually hashed. https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:105: // works with raw ids. Does the audio manager not work with default and communications IDs? Or by "the audio manager" you mean this fake audio manager? Please clarify in the comments. https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:276: // Index 0 is default, so use 1. Shouldn't you have a check or expectation that the size is > 0?
On 2016/11/10 15:26:05, Guido Urdaneta wrote: > Will check the tests later, although they look comprehensive at first glance. > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > File content/browser/renderer_host/media/audio_output_authorization_handler.cc > (right): > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: > device_info.device_id)); > On 2016/11/10 14:59:52, Max Morin Chromium wrote: > > On 2016/11/03 10:11:52, Guido Urdaneta wrote: > > > On 2016/11/03 10:07:22, o1ka wrote: > > > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here > instead > > of > > > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug > that > > > we > > > > missed before? (this is inherited from the original code) > > > > > > > > Overall, I find all that device id naming quite misleading (I know it's > > > > inherited from the original code) > > > > > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > > > > > We should use device_id instead of device_info.device-id. Nice catch! > > > Maybe we need a unit test to verify this? > > > > ARH actually needs the translated id. I'll make the ARH hash the id again > before > > sending it to the renderer. Test is CreateUnifiedStream in > > audio_renderer_host_unittest.cc, assertion is on line 194. > > Actually you can provide the empty string, as in the code you are replacing. > The translated ID is a requirement only for the case where you use the > session_id. > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > content/browser/renderer_host/media/audio_output_authorization_handler.cc:192: > const std::string& unique_id, > On 2016/11/10 14:59:52, Max Morin Chromium wrote: > > On 2016/11/03 10:07:22, o1ka wrote: > > > we need to name this appropriately (see my comment above). > > > > I changed this to translated_device_id. > > Note that in the original code, |unique_id| was used for the original > (unstranslated) ID. > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > File content/browser/renderer_host/media/audio_output_authorization_handler.cc > (right): > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > content/browser/renderer_host/media/audio_output_authorization_handler.cc:16: > media::AudioParameters MaybeFixAudioParameters( > Maybe I missed another review comment, but why force a copy in the case where > it's unnecessary? > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > File content/browser/renderer_host/media/audio_output_authorization_handler.h > (right): > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > content/browser/renderer_host/media/audio_output_authorization_handler.h:48: // > will be translated, i.e. not hashed. > remove "i.e. not hashed". > > "translated" is precise and clear. the "not hashed" part adds confusion because > the translated ID is actually hashed. > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > File content/browser/renderer_host/media/audio_renderer_host_unittest.cc > (right): > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > content/browser/renderer_host/media/audio_renderer_host_unittest.cc:105: // > works with raw ids. > Does the audio manager not work with default and communications IDs? > Or by "the audio manager" you mean this fake audio manager? > Please clarify in the comments. > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > content/browser/renderer_host/media/audio_renderer_host_unittest.cc:276: // > Index 0 is default, so use 1. > Shouldn't you have a check or expectation that the size is > 0? I'm not sure we're on the same page here. An id like "alsa_output.pci-0000_04_00.1.hdmi-stereo" or "default" is what I call "raw" or "translated" (if it came from a hash and we translated it back). The hashed name is some special name like "default" or a 64-character string of hex digits, and is unfortunately usually simply referred to as a "device_id" or "unique_id". Does this differ from the usual way to use these words?
On 2016/11/10 15:47:27, Max Morin Chromium wrote: > On 2016/11/10 15:26:05, Guido Urdaneta wrote: > > Will check the tests later, although they look comprehensive at first glance. > > > > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > > File content/browser/renderer_host/media/audio_output_authorization_handler.cc > > (right): > > > > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > > content/browser/renderer_host/media/audio_output_authorization_handler.cc:181: > > device_info.device_id)); > > On 2016/11/10 14:59:52, Max Morin Chromium wrote: > > > On 2016/11/03 10:11:52, Guido Urdaneta wrote: > > > > On 2016/11/03 10:07:22, o1ka wrote: > > > > > +guidou@ - I'm a bit confused: shouldn't we return |device_id| here > > instead > > > of > > > > > raw id, which |device_info.device_id| is? Suspiciously looks like a bug > > that > > > > we > > > > > missed before? (this is inherited from the original code) > > > > > > > > > > Overall, I find all that device id naming quite misleading (I know it's > > > > > inherited from the original code) > > > > > > > > > > What is unique_id we expect to receive in DeviceParametersReceived? > > > > > > > > We should use device_id instead of device_info.device-id. Nice catch! > > > > Maybe we need a unit test to verify this? > > > > > > ARH actually needs the translated id. I'll make the ARH hash the id again > > before > > > sending it to the renderer. Test is CreateUnifiedStream in > > > audio_renderer_host_unittest.cc, assertion is on line 194. > > > > Actually you can provide the empty string, as in the code you are replacing. > > The translated ID is a requirement only for the case where you use the > > session_id. > > > > > https://codereview.chromium.org/2424163004/diff/140001/content/browser/render... > > content/browser/renderer_host/media/audio_output_authorization_handler.cc:192: > > const std::string& unique_id, > > On 2016/11/10 14:59:52, Max Morin Chromium wrote: > > > On 2016/11/03 10:07:22, o1ka wrote: > > > > we need to name this appropriately (see my comment above). > > > > > > I changed this to translated_device_id. > > > > Note that in the original code, |unique_id| was used for the original > > (unstranslated) ID. > > > > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > > File content/browser/renderer_host/media/audio_output_authorization_handler.cc > > (right): > > > > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > > content/browser/renderer_host/media/audio_output_authorization_handler.cc:16: > > media::AudioParameters MaybeFixAudioParameters( > > Maybe I missed another review comment, but why force a copy in the case where > > it's unnecessary? > > > > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > > File content/browser/renderer_host/media/audio_output_authorization_handler.h > > (right): > > > > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > > content/browser/renderer_host/media/audio_output_authorization_handler.h:48: > // > > will be translated, i.e. not hashed. > > remove "i.e. not hashed". > > > > "translated" is precise and clear. the "not hashed" part adds confusion > because > > the translated ID is actually hashed. > > > > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > > File content/browser/renderer_host/media/audio_renderer_host_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > > content/browser/renderer_host/media/audio_renderer_host_unittest.cc:105: // > > works with raw ids. > > Does the audio manager not work with default and communications IDs? > > Or by "the audio manager" you mean this fake audio manager? > > Please clarify in the comments. > > > > > https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... > > content/browser/renderer_host/media/audio_renderer_host_unittest.cc:276: // > > Index 0 is default, so use 1. > > Shouldn't you have a check or expectation that the size is > 0? > > I'm not sure we're on the same page here. An id like > "alsa_output.pci-0000_04_00.1.hdmi-stereo" or "default" is what I call "raw" or > "translated" (if it came from a hash and we translated it back). The hashed name > is some special name like "default" or a 64-character string of hex digits, and > is unfortunately usually simply referred to as a "device_id" or "unique_id". > Does this differ from the usual way to use these words? Sorry, you're right. Nevertheless, in other places "translated" means the opposite (translated from raw to hashed), so maybe it's better to use the terms raw and hashed to avoid further confusion. IIRC, |unique_id| is normally used to refer to a raw device name, though. Not a very good name, because both raw and hashed IDs are unique.
https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:16: media::AudioParameters MaybeFixAudioParameters( On 2016/11/10 15:26:04, Guido Urdaneta wrote: > Maybe I missed another review comment, but why force a copy in the case where > it's unnecessary? Updated the code so that this function is only called if parameters are invalid. https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:48: // will be translated, i.e. not hashed. On 2016/11/10 15:26:05, Guido Urdaneta wrote: > remove "i.e. not hashed". > > "translated" is precise and clear. the "not hashed" part adds confusion because > the translated ID is actually hashed. It is translated from hashed to non-hashed. The ARH needs the raw id for opening the audio stream, and hashes it before sending it to the renderer. It also only sends it to the renderer in case session_id is used since the renderer depends on this behavior (this logic is in ARH). https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:276: // Index 0 is default, so use 1. On 2016/11/10 15:26:05, Guido Urdaneta wrote: > Shouldn't you have a check or expectation that the size is > 0? Done (size > 1).
https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:89: // output nit: comment formating https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:109: LOG(WARNING) << "session id used for device selection " add a comment that the output will go to the default device? https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:124: if (media::AudioDeviceDescription::IsDefaultDevice(device_id)) { first check for default device and return after AccessChecked(), and then l.116 and than l.129? https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:142: bool have_access) const { has_access? https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:192: cb.Run(media::OUTPUT_DEVICE_STATUS_ERROR_NOT_FOUND, guidou@ What do you think of having LOG() messages for all authorization failures? Won't it help debugging? https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:33: base::Callback<void(media::OutputDeviceStatus status, Explain parameters meaning? https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:68: // TranslateDeviceID: Could you make it per-function and explain parameter meaning as well? It's still not clear which id this function takes. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Awesome that you've done all that testing! https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:72: audio_thread_ = base::MakeUnique<base::Thread>("AudioThread"); make audio_thread_ same as UI thread on Mac? https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:95: // DestructionObserver, so we delete the threads explicitly here. can you do it by just declaring them in the correct order? (and adding the comment on that :)) https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:419: if (should_send_id) { What if we hit audio_output_authorization_handler.cc:109?
PTAL https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:89: // output On 2016/11/14 13:24:04, o1ka wrote: > nit: comment formating Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:109: LOG(WARNING) << "session id used for device selection " On 2016/11/14 13:24:04, o1ka wrote: > add a comment that the output will go to the default device? Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:124: if (media::AudioDeviceDescription::IsDefaultDevice(device_id)) { On 2016/11/14 13:24:04, o1ka wrote: > first check for default device and return after AccessChecked(), and then l.116 > and than l.129? Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:142: bool have_access) const { On 2016/11/14 13:24:04, o1ka wrote: > has_access? Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:33: base::Callback<void(media::OutputDeviceStatus status, On 2016/11/14 13:24:04, o1ka wrote: > Explain parameters meaning? Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:68: // TranslateDeviceID: On 2016/11/14 13:24:04, o1ka wrote: > Could you make it per-function and explain parameter meaning as well? It's still > not clear which id this function takes. Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/14 13:24:04, o1ka wrote: > Awesome that you've done all that testing! :) https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:72: audio_thread_ = base::MakeUnique<base::Thread>("AudioThread"); On 2016/11/14 13:24:04, o1ka wrote: > make audio_thread_ same as UI thread on Mac? Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:95: // DestructionObserver, so we delete the threads explicitly here. On 2016/11/14 13:24:04, o1ka wrote: > can you do it by just declaring them in the correct order? (and adding the > comment on that :)) Done. https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:419: if (should_send_id) { On 2016/11/14 13:24:04, o1ka wrote: > What if we hit audio_output_authorization_handler.cc:109? Fixed. I figured it was always supposed to return the device id when a device was requested with session id, but looking at the code this is only done when a device is actually found.
https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:16: media::AudioParameters MaybeFixAudioParameters( If you are calling this function only when parameters need to be fixed (to avoid copying most of the time), then rename to FixAudioParameters. Maybe add a DCHECK(!IsValid()) to make sure you don't call this unnecessarily. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:69: // weak_factory is not thread safe. Make sure it's destructed on the weak_factory->|weak_factory_|. Apply to other comments. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:111: LOG(WARNING) << "Session id used for device selection but no device info" I don't think this is a warning situation. It would be perfectly normal for this to happen. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:122: // Ignore check for default device, which is always authorized. Remove the second part of the comment. Maybe move it to above the previous if. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:204: const std::string& translated_device_id, translate_device_id -> raw_device_id https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:33: // variable should_send_id, in which case the renderer expects to get the id should_send_id -> |should_send_id|. Also, why is this necessary? The renderer always knows if it needs to read the device_id coming in the reply or not. |translated_device_id| is not a good name since it could be interpreted as translated to raw or translated to hashed (as happened to me in a previous round). Since this is intended to be raw, perhaps it's better to use |raw_device_id|. Finally, wouldn't it be more consistent for this class to receive and return hashed IDs instead of receiving hashed IDs and returning raw IDs? It would provide a better path for a mojo migration since this callback would be the callback sent by the mojo client. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:60: void OverridePermissionsForTesting(bool override_value); Document what |override_value| means. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:68: // RequestDeviceAuthorization will check if session_id should be used. If yes, session_id -> |session_id| I think such detailed comments for the private methods are overkill and will be a maintenance burden as the implementation changes over time. I would prefer shorter comments that explain what the methods do (or why) or no comment at all, than this kind of detailed comment saying how the methods do what they do. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:93: // renderer expects to get the chosen id back, which is the case if a device The original contract was to let the renderer know which output device was going to be used, not if a match was found. Therefore, the empty string (default device) was sent when no device was found. For the case when the renderer already knows the device ID, the empty string was sent as a don't-care value.
https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:16: media::AudioParameters MaybeFixAudioParameters( On 2016/11/16 12:40:18, Guido Urdaneta wrote: > If you are calling this function only when parameters need to be fixed (to avoid > copying most of the time), then rename to FixAudioParameters. > Maybe add a DCHECK(!IsValid()) to make sure you don't call this unnecessarily. Well, it's not guaranteed that it will fix the audio parameters, so I'm future-proofing against further review comments by naming it TryToFixAudioParameters. The only time it actually fixes it is when the parameters are good except that over 32 channels are specified, so it's probably very rare for it to fix the parameters :). DCHECK added. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:69: // weak_factory is not thread safe. Make sure it's destructed on the On 2016/11/16 12:40:18, Guido Urdaneta wrote: > weak_factory->|weak_factory_|. Apply to other comments. Done. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:111: LOG(WARNING) << "Session id used for device selection but no device info" On 2016/11/16 12:40:18, Guido Urdaneta wrote: > I don't think this is a warning situation. It would be perfectly normal for this > to happen. I made it into a comment instead. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:122: // Ignore check for default device, which is always authorized. On 2016/11/16 12:40:18, Guido Urdaneta wrote: > Remove the second part of the comment. Maybe move it to above the previous if. Done. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:204: const std::string& translated_device_id, On 2016/11/16 12:40:19, Guido Urdaneta wrote: > translate_device_id -> raw_device_id Done. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:33: // variable should_send_id, in which case the renderer expects to get the id On 2016/11/16 12:40:19, Guido Urdaneta wrote: > should_send_id -> |should_send_id|. Also, why is this necessary? > The renderer always knows if it needs to read the device_id coming in the reply > or not. > > |translated_device_id| is not a good name since it could be interpreted as > translated to raw or translated to hashed (as happened to me in a previous > round). Since this is intended to be raw, perhaps it's better to use > |raw_device_id|. > > Finally, wouldn't it be more consistent for this class to receive and return > hashed IDs instead of receiving hashed IDs and returning raw IDs? > It would provide a better path for a mojo migration since this callback would be > the callback sent by the mojo client. The ARH (and mojo replacement) needs to know if it should send the id to the renderer. This is what |should_send_id| indicates. They also need the raw id for the stream, since the raw id is the one that the audio manager expects. The current plan is not to just send the callback from the renderer straight here, there will be a class in-between (~taking the place of AudioRendererHost). https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:60: void OverridePermissionsForTesting(bool override_value); On 2016/11/16 12:40:19, Guido Urdaneta wrote: > Document what |override_value| means. Done. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:68: // RequestDeviceAuthorization will check if session_id should be used. If yes, On 2016/11/16 12:40:19, Guido Urdaneta wrote: > session_id -> |session_id| > > I think such detailed comments for the private methods are overkill and will be > a maintenance burden as the implementation changes over time. > I would prefer shorter comments that explain what the methods do (or why) or no > comment at all, than this kind of detailed comment saying how the methods do > what they do. I don't disagree, but I'm not sure how I would find a solution that both you and Olga find acceptable, since she wanted more comments here. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:93: // renderer expects to get the chosen id back, which is the case if a device On 2016/11/16 12:40:19, Guido Urdaneta wrote: > The original contract was to let the renderer know which output device was going > to be used, not if a match was found. Therefore, the empty string (default > device) was sent when no device was found. > For the case when the renderer already knows the device ID, the empty string was > sent as a don't-care value. Yes, I preserve this behavior by providing the AudioRendererHost with a bool indicating if this is a request for which a device was found for a specific session id.
lgtm, but consider removing details from comments for the private methods. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:33: // variable should_send_id, in which case the renderer expects to get the id On 2016/11/16 14:12:19, Max Morin Chromium wrote: > On 2016/11/16 12:40:19, Guido Urdaneta wrote: > > should_send_id -> |should_send_id|. Also, why is this necessary? > > The renderer always knows if it needs to read the device_id coming in the > reply > > or not. > > > > |translated_device_id| is not a good name since it could be interpreted as > > translated to raw or translated to hashed (as happened to me in a previous > > round). Since this is intended to be raw, perhaps it's better to use > > |raw_device_id|. > > > > Finally, wouldn't it be more consistent for this class to receive and return > > hashed IDs instead of receiving hashed IDs and returning raw IDs? > > It would provide a better path for a mojo migration since this callback would > be > > the callback sent by the mojo client. > > The ARH (and mojo replacement) needs to know if it should send the id to the > renderer. This is what |should_send_id| indicates. They also need the raw id for > the stream, since the raw id is the one that the audio manager expects. The > current plan is not to just send the callback from the renderer straight here, > there will be a class in-between (~taking the place of AudioRendererHost). I see. You can't send an empty string for some of the cases where |should_send_id| is false because you need the ID to create the streams in ARH. https://codereview.chromium.org/2424163004/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:68: // RequestDeviceAuthorization will check if session_id should be used. If yes, On 2016/11/16 14:12:19, Max Morin Chromium wrote: > On 2016/11/16 12:40:19, Guido Urdaneta wrote: > > session_id -> |session_id| > > > > I think such detailed comments for the private methods are overkill and will > be > > a maintenance burden as the implementation changes over time. > > I would prefer shorter comments that explain what the methods do (or why) or > no > > comment at all, than this kind of detailed comment saying how the methods do > > what they do. > > I don't disagree, but I'm not sure how I would find a solution that both you and > Olga find acceptable, since she wanted more comments here. I'm OK with comments that clarify what the method does if the name is not sufficiently obvious. For example, I would leave the first sentence in the comment for TranslateDeviceID, but remove the second. Details about the internal implementation details are never as precise as the code and can easily become obsolete.
lgtm Good work! Let's hope unit tests caught all the bugs ;) https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:65: DCHECK(permission_checker_); What is the point for this check? https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:81: bool have_access) const; has_access? https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:84: // device id. After that, it calls GetDeviceParameters with the result. Agree with Guido: we don;t need a description of what's happening in the function. Juts clarify what device_id it expects: raw or hashed. https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:158: const std::string& unique_id); raw_id? https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:257: // ScopedAudioManagerPtr. It will immediately destruct, cleaning up the well, not "immediately", but when scoped ptr goes out of scope. Why not to store it in the correct scoped pointer for the beginning?
Thanks everyone! Dale: Do you want to take another look at this before I commit it? https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.cc:65: DCHECK(permission_checker_); On 2016/11/16 15:11:28, o1ka wrote: > What is the point for this check? Not really necessary. Removed. https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:81: bool have_access) const; On 2016/11/16 15:11:28, o1ka wrote: > has_access? Fixed. I'm surprised the compiler doesn't warn about the difference in h and cc file. https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler.h:84: // device id. After that, it calls GetDeviceParameters with the result. On 2016/11/16 15:11:28, o1ka wrote: > Agree with Guido: we don;t need a description of what's happening in the > function. Juts clarify what device_id it expects: raw or hashed. Done. https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:158: const std::string& unique_id); On 2016/11/16 15:11:28, o1ka wrote: > raw_id? Done. https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:257: // ScopedAudioManagerPtr. It will immediately destruct, cleaning up the On 2016/11/16 15:11:28, o1ka wrote: > well, not "immediately", but when scoped ptr goes out of scope. > Why not to store it in the correct scoped pointer for the beginning? The ScoperAudioManagerPtr is temporary (not assigned to a variable), so it will be immediately destructed (after being constructed, in this case). It is not stored in a ScopedAudioManagerPtr since I'd have to cast it to a FakeAudioManagerWithAssociations* to call CreateDeviceAssociation on it in that case. It is possible to do that, and it would also be possible to create a ScopedFakeAudioManagerWithAssociationsPtr to store it in. If this was production code, I would use ScopedFakeAudioManagerWithAssociationsPtr but I think that using fewer lines is better in this case.
https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2424163004/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:257: // ScopedAudioManagerPtr. It will immediately destruct, cleaning up the On 2016/11/16 15:54:36, Max Morin Chromium wrote: > On 2016/11/16 15:11:28, o1ka wrote: > > well, not "immediately", but when scoped ptr goes out of scope. > > Why not to store it in the correct scoped pointer for the beginning? > > The ScoperAudioManagerPtr is temporary (not assigned to a variable), so it will > be immediately destructed (after being constructed, in this case). It is not > stored in a ScopedAudioManagerPtr since I'd have to cast it to a > FakeAudioManagerWithAssociations* to call CreateDeviceAssociation on it in that > case. It is possible to do that, and it would also be possible to create a > ScopedFakeAudioManagerWithAssociationsPtr to store it in. If this was production > code, I would use ScopedFakeAudioManagerWithAssociationsPtr but I think that > using fewer lines is better in this case. Right, I missed that. Thanks for clarification!
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Dale: Ping! Do you want to have a final look at this before I commit it?
still lgtm % q. https://codereview.chromium.org/2424163004/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2424163004/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:402: url::Origin security_origin, Doesn't seem like a movable type?
The CQ bit was checked by maxmorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, holte@chromium.org, guidou@chromium.org, olka@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2424163004/#ps300001 (title: "const&")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/17 20:10:37, DaleCurtis wrote: > still lgtm % q. > > https://codereview.chromium.org/2424163004/diff/280001/content/browser/render... > File content/browser/renderer_host/media/audio_renderer_host.cc (right): > > https://codereview.chromium.org/2424163004/diff/280001/content/browser/render... > content/browser/renderer_host/media/audio_renderer_host.cc:402: url::Origin > security_origin, > Doesn't seem like a movable type? Thanks. Well spotted.
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Factor out hadndling of the authorization IPC from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations. BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Factor out hadndling of the authorization IPC from AudioRendererHost. This is part of an effort to make AudioRendererHost small enough to easily replace with a class using mojo. The pending/completed authorizations are still stored in ARH, since the mojo implementation won't need to store pending authorizations. BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/b08e842c3fab03521e0c1fb954c39453c96edeab Cr-Commit-Position: refs/heads/master@{#433160} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/b08e842c3fab03521e0c1fb954c39453c96edeab Cr-Commit-Position: refs/heads/master@{#433160} |
