media: Add helper function to register pepper CDMs

media: Add helper function to register pepper CDMs

This removes duplicate pepper CDM registration code.

BUG=610581, 582622
TEST=No functionality change.

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

[xhwang, 2016-05-14 07:35:36]

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

[xhwang, 2016-05-14 07:35:37]

A cleanup CL. PTAL

[ddorwin, 2016-05-16 21:37:55]

LGTM with commments

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.cc (right):

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.cc:15: #if
defined(ENABLE_PEPPER_CDMS)
Do we even build this if not defined? I believe that's a common strategy.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.h (right):

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:12: #if
defined(ENABLE_PEPPER_CDMS)
nit: Should we ifdef the includes too?

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:24: extern const char
kClearKeyCdmPluginMimeType[];
Use "Pepper" or just drop the word.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:27: // cdm using the provided
parameters and a dummy version.
nit: CDM

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:28: // Multiple results may be
passed to kRegisterPepperPlugins, separated by ",".
Return values from multiple calls may be passed to the kRegisterPepperPlugins
switch...

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:37: void
RegisterPepperCdm(base::CommandLine* command_line,
// May only be called once per test (|command_line|).

We lose the ability to DCHECK for this, though we could probably search for the
switch. Probably not worth it since this will be removed soon.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/chrome_te...
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/chrome_te...
chrome/chrome_tests.gypi:2674: ['enable_pepper_cdms==1', {
New .cc could be added here?

[xhwang, 2016-05-18 00:38:29]

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.cc (right):

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.cc:15: #if
defined(ENABLE_PEPPER_CDMS)
On 2016/05/16 21:37:55, ddorwin wrote:
> Do we even build this if not defined? I believe that's a common strategy.

Done.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.h (right):

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:12: #if
defined(ENABLE_PEPPER_CDMS)
On 2016/05/16 21:37:55, ddorwin wrote:
> nit: Should we ifdef the includes too?

Not necessary now since we only use this file when enable_pepper_cdms is true.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:24: extern const char
kClearKeyCdmPluginMimeType[];
On 2016/05/16 21:37:55, ddorwin wrote:
> Use "Pepper" or just drop the word.

Done.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:28: // Multiple results may be
passed to kRegisterPepperPlugins, separated by ",".
On 2016/05/16 21:37:55, ddorwin wrote:
> Return values from multiple calls may be passed to the kRegisterPepperPlugins
> switch...

Done.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:37: void
RegisterPepperCdm(base::CommandLine* command_line,
On 2016/05/16 21:37:55, ddorwin wrote:
> // May only be called once per test (|command_line|).
> 
> We lose the ability to DCHECK for this, though we could probably search for
the
> switch. Probably not worth it since this will be removed soon.

Acknowledged.

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/chrome_te...
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/chrome_te...
chrome/chrome_tests.gypi:2674: ['enable_pepper_cdms==1', {
On 2016/05/16 21:37:55, ddorwin wrote:
> New .cc could be added here?

Good point. Done.

[xhwang, 2016-05-18 00:39:12]

xhwang@chromium.org changed reviewers:
+ thestig@chromium.org

[xhwang, 2016-05-18 00:39:13]

thestig: Please OWNERS review chrome/. This is a small cleanup change.

[Lei Zhang, 2016-05-18 00:53:11]

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.cc (right):

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.cc:31:
DCHECK(PathService::Get(base::DIR_MODULE, &adapter_path));
Can't put this in a DCHECK. It won't run in release builds.

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.cc:35: base::FilePath::StringType
pepper_cdm_registration = adapter_path.value();
If this is just a base::FilePath, then you can use AppendASCII() below and you
won't need the ifdef.

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.h (right):

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:25: // The CDM adapter should be
located in |adapter_base_dir| in DIR_MODULE.
There's no |adapter_base_dir| param.

[xhwang, 2016-05-18 02:08:10]

Thanks for the review. Please take a look again.

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.cc (right):

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.cc:31:
DCHECK(PathService::Get(base::DIR_MODULE, &adapter_path));
On 2016/05/18 00:53:11, Lei Zhang wrote:
> Can't put this in a DCHECK. It won't run in release builds.

Good catch. It's really not interesting to check this. Dropped the DCHECK.

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.cc:35: base::FilePath::StringType
pepper_cdm_registration = adapter_path.value();
On 2016/05/18 00:53:11, Lei Zhang wrote:
> If this is just a base::FilePath, then you can use AppendASCII() below and you
> won't need the ifdef.

The registration is a string. AppendASCII will add delimiters like "/" that
won't work here.

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
File chrome/browser/media/pepper_cdm_test_helper.h (right):

https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/m...
chrome/browser/media/pepper_cdm_test_helper.h:25: // The CDM adapter should be
located in |adapter_base_dir| in DIR_MODULE.
On 2016/05/18 00:53:11, Lei Zhang wrote:
> There's no |adapter_base_dir| param.

Done.

[Lei Zhang, 2016-05-18 02:22:47]

lgtm

[xhwang, 2016-05-18 02:25:47]

Patchset #5 (id:70001) has been deleted

[xhwang, 2016-05-18 02:26:22]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-05-18 02:26:23]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1983563002/#ps50008
(title: "comments")

[commit-bot: I haz the power, 2016-05-18 02:27:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983563002/50008
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983563002/50008

[commit-bot: I haz the power, 2016-05-18 04:18:07]

Committed patchset #4 (id:50008)

[commit-bot: I haz the power, 2016-05-18 04:19:39]

Description was changed from

==========
media: Add helper function to register pepper CDMs

This removes duplicate pepper CDM registration code.

BUG=610581,582622
TEST=No functionality change.
==========

to

==========
media: Add helper function to register pepper CDMs

This removes duplicate pepper CDM registration code.

BUG=610581,582622
TEST=No functionality change.

Committed: https://crrev.com/eb61e1d9dde1d6c02919c3716a2fd6729267cbac
Cr-Commit-Position: refs/heads/master@{#394332}
==========

[commit-bot: I haz the power, 2016-05-18 04:19:40]

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

[Nico, 2016-05-18 14:27:21]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-05-18 14:27:22]

Looks like this make tests fail in official builds:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/build...
(that bot also uses clang, but the interesting bit is that it does official,
chrome-branded builds)

[xhwang, 2016-05-18 16:02:28]

On 2016/05/18 14:27:22, Nico (vacation May 19 to 22) wrote:
> Looks like this make tests fail in official builds:
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/build...
> (that bot also uses clang, but the interesting bit is that it does official,
> chrome-branded builds)

I'll see if I can fix it in 20 mins. Otherwise I'll revert.

[xhwang, 2016-05-18 16:34:11]

On 2016/05/18 16:02:28, xhwang wrote:
> On 2016/05/18 14:27:22, Nico (vacation May 19 to 22) wrote:
> > Looks like this make tests fail in official builds:
> >
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/build...
> > (that bot also uses clang, but the interesting bit is that it does official,
> > chrome-branded builds)
> 
> I'll see if I can fix it in 20 mins. Otherwise I'll revert.

I can't repro on Linux. I'll revert and investigate more.

[xhwang, 2016-05-18 16:34:59]

A revert of this CL (patchset #4 id:50008) has been created in
https://chromiumcodereview.appspot.com/1993813002/ by xhwang@chromium.org.

The reason for reverting is: This make tests fail in official builds:

https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/build....