Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1369723003:
Do not hash default and communications media device ID (Closed)
|
Created:
5 years, 2 months ago by Guido Urdaneta Modified:
5 years, 2 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not hash default and communications media device ID
BUG=535980
Committed: https://crrev.com/9a60f773372afb2625253e8d7632c7036b33086b
Cr-Commit-Position: refs/heads/master@{#351132}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix nit #Patch Set 3 : Move hash implementation to MediaStreamManager #Patch Set 4 : Make MediaStreamManager use its own implementations of device ID methods #Patch Set 5 : Add unit test #Patch Set 6 : Fix format #
Total comments: 2
Patch Set 7 : Tommi's comments #
Total comments: 5
Messages
Total messages: 31 (9 generated)
guidou@chromium.org changed reviewers: + tommi@chromium.org
Hi, PTAL
guidou@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in content/public
https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... File content/public/browser/media_device_id.cc (right): https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... content/public/browser/media_device_id.cc:17: const char kDefaultDeviceId[] = "default"; Can we use the constants from media/ instead? https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... content/public/browser/media_device_id.cc:21: return (source_id == kDefaultDeviceId) || nit: () not necessary
charlieloveeric3@gmail.com changed reviewers: + charlieloveeric3@gmail.com - jochen@chromium.org, tommi@chromium.org
Learning Mercurial in Workflows prepare Mercurial As a first step, you should teach your Mercurial name. So you open the file ~ / .hgrc (or Mercurial.ini in your home directory for Windows) with a text editor and add the ui section (user interaction) with your user name: if You are systems that project members checked or validated changes in the source code, or by using command-line tools to upload files to the project of the "Downloads" tab. Password is: kt2qK3qV5PX4 http://git-scm.com/downloads http://mercurial.selenic.com/downloads
guidou@chromium.org changed reviewers: + jochen@chromium.org, tommi@chromium.org - charlieloveeric3@gmail.com
https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... File content/public/browser/media_device_id.cc (right): https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... content/public/browser/media_device_id.cc:17: const char kDefaultDeviceId[] = "default"; On 2015/09/26 09:27:23, tommi wrote: > Can we use the constants from media/ instead? > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... I originally tried that, but, unfortunaetly, it is forbidden to #include "media/" in content/public. Maybe we should also expose these public/content constants and check that they match the media constants somewhere? https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... content/public/browser/media_device_id.cc:21: return (source_id == kDefaultDeviceId) || On 2015/09/26 09:27:24, tommi wrote: > nit: () not necessary Done.
https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... File content/public/browser/media_device_id.cc (right): https://codereview.chromium.org/1369723003/diff/1/content/public/browser/medi... content/public/browser/media_device_id.cc:17: const char kDefaultDeviceId[] = "default"; On 2015/09/26 09:48:58, Guido Urdaneta wrote: > On 2015/09/26 09:27:23, tommi wrote: > > Can we use the constants from media/ instead? > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... > > I originally tried that, but, unfortunaetly, it is forbidden to #include > "media/" in content/public. > > Maybe we should also expose these public/content constants and check that they > match the media constants somewhere? Ah right. I didn't realize we're in public/. Hmm... it's unfortunate that content/ can't access media/ for the benefit of content/. I think the actual implementation of these methods needs to be moved into content (e.g. under content/browser/media)
What about this approach? We still need the wrappers in content/public because extensions use these and cannot include from content/browser/renderer_host/media.
great. this works :) lgtm.
henrik.kjellander@gmail.com changed reviewers: + henrik.kjellander@gmail.com - tommi@chromium.org
Testing commenting and removing tommi@ as reviewer (see https://crbug.com/536695 for details).
On 2015/09/28 09:01:53, henrik.kjellander wrote: > Testing commenting and removing tommi@ as reviewer (see https://crbug.com/536695 > for details). Apparently nothing happens even if I try to remove tommi@ in the dialog presented when you publish comments.
jochen@chromium.org changed reviewers: + tommi@chromium.org
what about tests?
Added tests.
lgtm https://codereview.chromium.org/1369723003/diff/90001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_manager_unittest.cc (right): https://codereview.chromium.org/1369723003/diff/90001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager_unittest.cc:198: std::string unique_default_id(media::AudioManagerBase::kDefaultDeviceId); nit: const? (same for unique_communications_id, unique_other_id, hashed_default_id and hashed_other_id)
https://codereview.chromium.org/1369723003/diff/90001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_manager_unittest.cc (right): https://codereview.chromium.org/1369723003/diff/90001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager_unittest.cc:198: std::string unique_default_id(media::AudioManagerBase::kDefaultDeviceId); On 2015/09/28 14:30:50, tommi wrote: > nit: const? (same for unique_communications_id, unique_other_id, > hashed_default_id and hashed_other_id) Done.
lgtm
The CQ bit was checked by tommi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1369723003/#ps110001 (title: "Tommi's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1369723003/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1369723003/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9a60f773372afb2625253e8d7632c7036b33086b Cr-Commit-Position: refs/heads/master@{#351132}
Message was sent while issue was closed.
ajm@chromium.org changed reviewers: + ajm@chromium.org
Message was sent while issue was closed.
Drive-by question. https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:87: device_id == media::AudioManagerBase::kCommunicationsDeviceId) { Does it matter that ChromeOS is not using these IDs when a mic array is available? See line 42-43: https://codereview.chromium.org/1362093002/diff/140001/media/audio/cras/audio...
Message was sent while issue was closed.
https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:87: device_id == media::AudioManagerBase::kCommunicationsDeviceId) { On 2015/09/28 22:47:18, ajm wrote: > Does it matter that ChromeOS is not using these IDs when a mic array is > available? See line 42-43: > https://codereview.chromium.org/1362093002/diff/140001/media/audio/cras/audio... No, it does not matter. This is a check for hashed device IDs coming from the renderer. Those special IDs in ChromeOS will come hashed as an hex string from the renderer just like nondefault IDs.
Message was sent while issue was closed.
https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:87: device_id == media::AudioManagerBase::kCommunicationsDeviceId) { On 2015/09/28 22:47:18, ajm wrote: > Does it matter that ChromeOS is not using these IDs when a mic array is > available? See line 42-43: > https://codereview.chromium.org/1362093002/diff/140001/media/audio/cras/audio... Is it using any other "known-and-not-translated" ID? If it's only using hashed IDs, then I don't think it makes a difference.
Message was sent while issue was closed.
https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:87: device_id == media::AudioManagerBase::kCommunicationsDeviceId) { On 2015/09/29 10:36:12, Guido Urdaneta wrote: > On 2015/09/28 22:47:18, ajm wrote: > > Does it matter that ChromeOS is not using these IDs when a mic array is > > available? See line 42-43: > > > https://codereview.chromium.org/1362093002/diff/140001/media/audio/cras/audio... > > No, it does not matter. This is a check for hashed device IDs coming from the > renderer. > Those special IDs in ChromeOS will come hashed as an hex string from the > renderer just like nondefault IDs. Great, thanks. https://codereview.chromium.org/1369723003/diff/110001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:87: device_id == media::AudioManagerBase::kCommunicationsDeviceId) { On 2015/09/29 11:59:51, tommi wrote: > On 2015/09/28 22:47:18, ajm wrote: > > Does it matter that ChromeOS is not using these IDs when a mic array is > > available? See line 42-43: > > > https://codereview.chromium.org/1362093002/diff/140001/media/audio/cras/audio... > > Is it using any other "known-and-not-translated" ID? No. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
