Introduce the ENABLE_BROWSER_CDMS macro.

Introduce the ENABLE_BROWSER_CDMS macro.

This will be defined when a platform needs to use browser side CDM to implement
EME API. Currently this is only used on Android.

Note that MediaWebContentsObserver is shared by Android media player managers
and browser CDM managers. Since ENABLE_BROWSER_CDMS is always true on Android,
we only check ENABLE_BROWSER_CDMS to decide whether MediaWebContentsObserver
should be used. Also, in media_web_contents_observer.*, we only check
OS_ANDROID to decide whether Android media player code should be used. This is
not perfect but makes the current code simple. This will be fixed when we have
a general (not Android specic) media player manager.

TBR=yfriedman@chromium.org
BUG=315312
TEST=Compiles on Android. Test pages still work on Android. Compiles when I choose to use browser CDM on Linux.

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

[xhwang, 2014-06-05 00:21:25]

PTAL!

https://codereview.chromium.org/318753010/diff/20001/content/browser/media/me...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/318753010/diff/20001/content/browser/media/me...
content/browser/media/media_web_contents_observer.h:66: BrowserCdmManager*
GetCdmManager(RenderFrameHost* render_frame_host);
I only moved things around to reduce number of #ifdef's. No other code editing.

https://codereview.chromium.org/318753010/diff/20001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/318753010/diff/20001/content/content_browser....
content/content_browser.gypi:1767:
'browser/media/media_web_contents_observer.h',
This is working for Android because Android always
definesenable_browser_cdms==1.

[xhwang, 2014-06-05 00:22:43]

BTW: this CL is based on https://codereview.chromium.org/315733003/

[ddorwin, 2014-06-05 17:59:48]

LGTM % comments

https://codereview.chromium.org/318753010/diff/20001/content/browser/media/me...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/318753010/diff/20001/content/browser/media/me...
content/browser/media/media_web_contents_observer.h:66: BrowserCdmManager*
GetCdmManager(RenderFrameHost* render_frame_host);
On 2014/06/05 00:21:25, xhwang wrote:
> I only moved things around to reduce number of #ifdef's. No other code
editing.

nit: Does it make more sense to put the common code first?

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:79: int GetCdmId() const;
Since this has a clear purpose now, should we name it GetBrowserCdmId (and
browser_cdm_id_)?

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:58: int GetCdmId()
const;
ditto

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:792: // TODO(xhwang): Fix this if
we have a platform that uses prefixed EME
Does this actually get compiled on such platforms today? If not, maybe #error?

https://codereview.chromium.org/318753010/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:444: #if defined(OS_ANDROID) &&
defined(VIDEO_HOLE)
OS_ANDROID seems unnecessary here.

[xhwang, 2014-06-09 16:18:45]

jam@: Please OWNERS review content/renderer/render_frame_impl.* and
content/browser/renderer_host/*

yfriedman@: Please OWNERS review content/browser/android/*

[jam, 2014-06-09 20:02:54]

lgtm

https://codereview.chromium.org/318753010/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/318753010/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.h:72: #if defined(ENABLE_BROWSER_CDMS)
nit: skip the ifdef around the forward declaration

[xhwang, 2014-06-09 20:54:15]

comments addressed

[xhwang, 2014-06-09 20:57:20]

https://codereview.chromium.org/318753010/diff/20001/content/browser/media/me...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/318753010/diff/20001/content/browser/media/me...
content/browser/media/media_web_contents_observer.h:66: BrowserCdmManager*
GetCdmManager(RenderFrameHost* render_frame_host);
On 2014/06/05 17:59:48, ddorwin wrote:
> On 2014/06/05 00:21:25, xhwang wrote:
> > I only moved things around to reduce number of #ifdef's. No other code
> editing.
> 
> nit: Does it make more sense to put the common code first?

Done.

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:79: int GetCdmId() const;
On 2014/06/05 17:59:48, ddorwin wrote:
> Since this has a clear purpose now, should we name it GetBrowserCdmId (and
> browser_cdm_id_)?

I tried. Actually it looks weird when you have code like:

proxy_decryptor_->GetBrowserCdmId() != RendererCdmManager::kInvalidCdmId)

It's a bit odd to use BrowserCdmId in the render side.

I'll take a look in another CL to see how we can improve this.

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:58: int GetCdmId()
const;
On 2014/06/05 17:59:48, ddorwin wrote:
> ditto

ditto

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:792: // TODO(xhwang): Fix this if
we have a platform that uses prefixed EME
On 2014/06/05 17:59:48, ddorwin wrote:
> Does this actually get compiled on such platforms today? If not, maybe #error?

Not yet. Added #error.

https://codereview.chromium.org/318753010/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/318753010/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:444: #if defined(OS_ANDROID) &&
defined(VIDEO_HOLE)
On 2014/06/05 17:59:48, ddorwin wrote:
> OS_ANDROID seems unnecessary here.

It's okay to drop OS_ANDROID here. But since |media_player_manager_| declaration
is guarded by OS_ANDROID, it's seems more consistent and easier to understand to
keep OS_ANDROID guard here.

[xhwang, 2014-06-09 20:57:34]

On 2014/06/09 20:02:54, jam wrote:
> lgtm
> 
>
https://codereview.chromium.org/318753010/diff/60001/content/renderer/render_...
> File content/renderer/render_frame_impl.h (right):
> 
>
https://codereview.chromium.org/318753010/diff/60001/content/renderer/render_...
> content/renderer/render_frame_impl.h:72: #if defined(ENABLE_BROWSER_CDMS)
> nit: skip the ifdef around the forward declaration

Done.

[Yaron, 2014-06-09 22:15:01]

content/browser/android lgtm

[xhwang, 2014-06-09 23:54:22]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-06-09 23:55:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/318753010/120001

[commit-bot: I haz the power, 2014-06-10 13:47:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 14:42:08]

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

[commit-bot: I haz the power, 2014-06-10 14:42:09]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[xhwang, 2014-06-10 15:34:35]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-06-10 15:36:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/318753010/120001

[commit-bot: I haz the power, 2014-06-10 16:26:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 17:10:03]

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

[commit-bot: I haz the power, 2014-06-10 17:10:04]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[xhwang, 2014-06-10 17:11:54]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-06-10 17:14:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/318753010/120001

[commit-bot: I haz the power, 2014-06-10 17:48:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 18:12:19]

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

[commit-bot: I haz the power, 2014-06-10 18:12:20]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[xhwang, 2014-06-10 18:51:27]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-06-10 18:55:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/318753010/120001

[commit-bot: I haz the power, 2014-06-10 23:21:58]

Change committed as 276200