Move MediaDeviceIDSalt from ProfileIOData to ProfileImpl.

Move MediaDeviceIDSalt from ProfileIOData to ProfileImpl.

The object is never used on the IO thread, and even had DCHECKs to
ensure that's the case, so it makes no sense in ProfileIOData.

It's also one of the few things exposed by ResourceContext, which we're
probably going to get rid of.

BUG=712296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2820163002
Cr-Commit-Position: refs/heads/master@{#466358}
Committed: https://chromium.googlesource.com/chromium/src/+/c0b2b8b1b919b0fcda51e691406d50a62f89771d

[mmenke, 2017-04-17 19:38:11]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 19:38:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 19:47:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 19:47:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2017-04-17 20:55:51]

Description was changed from

==========
No salt

No salt

BUG=
==========

to

==========
No salt

No salt

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[mmenke, 2017-04-17 20:56:04]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 20:56:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-04-17 20:58:13]

Description was changed from

==========
No salt

No salt

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
The object is never used on the IO thread, and even had DCHECKs to
ensure that's the case, so it makes no sense in ProfileIOData.

It's also one of the few things exposed by ResourceContext, which we're
probably going to get rid of.

BUG=712296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-04-17 21:09:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 21:09:16]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2017-04-17 21:35:04]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 21:35:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-04-17 21:44:15]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 21:44:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 21:58:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 21:58:52]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-04-17 22:20:41]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 22:20:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 23:41:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 23:41:29]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-04-18 18:41:13]

Patchset #2 (id:20001) has been deleted

[mmenke, 2017-04-18 18:52:15]

mmenke@chromium.org changed reviewers:
+ mlerman@chromium.org, tommi@chromium.org

[mmenke, 2017-04-18 18:52:16]

[tommi]:  Please review everything.
[mlerman]:  Please review profiles/profile_impl.* (I own the directory, but only
for ProfileIOData, and I'm not going to take advantage of that to ignore the big
scary warning).  Happy for suggestions on how to better get this stuff out of
ProfileIOData.

https://codereview.chromium.org/2820163002/diff/70001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/2820163002/diff/70001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.h:258: // STOP!!!! DO NOT ADD ANY MORE
ITEMS HERE!!!!
Note that I'm ignoring this, largely because this is already part of a content/
API, and I'm unaware of a good way to make this a service (Note that it has
consumers in chrome and content, a default behavior, and a special pref-based
behavior when run in Chrome).

I agree moving it makes sense, but I'm also reluctant to redesign a service I
neither own nor understand - I just want to remove the code from ProfileIOData,
as it's an even worse spot than here.  If someone on the media team that owns
the MediaDeviceIDSalt wants to do a more principled approach, I'm happy to hand
it off or work together on a better design.

[Mike Lerman, 2017-04-18 19:09:46]

https://codereview.chromium.org/2820163002/diff/70001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/2820163002/diff/70001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.h:258: // STOP!!!! DO NOT ADD ANY MORE
ITEMS HERE!!!!
On 2017/04/18 18:52:16, mmenke wrote:
> Note that I'm ignoring this, largely because this is already part of a
content/
> API, and I'm unaware of a good way to make this a service (Note that it has
> consumers in chrome and content, a default behavior, and a special pref-based
> behavior when run in Chrome).
> 
> I agree moving it makes sense, but I'm also reluctant to redesign a service I
> neither own nor understand - I just want to remove the code from
ProfileIOData,
> as it's an even worse spot than here.  If someone on the media team that owns
> the MediaDeviceIDSalt wants to do a more principled approach, I'm happy to
hand
> it off or work together on a better design.

webrtc_audio_private_api.cc is the only place in Chrome that needs to care about
the MediaDeviceIDSalt object or preference. Can we have that class either hold
the reference to the object (in place of the Profile) or just interact with, and
manage, the pref directly?

It seems like the answer here is that, since there's only one place in
chrome/browser that needs to care, to push the responsibility out of Profile
into there. It seems like, logically, the results would be equivalent; and while
we're going to the effort to move this logic out of profile_io_data, let's move
it out of chrome/browser/profiles completely.

[mmenke, 2017-04-18 20:04:57]

https://codereview.chromium.org/2820163002/diff/70001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/2820163002/diff/70001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.h:258: // STOP!!!! DO NOT ADD ANY MORE
ITEMS HERE!!!!
On 2017/04/18 19:09:46, Mike Lerman wrote:
> On 2017/04/18 18:52:16, mmenke wrote:
> > Note that I'm ignoring this, largely because this is already part of a
> content/
> > API, and I'm unaware of a good way to make this a service (Note that it has
> > consumers in chrome and content, a default behavior, and a special
pref-based
> > behavior when run in Chrome).
> > 
> > I agree moving it makes sense, but I'm also reluctant to redesign a service
I
> > neither own nor understand - I just want to remove the code from
> ProfileIOData,
> > as it's an even worse spot than here.  If someone on the media team that
owns
> > the MediaDeviceIDSalt wants to do a more principled approach, I'm happy to
> hand
> > it off or work together on a better design.
> 
> webrtc_audio_private_api.cc is the only place in Chrome that needs to care
about
> the MediaDeviceIDSalt object or preference. Can we have that class either hold
> the reference to the object (in place of the Profile) or just interact with,
and
> manage, the pref directly?
> 
> It seems like the answer here is that, since there's only one place in
> chrome/browser that needs to care, to push the responsibility out of Profile
> into there. It seems like, logically, the results would be equivalent; and
while
> we're going to the effort to move this logic out of profile_io_data, let's
move
> it out of chrome/browser/profiles completely.

Are you sure?  Looks like
content/browser/renderer_host/render_process_host_impl.cc passes it to the
AudioRendererHost and MediaStreamDispatcherHost

[tommi (sloooow) - chröme, 2017-04-19 10:45:39]

tommi@chromium.org changed reviewers:
+ guidou@chromium.org

[tommi (sloooow) - chröme, 2017-04-19 10:45:40]

Guido - can you take a look?

[Guido Urdaneta, 2017-04-19 10:55:41]

lgtm

[Mike Lerman, 2017-04-19 14:50:16]

Given neither of us is familiar with the intricacies of this change, are we
confident we're not introducing bad behavior?

I would strongly prefer that if this is being moved, it is done properly as some
sort of BrowserContextKeyedService/ProfileKeyedService. I understand you're
comment that we're just moving it from one profile entity to another, but at
least where it is now, we know everything works :)

[mmenke, 2017-04-19 15:07:58]

On 2017/04/19 14:50:16, Mike Lerman wrote:
> Given neither of us is familiar with the intricacies of this change, are we
> confident we're not introducing bad behavior?
> 
> I would strongly prefer that if this is being moved, it is done properly as
some
> sort of BrowserContextKeyedService/ProfileKeyedService. I understand you're
> comment that we're just moving it from one profile entity to another, but at
> least where it is now, we know everything works :)

I think this makes sense, unfortunately, I don't know a better design, given
that this is used in content/.  Given another design (Or a pointer to other
things that do this), I'm happy to do the right thing, and implement it.  We're
working towards getting rid of ResourceContext completely (And hopefully
ProfileIOData as well), so it can't stay where it is.

As for correctness:  It has DCHECKs that it's never used on the IO thread, and
the only production uses of it (In
/content/browser/renderer_host/render_process_host_impl.cc,
chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc,
and extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc) use
profile->GetResourceContext()->GetMediaDeviceIDSalt() all one one line, so we're
not introducing any profile/BrowserContext dereferences that would suddenly be
happening on another thread.

For startup, I initialize it earlier than it was initialized before, so no new
races there.

For shutdown, I put it after prefs_ (Which it depends on), so it's destroyed and
stops watching prefs before prefs are destroyed.  It's also destroyed later than
when it used to be (End of profile destructor, as opposed to when profile tells
the ProfileIOData to be torn down).

Any other potential problem I should be on the lookout for?

[Guido Urdaneta, 2017-04-19 15:15:00]

I did not author this code, but have worked on it before (I changed the return
value from a callback to a string and the default behavior from an empty string
to a random string in https://codereview.chromium.org/1987643002/). IMO, this CL
is an improvement over the current situation. I don't think it is introducing
bad behavior.

[Mike Lerman, 2017-04-20 13:30:40]

WDYT about creating a bug (tagged against both profiles and media components)
and leaving a TODO against that bug to migrate this out of c/b/profiles and into
some sort of KeyedService?

I'm sure there is a way to use either BrowserContextKeyedServices or
ProfileKeyedServices to achieve our goals; I've been out of Chrome for too long
to have a good intuition of what this is, but I'm sure there is one.

[Guido Urdaneta, 2017-04-20 14:14:54]

On 2017/04/20 13:30:40, Mike Lerman wrote:
> WDYT about creating a bug (tagged against both profiles and media components)
> and leaving a TODO against that bug to migrate this out of c/b/profiles and
into
> some sort of KeyedService?
> 
> I'm sure there is a way to use either BrowserContextKeyedServices or
> ProfileKeyedServices to achieve our goals; I've been out of Chrome for too
long
> to have a good intuition of what this is, but I'm sure there is one.

SGTM. You can assign the bug to me.

[mmenke, 2017-04-20 15:11:24]

On 2017/04/20 14:14:54, Guido Urdaneta wrote:
> On 2017/04/20 13:30:40, Mike Lerman wrote:
> > WDYT about creating a bug (tagged against both profiles and media
components)
> > and leaving a TODO against that bug to migrate this out of c/b/profiles and
> into
> > some sort of KeyedService?
> > 
> > I'm sure there is a way to use either BrowserContextKeyedServices or
> > ProfileKeyedServices to achieve our goals; I've been out of Chrome for too
> long
> > to have a good intuition of what this is, but I'm sure there is one.
> 
> SGTM. You can assign the bug to me.

SGTM, too.  Filed bug, added TODO.

[mmenke, 2017-04-20 15:13:49]

mmenke@chromium.org changed reviewers:
+ jochen@chromium.org, rdevlin.cronin@chromium.org, zork@chromium.org

[mmenke, 2017-04-20 15:13:50]

[+jochen]:  Please review content/browser (Moving something from ResourceContext
to BrowserContext - ResourceContext is going away, and should only be used on
the IO thread, anyways, and this is a UI-thread method).
[+rdevlin.cronin]:  Please review browser/extensions/api/webrtc_audio_private/
[+zork]:  Please review
extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc

Thanks!

[Devlin, 2017-04-20 15:39:15]

extensions lgtm

Description Nit:

The first line of the CL description field in Rietveld is what becomes the
commit title, so right now this would be committed with a title of "The object
is never used on the IO thread, and even had DCHECKs to".  Can you format the
description to include the title?
<Title>

<Description>

BUG=<nnn>

[mmenke, 2017-04-20 15:41:46]

Description was changed from

==========
The object is never used on the IO thread, and even had DCHECKs to
ensure that's the case, so it makes no sense in ProfileIOData.

It's also one of the few things exposed by ResourceContext, which we're
probably going to get rid of.

BUG=712296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move MediaDeviceIDSalt from ProfileIOData to ProfileImpl.

The object is never used on the IO thread, and even had DCHECKs to
ensure that's the case, so it makes no sense in ProfileIOData.

It's also one of the few things exposed by ResourceContext, which we're
probably going to get rid of.

BUG=712296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[mmenke, 2017-04-20 15:42:59]

On 2017/04/20 15:39:15, Devlin wrote:
> extensions lgtm
> 
> Description Nit:
> 
> The first line of the CL description field in Rietveld is what becomes the
> commit title, so right now this would be committed with a title of "The object
> is never used on the IO thread, and even had DCHECKs to".  Can you format the
> description to include the title?
> <Title>
> 
> <Description>
> 
> BUG=<nnn>

Done.  I blame our tooling - note that the issue title was what should have been
on the first line, but was not there, for some reason.

[Devlin, 2017-04-20 16:07:48]

On 2017/04/20 15:42:59, mmenke wrote:
> On 2017/04/20 15:39:15, Devlin wrote:
> > extensions lgtm
> > 
> > Description Nit:
> > 
> > The first line of the CL description field in Rietveld is what becomes the
> > commit title, so right now this would be committed with a title of "The
object
> > is never used on the IO thread, and even had DCHECKs to".  Can you format
the
> > description to include the title?
> > <Title>
> > 
> > <Description>
> > 
> > BUG=<nnn>
> 
> Done.  I blame our tooling - note that the issue title was what should have
been
> on the first line, but was not there, for some reason.

In my experience, this is usually because I change the description post-upload
and forget to put the title.  git cl upload will do the right thing, but if you
edit the description by hand, it won't auto-add the title back.

[Zachary Kuznia, 2017-04-20 16:10:54]

lgtm

[Mike Lerman, 2017-04-20 16:53:27]

lgtm, thanks for filing the bug

[jochen (gone - plz use gerrit), 2017-04-21 08:38:45]

lgtm

[mmenke, 2017-04-21 11:42:38]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-04-21 11:42:39]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org
Link to the patchset: https://codereview.chromium.org/2820163002/#ps110001
(title: "Reflow comment")

[commit-bot: I haz the power, 2017-04-21 11:42:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 11:45:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 11:45:44]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2017-04-21 15:26:58]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-04-21 15:26:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, jochen@chromium.org, mlerman@chromium.org,
zork@chromium.org, guidou@chromium.org
Link to the patchset: https://codereview.chromium.org/2820163002/#ps130001
(title: "merge")

[commit-bot: I haz the power, 2017-04-21 15:27:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 16:27:55]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1492788418211260,
"parent_rev": "0d5af94346c215cb917920baced6c06bb9110e61", "commit_rev":
"c0b2b8b1b919b0fcda51e691406d50a62f89771d"}

[commit-bot: I haz the power, 2017-04-21 16:28:10]

Description was changed from

==========
Move MediaDeviceIDSalt from ProfileIOData to ProfileImpl.

The object is never used on the IO thread, and even had DCHECKs to
ensure that's the case, so it makes no sense in ProfileIOData.

It's also one of the few things exposed by ResourceContext, which we're
probably going to get rid of.

BUG=712296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move MediaDeviceIDSalt from ProfileIOData to ProfileImpl.

The object is never used on the IO thread, and even had DCHECKs to
ensure that's the case, so it makes no sense in ProfileIOData.

It's also one of the few things exposed by ResourceContext, which we're
probably going to get rid of.

BUG=712296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2820163002
Cr-Commit-Position: refs/heads/master@{#466358}
Committed:
https://chromium.googlesource.com/chromium/src/+/c0b2b8b1b919b0fcda51e691406d...
==========

[commit-bot: I haz the power, 2017-04-21 16:28:12]

Committed patchset #7 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/c0b2b8b1b919b0fcda51e691406d...