media: Add BrowserCdmManagerProcessWatcher.

media: Add BrowserCdmManagerProcessWatcher.

Based on nick@chromium.org's CL: http://crrev.com/1214073015#ps1

Instead of having RenderProcessHostImpl hosts BrowserCdmManager, add
BrowserCdmManagerProcessWatcher, a RenderProcessHostObserver, to keep a
reference to BrowserCdmManager, and manage the entries in
g_browser_cdm_manager_map.

BUG=506188
TEST=Test reenabled.

Committed: https://crrev.com/ff9a7da8b4c0696dc9471c6b85a2ffbf6a1a46d0
Cr-Commit-Position: refs/heads/master@{#338146}

[xhwang, 2015-07-08 20:49:08]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, nick@chromium.org

[xhwang, 2015-07-08 20:49:08]

xhwang@chromium.org changed required reviewers:
+ nick@chromium.org

[xhwang, 2015-07-08 20:49:09]

PTAL

[ncarter (slow), 2015-07-08 20:56:31]

lgtm if it looks good to the android trybots

https://codereview.chromium.org/1225083002/diff/1/content/browser/media/cdm/b...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1225083002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:112: 
We should probably add a comment here along the lines of:

// Keeps the BrowserCdmManager alive, and in the global map, for as long as the
// RenderProcessHost is connected to the child process. This class is a
// self-owned observer.

[xhwang, 2015-07-08 22:15:30]

https://codereview.chromium.org/1225083002/diff/1/content/browser/media/cdm/b...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1225083002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:112: 
On 2015/07/08 20:56:31, ncarter wrote:
> We should probably add a comment here along the lines of:
> 
> // Keeps the BrowserCdmManager alive, and in the global map, for as long as
the
> // RenderProcessHost is connected to the child process. This class is a
> // self-owned observer.

Done.

[xhwang, 2015-07-08 22:16:20]

ddorwin: Do you want to take a look as well?

[ddorwin, 2015-07-09 17:52:22]

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:125: .second);
Is this checking that manager.get() is not NULL? Is there a reason not to do
that independent of the insert?

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:132: Destroy(host);
nit: This looks like it destroys |host|.

It's a bit strange that we need a parameter to destroy |this|. Should we have
saved it at line 122 then DCHECKed it here and had no parameter for Destroy()?

Alternatively, s/Destroy/RemoveHostOberverAndDestroy/ or something like that.

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:168: 
Can/should we check that there is no entry for the render_process_id? Can there
be an existing entry? If so, you might comment that. I don't think so, though,
because that would break FromProcess().

https://codereview.chromium.org/1225083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1225083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1026:
BrowserCdmManager* manager = BrowserCdmManager::FromProcess(GetID());
Is this the only use of FromProcess()? If so, we could entirely avoid the g_ map
by saving the Manager created at line 851.

[xhwang, 2015-07-09 19:04:18]

comments addressed

[xhwang, 2015-07-09 19:04:40]

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:125: .second);
On 2015/07/09 17:52:22, ddorwin wrote:
> Is this checking that manager.get() is not NULL? Is there a reason not to do
> that independent of the insert?

This is to check that the insert succeeded, meaning that there wasn't an
existing entry in the map with |render_process_id|.

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:132: Destroy(host);
On 2015/07/09 17:52:22, ddorwin wrote:
> nit: This looks like it destroys |host|.
> 
> It's a bit strange that we need a parameter to destroy |this|. Should we have
> saved it at line 122 then DCHECKed it here and had no parameter for Destroy()?

Hmm, all HostObservers know which Host they are observing. But still we have
|host| in RenderProcessExited() and RenderProcessHostDestroyed(). I think this
is root of this strangeness.

> Alternatively, s/Destroy/RemoveHostOberverAndDestroy/ or something like that.

Done.

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:168: 
On 2015/07/09 17:52:22, ddorwin wrote:
> Can/should we check that there is no entry for the render_process_id? 

This is indirectly checked on line 123.

> Can there be an existing entry? 

No. Basically you should never have two BrowserCdmManagerProcessWatcher alive
for the same process_id.

> If so, you might comment that. I don't think so, though,
> because that would break FromProcess().

Ack.

https://codereview.chromium.org/1225083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1225083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1026:
BrowserCdmManager* manager = BrowserCdmManager::FromProcess(GetID());
On 2015/07/09 17:52:22, ddorwin wrote:
> Is this the only use of FromProcess()? If so, we could entirely avoid the g_
map
> by saving the Manager created at line 851.

We also need it in MediaWebContentsObserver [1, 2]. That's the major reason why
we have this crazy map :(

Part of the idea of this CL is to NOT hold a reference to BCM in RPHI, to make
the lifetime management simpler.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

[ddorwin, 2015-07-09 19:13:17]

lgtm

[ncarter (slow), 2015-07-09 19:16:23]

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1225083002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:132: Destroy(host);
On 2015/07/09 19:04:39, xhwang wrote:
> On 2015/07/09 17:52:22, ddorwin wrote:
> > nit: This looks like it destroys |host|.
> > 
> > It's a bit strange that we need a parameter to destroy |this|. Should we
have
> > saved it at line 122 then DCHECKed it here and had no parameter for
Destroy()?
> 
> Hmm, all HostObservers know which Host they are observing. But still we have
> |host| in RenderProcessExited() and RenderProcessHostDestroyed(). I think this
> is root of this strangeness.
> 

It's structured this way to allow a RPHObserver instance to be an observer of
multiple RPH's at the same time.

> > Alternatively, s/Destroy/RemoveHostOberverAndDestroy/ or something like
that.
> 
> Done.

[xhwang, 2015-07-09 21:01:36]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-07-09 21:01:37]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1225083002/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-07-09 21:03:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225083002/40001

[commit-bot: I haz the power, 2015-07-09 21:12:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-09 21:13:05]

Patchset 3 (id:??) landed as
https://crrev.com/ff9a7da8b4c0696dc9471c6b85a2ffbf6a1a46d0
Cr-Commit-Position: refs/heads/master@{#338146}