media: Move key_system_names to media/base/

media: Move key_system_names to media/base/

This file is already used from media/base/ and should be there to
clean up some dependency issues (as noted by gn check).

BUG=599578
TEST=compiles with gn and gyp

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

[jrummell, 2016-04-25 21:08:30]

jrummell@chromium.org changed reviewers:
+ sandersd@chromium.org

[jrummell, 2016-04-25 21:08:30]

PTAL. Simply moving a file to media/base/

[ddorwin, 2016-04-25 21:32:16]

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

[ddorwin, 2016-04-25 21:32:16]

To the extent that media/base/ is our public directory, I'm not sure this should
be exposed. It appears that the only reference from media/base/ is
media/base/key_systems.cc. 
halliwell@ is currently refactoring that, and it's possible we could move the
definition of ClearKey and others outside media/base/, leaving only the
registry/interface in key_systems.cc.

Can we suppress this until halliwell@'s work is done?

[ddorwin, 2016-04-25 23:25:58]

On 2016/04/25 21:32:16, ddorwin wrote:
> To the extent that media/base/ is our public directory, I'm not sure this
should
> be exposed. It appears that the only reference from media/base/ is
> media/base/key_systems.cc. 
> halliwell@ is currently refactoring that, and it's possible we could move the
> definition of ClearKey and others outside media/base/, leaving only the
> registry/interface in key_systems.cc.
> 
> Can we suppress this until halliwell@'s work is done?

Looking at the places where values exposed by key_system_names.h are used,
almost all of them are in key_systems.cc. Thus, the right eventual solution
might be that these values should be in media/base but only inside
key_systems.cc.

The exceptions are:
* media/blink/webcontentdecryptionmodulesession_impl.cc, which I think should
instead use the new KeySystemProperties object, though we may need to rename it.
* chromecast/media/base/key_systems_common.cc, which I think should be using the
new CDM registry or something else other than key system names. This is easy to
solve by duplicating the string if necessary.

Thus, since this code should end up in media/base/, this is probably an okay
step forward, especially since it unblocks other work. I think we should file a
bug to eliminate key_system_names and reference it as appropriate.

We might be able to eliminate kExternalClearKey from the .h file.
chrome/renderer/media/chrome_key_systems.cc and various tests define it
themselves.

We can probably make kClearKey local as well if we stop using it in one place in
key_systems.cc.

https://codereview.chromium.org/1921823003/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1921823003/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:29: const char kClearKeyKeySystem[] =
"org.w3.clearkey";
We already include a file that defines kClearKey. We should eliminate one of
them.

[halliwell, 2016-04-25 23:43:41]

> > Can we suppress this until halliwell@'s work is done?

Seems like it would be fine to land this now without interfering with the
changes I'm working on, especially if it's unblocking other things.

> * chromecast/media/base/key_systems_common.cc, which I think should be using
the
> new CDM registry or something else other than key system names. This is easy
to
> solve by duplicating the string if necessary.

For context, this is used on the browser process side to instantiate the CDM:
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...

Although, we only use it for the ClearKey name, and ClearKey is broken right now
(see bug 441957).

[jrummell, 2016-04-26 00:27:23]

jrummell@chromium.org changed reviewers:
+ halliwell@chromium.org

[jrummell, 2016-04-26 00:27:24]

Opened issue 606579 to track merging key_system_names into key_systems. Also
fixed the duplicated string.

+halliwell@ since I'll need his approval for the chromecast file change.

https://codereview.chromium.org/1921823003/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1921823003/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:29: const char kClearKeyKeySystem[] =
"org.w3.clearkey";
On 2016/04/25 23:25:57, ddorwin wrote:
> We already include a file that defines kClearKey. We should eliminate one of
> them.

Done.

[halliwell, 2016-04-26 02:56:01]

On 2016/04/26 00:27:24, jrummell wrote:
> Opened issue 606579 to track merging key_system_names into key_systems. Also
> fixed the duplicated string.
> 
> +halliwell@ since I'll need his approval for the chromecast file change.
> 
> https://codereview.chromium.org/1921823003/diff/1/media/base/key_systems.cc
> File media/base/key_systems.cc (right):
> 
>
https://codereview.chromium.org/1921823003/diff/1/media/base/key_systems.cc#n...
> media/base/key_systems.cc:29: const char kClearKeyKeySystem[] =
> "org.w3.clearkey";
> On 2016/04/25 23:25:57, ddorwin wrote:
> > We already include a file that defines kClearKey. We should eliminate one of
> > them.
> 
> Done.

chromecast/ lgtm

[sandersd (OOO until July 31), 2016-04-26 17:06:44]

lgtm

[jrummell, 2016-04-26 21:30:35]

Patchset #3 (id:40001) has been deleted

[jrummell, 2016-04-26 21:59:43]

Updated to remove an unnecessary TODO and make the mojo test not depend on the
public char[] (which should go away). Didn't change the chromecast code since
that may change with the refactoring halliwell@ is doing.

[ddorwin, 2016-04-27 19:03:27]

LGTM with comments. I don't think the proposed changes to the chromecast code
are that big or would be affected by any refactoring. It's not a big deal,
though.

https://codereview.chromium.org/1921823003/diff/60001/chromecast/media/base/k...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/1921823003/diff/60001/chromecast/media/base/k...
chromecast/media/base/key_systems_common.cc:33: if
(key_system_name.compare(::media::kClearKey) == 0) {
This could use IsClearKey() instead. Or just duplicate the string.

https://codereview.chromium.org/1921823003/diff/60001/media/base/key_system_n...
File media/base/key_system_names.h (right):

https://codereview.chromium.org/1921823003/diff/60001/media/base/key_system_n...
media/base/key_system_names.h:18: MEDIA_EXPORT extern const char kClearKey[];
We could probably eliminate this by duplicating it in a couple places. This
would reduce the likelihood of others using it.

[jrummell, 2016-04-27 22:42:02]

Thanks for the reviews. Looks like I need to rebase and then it should be good
to go.

https://codereview.chromium.org/1921823003/diff/60001/chromecast/media/base/k...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/1921823003/diff/60001/chromecast/media/base/k...
chromecast/media/base/key_systems_common.cc:33: if
(key_system_name.compare(::media::kClearKey) == 0) {
On 2016/04/27 19:03:27, ddorwin wrote:
> This could use IsClearKey() instead. Or just duplicate the string.

Done.

https://codereview.chromium.org/1921823003/diff/60001/media/base/key_system_n...
File media/base/key_system_names.h (right):

https://codereview.chromium.org/1921823003/diff/60001/media/base/key_system_n...
media/base/key_system_names.h:18: MEDIA_EXPORT extern const char kClearKey[];
On 2016/04/27 19:03:27, ddorwin wrote:
> We could probably eliminate this by duplicating it in a couple places. This
> would reduce the likelihood of others using it.

Done.

[ddorwin, 2016-04-27 22:45:41]

LGTM. Thanks.

halliwell: FYI, there was a minor change to chromecast/

[halliwell, 2016-04-27 22:48:55]

On 2016/04/27 22:45:41, ddorwin wrote:
> LGTM. Thanks.
> 
> halliwell: FYI, there was a minor change to chromecast/

chromecast/ still lgtm.

[jrummell, 2016-04-28 00:53:38]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-04-28 00:53:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, halliwell@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1921823003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-28 00:54:03]

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

[commit-bot: I haz the power, 2016-04-28 02:10:50]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:15:36]

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

[commit-bot: I haz the power, 2016-04-30 17:53:47]

Description was changed from

==========
media: Move key_system_names to media/base/

This file is already used from media/base/ and should be there to
clean up some dependency issues (as noted by gn check).

BUG=599578
TEST=compiles with gn and gyp
==========

to

==========
media: Move key_system_names to media/base/

This file is already used from media/base/ and should be there to
clean up some dependency issues (as noted by gn check).

BUG=599578
TEST=compiles with gn and gyp

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