Implement a salt for MediaSource IDs that can be cleared by a user.

This cl implement a salt that is being used for obfuscating device ids before a device id is sent to the render process. The purpose of the salt is to make sure that the device ids as seen by the render process are invalidated if cookies are cleared.

BUG=269139
TEST= open http://src.chromium.org/chrome/trunk/src/chrome/test/data/webrtc/manual/peerconnection.html using http (not https) note the source ids in the Audio source and Video source dropdown. Restart chrome and notice that the source ids in the dropdown are unchanged. Clear the cookie cache and press the Refresh devices button and notice that the source ids have changed.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238103

[perkj_chrome, 2013-10-31 15:31:47]

Hej Joi.
Here is my first attempt- can you take a look and advice? 
I have not even build any unit tests- only tested with the test file mentioned
in the cl description.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:287:
if (content::DoesMediaDeviceIDMatchHMAC(NULL,
help!

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:80: base::Unretained(this),
media_device_id_salt_.GetValue()));
Is Unretained OK here or should MediaDeviceSalt be ref counted?

https://codereview.chromium.org/54863002/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/54863002/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:1042: 'browser/media/desktop_media_picker_model.h',
fix

[Jói, 2013-10-31 16:14:47]

Very cool, almost there. Just a few comments below.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client.cc:2384: std::string
ChromeContentBrowserClient::GetDeviceIdSalt(
Should this name match the other, i.e. GetMediaDeviceIdSalt?

Also I think in the Content API the more common approach is to capitalize
abbreviations such as URL and ID, so this should then be GetMediaDeviceIDSalt.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:287:
if (content::DoesMediaDeviceIDMatchHMAC(NULL,
On 2013/10/31 15:31:48, perkj wrote:
> help!

This object inherits from ExtensionFunction, which provides a GetProfile()
method.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:17: std::string CreateSalt() {
I would be surprised if we don't already have a function somewhere to create a
random string. Maybe in //crypto?

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:69: void
MediaDeviceIDSalt::Reset(PrefService* pref_service) {
I was a bit surprised this was static, but I see now how you're doing it. Seems
fine by me; the other approach would have been to have Reset be a member
variable, and have BrowsingDataRemover look up the MediaDeviceIDSalt object on
which to call Reset from the Profile for which it is clearing data. That way,
you wouldn't have to have a StringPrefMember, you would just call
pref_service->ClearPref and pref_service->SetPref in your Reset member when
setting the simple string member salt_.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:80: base::Unretained(this),
media_device_id_salt_.GetValue()));
On 2013/10/31 15:31:48, perkj wrote:
> Is Unretained OK here or should MediaDeviceSalt be ref counted?

Good question. I think it may be OK since probably any message posted from UI to
IO to destroy the IO side of ProfileIOData wouldn't have been posted by this
point, so your message would be processed first, so the IO side of ProfileIOData
would still be guaranteed to exist. I'm not 100% sure though, would need to look
through the code, and this is a guarantee that won't necessarily hold when code
changes.

I'm starting to think we should just use a lock to protect access to the salt_
member and access it directly from either UI or IO threads. There will almost
never be contention for accessing it (the only time it might possibly be
accessed around the same time by multiple threads is when you're resetting it)
so the lock would be very close to free.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_io_data.cc:652: return
media_device_id_salt_->GetSalt();
If you name your method using Linux hacker style (media_device_id_salt) then it
is supposed to be a simple accessor for the member named similarly
(media_device_id_salt_).

I would suggest actually returning the MediaDeviceIDSalt object here, rather
than calling GetSalt() on behalf of the caller.

Alternatively, leave as is and rename to GetMediaDeviceIDSalt.

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:17:
BrowserContext* browser_context,
Why not simply take a ResourceContext? No need for this object to know about
BrowserContext.

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:854: /*bool
MediaStreamManager::FindExistingRequestedDeviceInfo(
Did you mean to leave this commented-out block of code?

https://codereview.chromium.org/54863002/diff/1/content/public/browser/conten...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/54863002/diff/1/content/public/browser/conten...
content/public/browser/content_browser_client.h:529: // the cache is cleared.
This is called on the IO thread.
Is clearing the cache the right trigger? This is more or less equivalent to a
persistent cookie so I would have thought "when cookies are cleared".

[perkj_chrome, 2013-10-31 17:05:44]

Thanks Joi- I will need to continue tomorrow with getting the existing unittests
to compile and work.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client.cc:2384: std::string
ChromeContentBrowserClient::GetDeviceIdSalt(
On 2013/10/31 16:14:47, Jói wrote:
> Should this name match the other, i.e. GetMediaDeviceIdSalt?
> 
> Also I think in the Content API the more common approach is to capitalize
> abbreviations such as URL and ID, so this should then be GetMediaDeviceIDSalt.

Done.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:287:
if (content::DoesMediaDeviceIDMatchHMAC(NULL,
On 2013/10/31 16:14:47, Jói wrote:
> On 2013/10/31 15:31:48, perkj wrote:
> > help!
> 
> This object inherits from ExtensionFunction, which provides a GetProfile()
> method.

Done.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:17: std::string CreateSalt() {
On 2013/10/31 16:14:47, Jói wrote:
> I would be surprised if we don't already have a function somewhere to create a
> random string. Maybe in //crypto?

Yes- but it turned out that crypto::RandBytes in random.h returns something that
can not be stored in UTF8. |media_device_id_salt_| got very sad then.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:69: void
MediaDeviceIDSalt::Reset(PrefService* pref_service) {
On 2013/10/31 16:14:47, Jói wrote:
> I was a bit surprised this was static, but I see now how you're doing it.
Seems
> fine by me; the other approach would have been to have Reset be a member
> variable, and have BrowsingDataRemover look up the MediaDeviceIDSalt object on
> which to call Reset from the Profile for which it is clearing data. That way,
> you wouldn't have to have a StringPrefMember, you would just call
> pref_service->ClearPref and pref_service->SetPref in your Reset member when
> setting the simple string member salt_.
Sure- that would simplify things even more.
How would I find the MediaDeviceIDSalt object? Can I get to the ProfielIOData
object from a Profile* ?

https://codereview.chromium.org/54863002/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_io_data.cc:652: return
media_device_id_salt_->GetSalt();
On 2013/10/31 16:14:47, Jói wrote:
> If you name your method using Linux hacker style (media_device_id_salt) then
it
> is supposed to be a simple accessor for the member named similarly
> (media_device_id_salt_).
> 
> I would suggest actually returning the MediaDeviceIDSalt object here, rather
> than calling GetSalt() on behalf of the caller.
> 
> Alternatively, leave as is and rename to GetMediaDeviceIDSalt.

Done.

https://codereview.chromium.org/54863002/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/54863002/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:1042: 'browser/media/desktop_media_picker_model.h',
On 2013/10/31 15:31:48, perkj wrote:
> fix

Done.

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:17:
BrowserContext* browser_context,
On 2013/10/31 16:14:47, Jói wrote:
> Why not simply take a ResourceContext? No need for this object to know about
> BrowserContext.

Done.

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/54863002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:854: /*bool
MediaStreamManager::FindExistingRequestedDeviceInfo(
On 2013/10/31 16:14:47, Jói wrote:
> Did you mean to leave this commented-out block of code?

eh- nope- sorry.

https://codereview.chromium.org/54863002/diff/1/content/public/browser/conten...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/54863002/diff/1/content/public/browser/conten...
content/public/browser/content_browser_client.h:529: // the cache is cleared.
This is called on the IO thread.
On 2013/10/31 16:14:47, Jói wrote:
> Is clearing the cache the right trigger? This is more or less equivalent to a
> persistent cookie so I would have thought "when cookies are cleared".

right - that is what I actually do...

[Jói, 2013-10-31 17:19:34]

Cool, took another look, see below.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:17: std::string CreateSalt() {
On 2013/10/31 17:05:44, perkj wrote:
> On 2013/10/31 16:14:47, Jói wrote:
> > I would be surprised if we don't already have a function somewhere to create
a
> > random string. Maybe in //crypto?
> 
> Yes- but it turned out that crypto::RandBytes in random.h returns something
that
> can not be stored in UTF8. |media_device_id_salt_| got very sad then.

OK.

https://codereview.chromium.org/54863002/diff/1/chrome/browser/media/media_de...
chrome/browser/media/media_device_id_salt.cc:69: void
MediaDeviceIDSalt::Reset(PrefService* pref_service) {
On 2013/10/31 17:05:44, perkj wrote:
> On 2013/10/31 16:14:47, Jói wrote:
> > I was a bit surprised this was static, but I see now how you're doing it.
> Seems
> > fine by me; the other approach would have been to have Reset be a member
> > variable, and have BrowsingDataRemover look up the MediaDeviceIDSalt object
on
> > which to call Reset from the Profile for which it is clearing data. That
way,
> > you wouldn't have to have a StringPrefMember, you would just call
> > pref_service->ClearPref and pref_service->SetPref in your Reset member when
> > setting the simple string member salt_.
> Sure- that would simplify things even more.
> How would I find the MediaDeviceIDSalt object? Can I get to the ProfielIOData
> object from a Profile* ?

ProfileIOData::FromResourceContext (and you can do profile->GetResourceContext()
since it is a BrowserContext).

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/extensions...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/extensions...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:287:
GetProfile()->GetResourceContext(), security_origin,
One param per line since they don't fit all on one.

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.cc:17: std::string CreateSalt() {
This should be tested, if nothing more than a manually-inspected test. A basic
automated test could verify that you get a different salt each time.

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.cc:73: void
MediaDeviceIDSalt::OnValueReset(const std::string& name) {
Are you sure this will trigger only when you reset the value, and not when you
change it a couple of lines down (leading to an endless loop)?

In either case this would go away if you switch to the approach we discussed
where Reset is a method instead of a static method.

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
File chrome/browser/media/media_device_id_salt.h (right):

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.h:37: // |salt_| is initialized in
ctor on UI thread but only read and changed on
This is no longer strictly true. You can say "salt_ is initialized in ctor on UI
thread and usually accessed on the IO thread, except for resetting. Access is
done while lock_ is held."

https://codereview.chromium.org/54863002/diff/90001/content/browser/media_dev...
File content/browser/media_devices_monitor.cc (right):

https://codereview.chromium.org/54863002/diff/90001/content/browser/media_dev...
content/browser/media_devices_monitor.cc:16: NULL, -1, -1, NULL, -1,
MEDIA_DEVICE_AUDIO_CAPTURE, GURL());
Does this need to be fixed or is NULL OK?

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:220: NULL);
Does this need to be fixed or is NULL OK?

[perkj_chrome, 2013-11-01 13:39:34]

How about this? 
I had to rebase to the master branch- sorry about that.

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/extensions...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/extensions...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:287:
GetProfile()->GetResourceContext(), security_origin,
On 2013/10/31 17:19:35, Jói wrote:
> One param per line since they don't fit all on one.

Done.

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.cc:17: std::string CreateSalt() {
On 2013/10/31 17:19:35, Jói wrote:
> This should be tested, if nothing more than a manually-inspected test. A basic
> automated test could verify that you get a different salt each time.

I found another way. How about using a guid? I took this new implementation from
notifications_api.cc

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.cc:73: void
MediaDeviceIDSalt::OnValueReset(const std::string& name) {
On 2013/10/31 17:19:35, Jói wrote:
> Are you sure this will trigger only when you reset the value, and not when you
> change it a couple of lines down (leading to an endless loop)?
> 
> In either case this would go away if you switch to the approach we discussed
> where Reset is a method instead of a static method.

YEs- it is only triggered once. I was afraid of the same thing but I never
looked up why this did not trigger a loop.

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
File chrome/browser/media/media_device_id_salt.h (right):

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.h:37: // |salt_| is initialized in
ctor on UI thread but only read and changed on
logic is now changed.
On 2013/10/31 17:19:35, Jói wrote:
> This is no longer strictly true. You can say "salt_ is initialized in ctor on
UI
> thread and usually accessed on the IO thread, except for resetting. Access is
> done while lock_ is held."

https://codereview.chromium.org/54863002/diff/90001/content/browser/media_dev...
File content/browser/media_devices_monitor.cc (right):

https://codereview.chromium.org/54863002/diff/90001/content/browser/media_dev...
content/browser/media_devices_monitor.cc:16: NULL, -1, -1, NULL, -1,
MEDIA_DEVICE_AUDIO_CAPTURE, GURL());
On 2013/10/31 17:19:35, Jói wrote:
> Does this need to be fixed or is NULL OK?

Null is ok. This is an ugly way of starting an enumeration of USB devices.
(don't blame me please)
This should probably be a separate method without any arguments at all. But not
in this cl.

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:220: NULL);
On 2013/10/31 17:19:35, Jói wrote:
> Does this need to be fixed or is NULL OK?

It is ok here too. This is webspeach abusing the MediaStreamManager. It
actuyally only uses it for asking for permissions and using the same ui as when
a stream is generated.
This should problably also be rewritten... but not in this cl.

[Jói, 2013-11-01 14:29:37]

Almost done.

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.cc:17: std::string CreateSalt() {
On 2013/11/01 13:39:34, perkj wrote:
> On 2013/10/31 17:19:35, Jói wrote:
> > This should be tested, if nothing more than a manually-inspected test. A
basic
> > automated test could verify that you get a different salt each time.
> 
> I found another way. How about using a guid? I took this new implementation
from
> notifications_api.cc

Yep, that seems fine since our GUIDs are actually random (the ones Windows APIs
generate are not, see
http://blogs.msdn.com/b/oldnewthing/archive/2012/05/23/10309199.aspx).

https://codereview.chromium.org/54863002/diff/90001/chrome/browser/media/medi...
chrome/browser/media/media_device_id_salt.cc:73: void
MediaDeviceIDSalt::OnValueReset(const std::string& name) {
On 2013/11/01 13:39:34, perkj wrote:
> On 2013/10/31 17:19:35, Jói wrote:
> > Are you sure this will trigger only when you reset the value, and not when
you
> > change it a couple of lines down (leading to an endless loop)?
> > 
> > In either case this would go away if you switch to the approach we discussed
> > where Reset is a method instead of a static method.
> 
> YEs- it is only triggered once. I was afraid of the same thing but I never
> looked up why this did not trigger a loop.

OK.

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:220: NULL);
On 2013/11/01 13:39:34, perkj wrote:
> On 2013/10/31 17:19:35, Jói wrote:
> > Does this need to be fixed or is NULL OK?
> 
> It is ok here too. This is webspeach abusing the MediaStreamManager. It
> actuyally only uses it for asking for permissions and using the same ui as
when
> a stream is generated.
> This should problably also be rewritten... but not in this cl.
> 

OK for both this and the other case. I wonder if you should leave a TODO in the
two locations where you're passing a NULL?

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.cc:20: return !salt.empty() ? salt :
base::RandBytesAsString(16);
Wait, why would the salt be empty?

And why not just always use RandBytesAsString?

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.cc:38:
media_device_id_salt_.MoveToThread(
Ah, cool, I had forgotten about this. Nice solution, makes this class quite
elegantly simple.

https://codereview.chromium.org/54863002/diff/350001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/54863002/diff/350001/content/public/browser/c...
content/public/browser/content_browser_client.h:528: // The salt should depend
on the current user profile and should be reset if
depend on -> "be stored in" or "be part of"

[perkj_chrome, 2013-11-01 15:49:06]

Here we go again. 
Adding tommi for review of content/media.

Joi- if you think this is close to completion - can you help me add reviewers
for chrome/ ?

Thanks

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/54863002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:220: NULL);
On 2013/11/01 14:29:37, Jói wrote:
> On 2013/11/01 13:39:34, perkj wrote:
> > On 2013/10/31 17:19:35, Jói wrote:
> > > Does this need to be fixed or is NULL OK?
> > 
> > It is ok here too. This is webspeach abusing the MediaStreamManager. It
> > actuyally only uses it for asking for permissions and using the same ui as
> when
> > a stream is generated.
> > This should problably also be rewritten... but not in this cl.
> > 
> 
> OK for both this and the other case. I wonder if you should leave a TODO in
the
> two locations where you're passing a NULL?

Done.

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.cc:20: return !salt.empty() ? salt :
base::RandBytesAsString(16);
On 2013/11/01 14:29:38, Jói wrote:
> Wait, why would the salt be empty?
> 
> And why not just always use RandBytesAsString?


/ Generate a 128-bit random GUID of the form: "%08X-%04X-%04X-%04X-%012llX".
// If GUID generation fails an empty string is returned.

RandBytesAsString turned out to also generate a string that can not be stored in
utf8.

However- I doubt that  base::GenerateGUID() really can fail- it is used heavily
in Chrome for kinds of purposes and there doesn't seem to be a check for failure
in most places. We DCHECK that when we generate our SourceIDs.

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.cc:38:
media_device_id_salt_.MoveToThread(
On 2013/11/01 14:29:38, Jói wrote:
> Ah, cool, I had forgotten about this. Nice solution, makes this class quite
> elegantly simple.

Done.

https://codereview.chromium.org/54863002/diff/350001/content/browser/renderer...
File
content/browser/renderer_host/media/device_request_message_filter_unittest.cc
(right):

https://codereview.chromium.org/54863002/diff/350001/content/browser/renderer...
content/browser/renderer_host/media/device_request_message_filter_unittest.cc:98:
EnumerateDevices(_, _,_, _, _, MEDIA_DEVICE_AUDIO_CAPTURE, _))
darn

https://codereview.chromium.org/54863002/diff/350001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/54863002/diff/350001/content/public/browser/c...
content/public/browser/content_browser_client.h:528: // The salt should depend
on the current user profile and should be reset if
On 2013/11/01 14:29:38, Jói wrote:
> depend on -> "be stored in" or "be part of"

Done.

[Jói, 2013-11-01 16:30:30]

LGTM once you fix GenerateSalt, see below.

For //chrome reviewers, I would suggest:

bauerb@ for //chrome/browser/browsing_data and //chrome/browser/prefs.

One of willchan@, mmenke@ or ajwong@ for the ProfileIOData changes.

For the rest, you either already have an OWNER or there is a * for owners of the
various files (e.g. pref_names).

Cheers,
Jói

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.cc:20: return !salt.empty() ? salt :
base::RandBytesAsString(16);
On 2013/11/01 15:49:06, perkj wrote:
> On 2013/11/01 14:29:38, Jói wrote:
> > Wait, why would the salt be empty?
> > 
> > And why not just always use RandBytesAsString?
> 
> 
> / Generate a 128-bit random GUID of the form: "%08X-%04X-%04X-%04X-%012llX".
> // If GUID generation fails an empty string is returned.
> 
> RandBytesAsString turned out to also generate a string that can not be stored
in
> utf8.
> 
> However- I doubt that  base::GenerateGUID() really can fail- it is used
heavily
> in Chrome for kinds of purposes and there doesn't seem to be a check for
failure
> in most places. We DCHECK that when we generate our SourceIDs.

Actually, I misread the GenerateGUID implementation before. There is one
specific to Windows that uses CoCreateGUID, which should not be used as a random
source for the reasons listed in the link i pasted earlier.

I suggest:

std::string salt;
base::Base64Encode(base::RandBytesAsString(16), &salt);
return salt;

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/media/med...
File chrome/browser/media/media_device_id_salt.h (right):

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.h:5: // MediaDeviceIDSalt is
responsible for creating and retrieving a salt string
Would move this to be right above the class definition (right above line 19 in
this file).

[Jói, 2013-11-01 16:58:01]

Adding OWNERS:
+bauerb for chrome/browser/{browsing_data, prefs}
+willchan for chrome/browser/profiles/profile_io_data*

Cheers,
Jói

[Bernhard Bauer, 2013-11-01 17:00:18]

browsing_data and prefs LGTM with a nit:

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:350:
MediaDeviceIDSalt::RegisterProfilePrefs(registry);
Nit: Alphabetize (MediaDevice comes before MediaStream).

[tommi (sloooow) - chröme, 2013-11-01 17:21:36]

lgtm % one question.  I'll leave it up to you to decide.

https://codereview.chromium.org/54863002/diff/400001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/54863002/diff/400001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:250: const
DeviceRequest* new_request,
is new_request past as a pointer since it can be null?  If it can never be NULL,
should we use const &?

[perkj_chrome, 2013-11-01 19:01:55]

Thanks all.
sky- can you help me with media_device_monitor.cc and  
content/browser/renderer_host/render_process_host_impl.cc changes?

/
Per

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/54863002/diff/350001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.cc:20: return !salt.empty() ? salt :
base::RandBytesAsString(16);
On 2013/11/01 16:30:31, Jói wrote:
> On 2013/11/01 15:49:06, perkj wrote:
> > On 2013/11/01 14:29:38, Jói wrote:
> > > Wait, why would the salt be empty?
> > > 
> > > And why not just always use RandBytesAsString?
> > 
> > 
> > / Generate a 128-bit random GUID of the form: "%08X-%04X-%04X-%04X-%012llX".
> > // If GUID generation fails an empty string is returned.
> > 
> > RandBytesAsString turned out to also generate a string that can not be
stored
> in
> > utf8.
> > 
> > However- I doubt that  base::GenerateGUID() really can fail- it is used
> heavily
> > in Chrome for kinds of purposes and there doesn't seem to be a check for
> failure
> > in most places. We DCHECK that when we generate our SourceIDs.
> 
> Actually, I misread the GenerateGUID implementation before. There is one
> specific to Windows that uses CoCreateGUID, which should not be used as a
random
> source for the reasons listed in the link i pasted earlier.
> 
> I suggest:
> 
> std::string salt;
> base::Base64Encode(base::RandBytesAsString(16), &salt);
> return salt;

Done.

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/media/med...
File chrome/browser/media/media_device_id_salt.h (right):

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.h:5: // MediaDeviceIDSalt is
responsible for creating and retrieving a salt string
On 2013/11/01 16:30:31, Jói wrote:
> Would move this to be right above the class definition (right above line 19 in
> this file).

Done.

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/54863002/diff/400001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:350:
MediaDeviceIDSalt::RegisterProfilePrefs(registry);
On 2013/11/01 17:00:19, Bernhard Bauer wrote:
> Nit: Alphabetize (MediaDevice comes before MediaStream).

Done.

https://codereview.chromium.org/54863002/diff/400001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/54863002/diff/400001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:250: const
DeviceRequest* new_request,
On 2013/11/01 17:21:37, tommi wrote:
> is new_request past as a pointer since it can be null?  If it can never be
NULL,
> should we use const &?

Done.

[sky, 2013-11-01 20:31:35]

https://codereview.chromium.org/54863002/diff/350002/content/browser/media_de...
File content/browser/media_devices_monitor.cc (right):

https://codereview.chromium.org/54863002/diff/350002/content/browser/media_de...
content/browser/media_devices_monitor.cc:15: // TODO(perkj): The argument list
suggests that this is the wrong design. Can
I don't know enough about the ramifications of this to sign off on this patch.
Please find another OWNER.

[perkj_chrome, 2013-11-02 07:00:30]

I clarified the cl description a bit.

I moved media_devices_monitor.cc|h to content/browser/media
I also add xians who is the original author of
content/browser/media_devices_monitor.cc. 
We are in the same team.
I moved content/browser/media_devices_monitor.cc.to
content/browser/media/media_devices_monitor.cc.

Sky - does the move make sence and can you approve the move of the file? I will
wait for xians approval or the actual code change. There are no other owners on
content/browser/ level.

Jochen can you please review the content/browsers/renderer_host changes?

[willchan no longer on Chromium, 2013-11-02 20:46:54]

chrome/browser/profiles LGTM with an ordering nit.

https://codereview.chromium.org/54863002/diff/510001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/54863002/diff/510001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:536: mutable
scoped_ptr<MediaDeviceIDSalt> media_device_id_salt_;
Move this earlier. See the comments at line 437. My guess is somewhere like 469
would be good.

[no longer working on chromium, 2013-11-04 08:58:50]

lgtm to the media_devices_monitor.cc

[jochen (gone - plz use gerrit), 2013-11-04 09:08:00]

lgtm

[perkj_chrome, 2013-11-04 11:52:50]

https://codereview.chromium.org/54863002/diff/510001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/54863002/diff/510001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:536: mutable
scoped_ptr<MediaDeviceIDSalt> media_device_id_salt_;
On 2013/11/02 20:46:55, willchan wrote:
> Move this earlier. See the comments at line 437. My guess is somewhere like
469
> would be good.

Done.

[perkj_chrome, 2013-11-04 15:27:55]

I think I am breaking my personal record when it comes to the number of
reviewers.

John, can you please help with content_browser.gypi?

Thanks

[jam, 2013-11-04 15:38:52]

lgtm with change below

https://codereview.chromium.org/54863002/diff/620001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/54863002/diff/620001/content/public/browser/c...
content/public/browser/content_browser_client.h:530: virtual std::string
GetMediaDeviceIDSalt(ResourceContext* rc);
since this method is invoked per ResourceContext, it should be moved to be on
ResourceContext

[Jói, 2013-11-04 16:53:28]

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:961:
media_device_id_salt_.reset(NULL);
I'm not sure this is correct (destroying it on the UI thread). I would suggest
moving ownership of MediaDeviceIDSalt to the ResourceContext object since that's
where it's used from. You could just add an is_incognito parameter to the
constructor of ResourceContext.

https://codereview.chromium.org/54863002/diff/720001/content/public/browser/r...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/54863002/diff/720001/content/public/browser/r...
content/public/browser/resource_context.h:59: // Returns a random salt string
that is used for creating MediaSourceIDs.
MediaSourceIDs -> media device IDs (?)

https://codereview.chromium.org/54863002/diff/720001/content/public/browser/r...
content/public/browser/resource_context.h:61: // if cookies are cleared. This is
called on the IO thread.
"This is called on the IO thread" is probably redundant given the comment for
the class that it lives on the IO thread.

[perkj_chrome, 2013-11-04 19:05:37]

PTAL

https://codereview.chromium.org/54863002/diff/620001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/54863002/diff/620001/content/public/browser/c...
content/public/browser/content_browser_client.h:530: virtual std::string
GetMediaDeviceIDSalt(ResourceContext* rc);
On 2013/11/04 15:38:53, jam wrote:
> since this method is invoked per ResourceContext, it should be moved to be on
> ResourceContext

ok- done.

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:961:
media_device_id_salt_.reset(NULL);
On 2013/11/04 16:53:29, Jói wrote:
> I'm not sure this is correct (destroying it on the UI thread). I would suggest
> moving ownership of MediaDeviceIDSalt to the ResourceContext object since
that's
> where it's used from. You could just add an is_incognito parameter to the
> constructor of ResourceContext.

This is as the same as the other objects and ensures that the objects are
created and destroyed on the UI thread if I understand this correct.
 If I move the ownership I get problem that I need the  the PrefService. I can
of course add  InitializeOnUIThread(Profile* profile) on the resource context as
well but MediaDeviceIdSalt::media_device_id_salt_ (StringPrefMember) must be
destroyed on the UI thread... So in that case I need to add a 
ShutdownOnUIThread() as well on the resource context... All in all it seems
better to me to keep this? Wdyt?

[Jói, 2013-11-04 19:24:14]

LGTM

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:961:
media_device_id_salt_.reset(NULL);
On 2013/11/04 19:05:38, perkj wrote:
> On 2013/11/04 16:53:29, Jói wrote:
> > I'm not sure this is correct (destroying it on the UI thread). I would
suggest
> > moving ownership of MediaDeviceIDSalt to the ResourceContext object since
> that's
> > where it's used from. You could just add an is_incognito parameter to the
> > constructor of ResourceContext.
> 
> This is as the same as the other objects and ensures that the objects are
> created and destroyed on the UI thread if I understand this correct.
>  If I move the ownership I get problem that I need the  the PrefService. I can
> of course add  InitializeOnUIThread(Profile* profile) on the resource context
as
> well but MediaDeviceIdSalt::media_device_id_salt_ (StringPrefMember) must be
> destroyed on the UI thread... So in that case I need to add a 
> ShutdownOnUIThread() as well on the resource context... All in all it seems
> better to me to keep this? Wdyt?  
> 

I see now from the ProfileIOData documentation that Profile will make sure to
destroy ProfileIOData on the IO thread. So either approach is fine.

[willchan no longer on Chromium, 2013-11-04 19:32:59]

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:961:
media_device_id_salt_.reset(NULL);
On 2013/11/04 19:24:15, Jói wrote:
> On 2013/11/04 19:05:38, perkj wrote:
> > On 2013/11/04 16:53:29, Jói wrote:
> > > I'm not sure this is correct (destroying it on the UI thread). I would
> suggest
> > > moving ownership of MediaDeviceIDSalt to the ResourceContext object since
> > that's
> > > where it's used from. You could just add an is_incognito parameter to the
> > > constructor of ResourceContext.
> > 
> > This is as the same as the other objects and ensures that the objects are
> > created and destroyed on the UI thread if I understand this correct.
> >  If I move the ownership I get problem that I need the  the PrefService. I
can
> > of course add  InitializeOnUIThread(Profile* profile) on the resource
context
> as
> > well but MediaDeviceIdSalt::media_device_id_salt_ (StringPrefMember) must be
> > destroyed on the UI thread... So in that case I need to add a 
> > ShutdownOnUIThread() as well on the resource context... All in all it seems
> > better to me to keep this? Wdyt?  
> > 
> 
> I see now from the ProfileIOData documentation that Profile will make sure to
> destroy ProfileIOData on the IO thread. So either approach is fine.

Sorry, I missed this. Just to doublecheck, what thread *should* this
MediaDeviceIDSalt be used on? IO thread? If so, it should *not* be destroyed
here. This function is for objects that need 2-phase shutdown (initiate shutdown
on UI thread, and then it gets deleted on the IO thread). You're actually doing
a delete on the UI thread. If this object is used on the IO thread, then this is
wrong and will lead to a use after free.

[perkj_chrome, 2013-11-04 20:16:34]

On 2013/11/04 19:32:59, willchan wrote:
>
https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
> chrome/browser/profiles/profile_io_data.cc:961:
> media_device_id_salt_.reset(NULL);
> On 2013/11/04 19:24:15, Jói wrote:
> > On 2013/11/04 19:05:38, perkj wrote:
> > > On 2013/11/04 16:53:29, Jói wrote:
> > > > I'm not sure this is correct (destroying it on the UI thread). I would
> > suggest
> > > > moving ownership of MediaDeviceIDSalt to the ResourceContext object
since
> > > that's
> > > > where it's used from. You could just add an is_incognito parameter to
the
> > > > constructor of ResourceContext.
> > > 
> > > This is as the same as the other objects and ensures that the objects are
> > > created and destroyed on the UI thread if I understand this correct.
> > >  If I move the ownership I get problem that I need the  the PrefService. I
> can
> > > of course add  InitializeOnUIThread(Profile* profile) on the resource
> context
> > as
> > > well but MediaDeviceIdSalt::media_device_id_salt_ (StringPrefMember) must
be
> > > destroyed on the UI thread... So in that case I need to add a 
> > > ShutdownOnUIThread() as well on the resource context... All in all it
seems
> > > better to me to keep this? Wdyt?  
> > > 
> > 
> > I see now from the ProfileIOData documentation that Profile will make sure
to
> > destroy ProfileIOData on the IO thread. So either approach is fine.
> 
> Sorry, I missed this. Just to doublecheck, what thread *should* this
> MediaDeviceIDSalt be used on? IO thread? If so, it should *not* be destroyed
> here. This function is for objects that need 2-phase shutdown (initiate
shutdown
> on UI thread, and then it gets deleted on the IO thread). You're actually
doing
> a delete on the UI thread. If this object is used on the IO thread, then this
is
> wrong and will lead to a use after free.

So now MediaDeviceIDSalt is owned by ProfileIOData and created and destroyed on
the UI thread.
It is however used via ProfileIOData::ResourceContext on the IO thread.
I am not sure I see the difference between what we do and example
google_services_username_ ?
Is there a better way to create and use a StringPrefMember on the IO thread?

[perkj_chrome, 2013-11-04 22:35:25]

On 2013/11/04 20:16:34, perkj wrote:
> On 2013/11/04 19:32:59, willchan wrote:
> >
>
https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
> > File chrome/browser/profiles/profile_io_data.cc (right):
> > 
> >
>
https://codereview.chromium.org/54863002/diff/720001/chrome/browser/profiles/...
> > chrome/browser/profiles/profile_io_data.cc:961:
> > media_device_id_salt_.reset(NULL);
> > On 2013/11/04 19:24:15, Jói wrote:
> > > On 2013/11/04 19:05:38, perkj wrote:
> > > > On 2013/11/04 16:53:29, Jói wrote:
> > > > > I'm not sure this is correct (destroying it on the UI thread). I would
> > > suggest
> > > > > moving ownership of MediaDeviceIDSalt to the ResourceContext object
> since
> > > > that's
> > > > > where it's used from. You could just add an is_incognito parameter to
> the
> > > > > constructor of ResourceContext.
> > > > 
> > > > This is as the same as the other objects and ensures that the objects
are
> > > > created and destroyed on the UI thread if I understand this correct.
> > > >  If I move the ownership I get problem that I need the  the PrefService.
I
> > can
> > > > of course add  InitializeOnUIThread(Profile* profile) on the resource
> > context
> > > as
> > > > well but MediaDeviceIdSalt::media_device_id_salt_ (StringPrefMember)
must
> be
> > > > destroyed on the UI thread... So in that case I need to add a 
> > > > ShutdownOnUIThread() as well on the resource context... All in all it
> seems
> > > > better to me to keep this? Wdyt?  
> > > > 
> > > 
> > > I see now from the ProfileIOData documentation that Profile will make sure
> to
> > > destroy ProfileIOData on the IO thread. So either approach is fine.
> > 
> > Sorry, I missed this. Just to doublecheck, what thread *should* this
> > MediaDeviceIDSalt be used on? IO thread? If so, it should *not* be destroyed
> > here. This function is for objects that need 2-phase shutdown (initiate
> shutdown
> > on UI thread, and then it gets deleted on the IO thread). You're actually
> doing
> > a delete on the UI thread. If this object is used on the IO thread, then
this
> is
> > wrong and will lead to a use after free.
> 
> So now MediaDeviceIDSalt is owned by ProfileIOData and created and destroyed
on
> the UI thread.
> It is however used via ProfileIOData::ResourceContext on the IO thread.
> I am not sure I see the difference between what we do and example
> google_services_username_ ?
> Is there a better way to create and use a StringPrefMember on the IO thread?

Humm- if I use my head and read the PrefMember::Destroy documentation I do
understand the difference.
New suggestion uploaded...

I also found out that due to the move to ResourceContext there are more files
that content/shell also need to be changed. 

Even with this change I still need to updated the new tests in
webrtc_audio_private_browsertest.cc with a resource context....

[willchan no longer on Chromium, 2013-11-05 00:19:03]

This looks fine to me now.

https://codereview.chromium.org/54863002/diff/680002/chrome/browser/media/med...
File chrome/browser/media/media_device_id_salt.h (right):

https://codereview.chromium.org/54863002/diff/680002/chrome/browser/media/med...
chrome/browser/media/media_device_id_salt.h:35: };
DISALLOW_COPY_AND_ASSIGN

[perkj_chrome, 2013-11-05 13:58:31]

Finally the bots seem happy with my changes as well. 
Joi - would you mind taking a new look? 

You might also want to take a look at 
http://build.chromium.org/p/tryserver.chromium/builders/mac_swarm_triggered/b...
and search for GetAssociatedSink. 
(The same test pass on windows swarm.)

[Jói, 2013-11-05 14:14:07]

Other than the below comment, I think this is ready.

https://codereview.chromium.org/54863002/diff/1230001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc
(right):

https://codereview.chromium.org/54863002/diff/1230001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:261:
function->GetProfile()->GetResourceContext(),
I don't know for sure, but I would think function->GetProfile here is going to
be NULL, since I don't think you have initialized it in any way. When function
objects are created by the extension system, that's different I believe since
they probably go through more initialization.

In an ExtensionAPITest (which this is) you can directly call profile()
(inherited from somewhere up the class hierarchy) to get the current profile.

[perkj_chrome, 2013-11-05 16:25:05]

https://codereview.chromium.org/54863002/diff/1230001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc
(right):

https://codereview.chromium.org/54863002/diff/1230001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:261:
function->GetProfile()->GetResourceContext(),
On 2013/11/05 14:14:08, Jói wrote:
> I don't know for sure, but I would think function->GetProfile here is going to
> be NULL, since I don't think you have initialized it in any way. When function
> objects are created by the extension system, that's different I believe since
> they probably go through more initialization.
> 
> In an ExtensionAPITest (which this is) you can directly call profile()
> (inherited from somewhere up the class hierarchy) to get the current profile.

Done.

[commit-bot: I haz the power, 2013-11-05 16:30:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/54863002/1360001

[Jói, 2013-11-05 16:51:02]

LGTM

[commit-bot: I haz the power, 2013-11-05 18:42:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jam, 2013-11-06 00:20:02]

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
File content/public/browser/media_device_id.cc (right):

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
content/public/browser/media_device_id.cc:10: #include
"content/public/common/content_client.h"
nit: remove these two

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
File content/public/browser/media_device_id.h (right):

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
content/public/browser/media_device_id.h:16: #include
"content/public/browser/resource_context.h"
nit: forward declare and move this include to the cc file

[perkj_chrome, 2013-11-28 21:42:13]

Rebased and fixed Jams comments. 
Joi- would you mind to do a new sanity check?

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
File content/public/browser/media_device_id.cc (right):

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
content/public/browser/media_device_id.cc:10: #include
"content/public/common/content_client.h"
On 2013/11/06 00:20:03, jam wrote:
> nit: remove these two

Done.

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
File content/public/browser/media_device_id.h (right):

https://codereview.chromium.org/54863002/diff/1360001/content/public/browser/...
content/public/browser/media_device_id.h:16: #include
"content/public/browser/resource_context.h"
On 2013/11/06 00:20:03, jam wrote:
> nit: forward declare and move this include to the cc file

Done.

https://codereview.chromium.org/54863002/diff/1630001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/1630001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:293:
GetProfile()->GetResourceContext(),
indentation

[Jói, 2013-11-29 14:50:02]

Sanity check done, LGTM.

[commit-bot: I haz the power, 2013-11-30 16:32:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/54863002/1630001

[commit-bot: I haz the power, 2013-11-30 18:26:57]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[perkj_chrome, 2013-11-30 22:01:26]

Looks like one more change is needed in webrtc_audio_private_browsertest.cc to
get that to retreive the source id on the IO thread. 
Can you take a look at my proposed solution Joi?

[Jói, 2013-12-02 10:05:52]

LGTM with a tiny nit.

https://codereview.chromium.org/54863002/diff/1690001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/1690001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:296:
GetProfile()->GetResourceContext(),
nit: I would group the two explicit parameters together, so either pass the
ResourceContext first or last, not between the two parameters.

[perkj_chrome, 2013-12-02 13:26:47]

https://codereview.chromium.org/54863002/diff/1690001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
(right):

https://codereview.chromium.org/54863002/diff/1690001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:296:
GetProfile()->GetResourceContext(),
On 2013/12/02 10:05:53, Jói wrote:
> nit: I would group the two explicit parameters together, so either pass the
> ResourceContext first or last, not between the two parameters.

Done.

[commit-bot: I haz the power, 2013-12-02 13:27:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/54863002/1710001

[commit-bot: I haz the power, 2013-12-02 15:19:25]

Change committed as 238103