Conditionally build support for Pepper-based CDMs.

Conditionally build support for Pepper-based CDMs.

Adds enable_pepper_cdms and ENABLE_PEPPER_CDMS to control building of this logic.
Previously, it was built for all platforms, but not all platforms use Pepper.

TEST=content_browsertests on platforms with and without Pepper CDM support

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

[xhwang, 2013-05-10 00:26:44]

Generally looks good. I just have some question why some pepper related code
aren't in the #ifdef blocks.

https://codereview.chromium.org/15028015/diff/2001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/2001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_browsertest.cc:482: const char
kLibraryName[] = "clearkeycdmadapter.plugin";
Just double check: on systems that don't have ENABLE_PEPPER_CDMS, the still have
"plugin"s?

https://codereview.chromium.org/15028015/diff/2001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_browsertest.cc:507: void
RunLoadPepperPluginTest(const char* mime_type, bool expect_loaded) {
What about this and line 582?

https://codereview.chromium.org/15028015/diff/2001/content/browser/media/encr...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/2001/content/browser/media/encr...
content/browser/media/encrypted_media_browsertest.cc:39: static const char
kWidevineLibraryName[] = "libwidevinecdmadapter.so";
For WV, can we use kWidevineCdmPluginFileName in widevine_cdm_common.h?

https://codereview.chromium.org/15028015/diff/2001/content/browser/media/encr...
content/browser/media/encrypted_media_browsertest.cc:109:
RegisterPepperCdm(command_line, kClearKeyLibraryName,
what's the reason these pepper related code isn't in the ENABLE_PEPPER_CDMS
scope.

[ddorwin, 2013-05-14 00:17:10]

Addressed feedback and cleaned up a few more things. Revision 3 shows changes
before rebasing.

https://codereview.chromium.org/15028015/diff/2001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/2001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_browsertest.cc:482: const char
kLibraryName[] = "clearkeycdmadapter.plugin";
On 2013/05/10 00:26:44, xhwang wrote:
> Just double check: on systems that don't have ENABLE_PEPPER_CDMS, the still
have
> "plugin"s?

Done. I moved the ifdef up. Yes, plugin (ppapi & npapi) support is separate from
whether platforms use Pepper for CDMs. (Or, at least, the code allows that.)

https://codereview.chromium.org/15028015/diff/2001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_browsertest.cc:507: void
RunLoadPepperPluginTest(const char* mime_type, bool expect_loaded) {
On 2013/05/10 00:26:44, xhwang wrote:
> What about this and line 582?

It's used for a NaCl test. If NaCl and Pepper CDMs were disabled, we'd compile
some extra code in our tests. I don't think it's a big deal.

https://codereview.chromium.org/15028015/diff/2001/content/browser/media/encr...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/2001/content/browser/media/encr...
content/browser/media/encrypted_media_browsertest.cc:39: static const char
kWidevineLibraryName[] = "libwidevinecdmadapter.so";
On 2013/05/10 00:26:44, xhwang wrote:
> For WV, can we use kWidevineCdmPluginFileName in widevine_cdm_common.h?

Done.

https://codereview.chromium.org/15028015/diff/2001/content/browser/media/encr...
content/browser/media/encrypted_media_browsertest.cc:109:
RegisterPepperCdm(command_line, kClearKeyLibraryName,
On 2013/05/10 00:26:44, xhwang wrote:
> what's the reason these pepper related code isn't in the ENABLE_PEPPER_CDMS
> scope.

We can handle compatibility with a single ifdef at 116 instead of adding 2 or
more ifdef blocks. It seems cleaner this way, though I see how this might
confuse someone.

UPDATE: After I ended up surrounding these as well since the |library_name| is
specific to the Pepper case.

[xhwang, 2013-05-14 16:35:44]

Looking mostly good with one comment and some nits.

I am not sure what's your convention of putting comment after "#endif" (i.e.
#endif  // defined(ENABLE_PEPPER_CDMS)). But I felt it's a bit inconsistent in
this CL. I pointed out some places then started to think you may have your rule
when to add the comment... WDYT?

https://codereview.chromium.org/15028015/diff/21001/content/browser/media/enc...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/21001/content/browser/media/enc...
content/browser/media/encrypted_media_browsertest.cc:131: #endif  //
defined(ENABLE_PEPPER_CDMS)
shouldn't this be after line 132?

https://codereview.chromium.org/15028015/diff/21001/content/browser/media/enc...
content/browser/media/encrypted_media_browsertest.cc:181: #endif
nit: #endif  // defined(ENABLE_PEPPER_CDMS)

https://codereview.chromium.org/15028015/diff/21001/third_party/widevine/cdm/...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/15028015/diff/21001/third_party/widevine/cdm/...
third_party/widevine/cdm/widevine_cdm_common.h:26: #endif
nit: #endif  // defined(ENABLE_PEPPER_CDMS)

https://codereview.chromium.org/15028015/diff/21001/webkit/media/crypto/key_s...
File webkit/media/crypto/key_systems.h (right):

https://codereview.chromium.org/15028015/diff/21001/webkit/media/crypto/key_s...
webkit/media/crypto/key_systems.h:41: #endif
nit: #endif  // defined(ENABLE_PEPPER_CDMS)

[ddorwin, 2013-05-14 20:14:28]

Replies and fixed a unit test.

https://codereview.chromium.org/15028015/diff/21001/content/browser/media/enc...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/21001/content/browser/media/enc...
content/browser/media/encrypted_media_browsertest.cc:131: #endif  //
defined(ENABLE_PEPPER_CDMS)
On 2013/05/14 16:35:44, xhwang wrote:
> shouldn't this be after line 132?

Done. Fixed in Patch 5.

https://codereview.chromium.org/15028015/diff/21001/content/browser/media/enc...
content/browser/media/encrypted_media_browsertest.cc:181: #endif
On 2013/05/14 16:35:44, xhwang wrote:
> nit: #endif  // defined(ENABLE_PEPPER_CDMS)

I believe the general pattern is to add a comment if there are nested ifdefs
and/or a long block of code between the #if and #endif. It's not an exact
science. In this case, the beginning and end are close together.

https://codereview.chromium.org/15028015/diff/21001/third_party/widevine/cdm/...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/15028015/diff/21001/third_party/widevine/cdm/...
third_party/widevine/cdm/widevine_cdm_common.h:26: #endif
On 2013/05/14 16:35:44, xhwang wrote:
> nit: #endif  // defined(ENABLE_PEPPER_CDMS)

Same.

https://codereview.chromium.org/15028015/diff/21001/webkit/media/crypto/key_s...
File webkit/media/crypto/key_systems.h (right):

https://codereview.chromium.org/15028015/diff/21001/webkit/media/crypto/key_s...
webkit/media/crypto/key_systems.h:41: #endif
On 2013/05/14 16:35:44, xhwang wrote:
> nit: #endif  // defined(ENABLE_PEPPER_CDMS)

Same

[ddorwin, 2013-05-23 00:58:25]

thestig, please do an OWNERS review of the chrome/ files.

[Lei Zhang, 2013-05-23 01:18:49]

chrome lgtm/

https://codereview.chromium.org/15028015/diff/56001/content/browser/media/enc...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/56001/content/browser/media/enc...
content/browser/media/encrypted_media_browsertest.cc:119: base::FilePath
plugin_lib = plugin_dir.Append(adapter_name);
You can AppendASCII() and skip all the FILE_PATH_LITERAL bits.

[ddorwin, 2013-05-23 01:47:31]

https://codereview.chromium.org/15028015/diff/56001/content/browser/media/enc...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/15028015/diff/56001/content/browser/media/enc...
content/browser/media/encrypted_media_browsertest.cc:119: base::FilePath
plugin_lib = plugin_dir.Append(adapter_name);
On 2013/05/23 01:18:50, Lei Zhang wrote:
> You can AppendASCII() and skip all the FILE_PATH_LITERAL bits.

Done. Thanks!

[commit-bot: I haz the power, 2013-05-23 01:47:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/15028015/62002

[commit-bot: I haz the power, 2013-05-23 10:44:40]

Change committed as 201738