MediaDrm: Only apply Widevine security levels to the Widevine key system.

MediaDrm: Only apply Widevine security levels to the Widevine key system.

BUG=459400
Committed: https://crrev.com/fd8af57f621326c5edce78077924135c14eb15dd
Cr-Commit-Position: refs/heads/master@{#316930}

[ddorwin, 2015-02-18 02:16:18]

Patchset #1 (id:1) has been deleted

[ddorwin, 2015-02-18 02:17:03]

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

[ddorwin, 2015-02-18 02:19:25]

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

[ddorwin, 2015-02-18 02:19:26]

qinmin: PTAL

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
File media/base/android/browser_cdm_factory_android.cc (right):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
media/base/android/browser_cdm_factory_android.cc:52:
switches::kMediaDrmEnableNonCompositing)) {
I believe all platforms where this path is relevant already specify this by
default.

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:637: // For other key systems, assume
true.
It appears that we used to always set L1*, so this is consistent with the
previous behavior*.

* On platforms that set kMediaDrmEnableNonCompositing.

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.h (left):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.h:45: static bool
IsSecurityLevelSupported(const std::string& key_system,
Does not appear to be used except in the tests, which I removed. Was this
intended to only be used for tests?

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge_unittest.cc (left):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:51: return
MediaDrmBridge::IsSecurityLevelSupported(key_system, security_level);
This is the only place this function is used. Does it only exist for testing or
is it okay to remove?

[qinmin, 2015-02-18 06:45:40]

lgtm

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
File media/base/android/browser_cdm_factory_android.cc (right):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
media/base/android/browser_cdm_factory_android.cc:42:
switches::kMediaDrmEnableNonCompositing)) {
4 space?

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
media/base/android/browser_cdm_factory_android.cc:52:
switches::kMediaDrmEnableNonCompositing)) {
ditto?

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge_unittest.cc (left):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:51: return
MediaDrmBridge::IsSecurityLevelSupported(key_system, security_level);
On 2015/02/18 02:19:26, ddorwin wrote:
> This is the only place this function is used. Does it only exist for testing
or
> is it okay to remove?

i am ok to remove these tests. The function is used to test whether the bots
support widevine or not, and nothing else.

[ddorwin, 2015-02-18 17:30:53]

New patchsets have been uploaded after l-g-t-m from qinmin@chromium.org

[ddorwin, 2015-02-18 17:33:54]

Patchset #2 (id:40001) has been deleted

[ddorwin, 2015-02-18 17:50:15]

Patchset #2 (id:60001) has been deleted

[qinmin, 2015-02-18 17:53:03]

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
File media/base/android/media_drm_bridge_unittest.cc (right):

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:99:
EXPECT_TRUE(media_drm_bridge->SetSecurityLevel(kL3));
have you tested this on ICS devices?

[ddorwin, 2015-02-18 18:11:51]

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
File media/base/android/browser_cdm_factory_android.cc (right):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
media/base/android/browser_cdm_factory_android.cc:42:
switches::kMediaDrmEnableNonCompositing)) {
On 2015/02/18 06:45:40, qinmin wrote:
> 4 space?

Clang formatted.

https://codereview.chromium.org/930413003/diff/20001/media/base/android/brows...
media/base/android/browser_cdm_factory_android.cc:52:
switches::kMediaDrmEnableNonCompositing)) {
On 2015/02/18 06:45:40, qinmin wrote:
> ditto?

Clang formatted.

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge_unittest.cc (left):

https://codereview.chromium.org/930413003/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:51: return
MediaDrmBridge::IsSecurityLevelSupported(key_system, security_level);
On 2015/02/18 06:45:40, qinmin wrote:
> On 2015/02/18 02:19:26, ddorwin wrote:
> > This is the only place this function is used. Does it only exist for testing
> or
> > is it okay to remove?
> 
> i am ok to remove these tests. The function is used to test whether the bots
> support widevine or not, and nothing else.

They were also covering setting security levels for Widevine, so I added a test
to specifically cover that.

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
File media/base/android/media_drm_bridge_unittest.cc (right):

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:79: TEST(MediaDrmBridgeTest,
CreateWithoutSessionSupport_Widevine) {
These two tests are new.

Note: CreateWithoutSessionSupport is really just a convenient wrapper around
Create and does what we would want to do anyway.

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:89: TEST(MediaDrmBridgeTest,
SetSecurityLevel_Widevine) {
This test covers the only remaining useful part of  the removed
IsSecurityLevelSupported tests - setting various levels for Widevine.

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:99:
EXPECT_TRUE(media_drm_bridge->SetSecurityLevel(kL3));
On 2015/02/18 17:53:03, qinmin wrote:
> have you tested this on ICS devices?

ICS devices should exit at line 94, right? This is just the test from Base's
line 58.

[qinmin, 2015-02-18 21:32:51]

lgtm

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
File media/base/android/media_drm_bridge_unittest.cc (right):

https://codereview.chromium.org/930413003/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge_unittest.cc:99:
EXPECT_TRUE(media_drm_bridge->SetSecurityLevel(kL3));
On 2015/02/18 18:11:50, ddorwin wrote:
> On 2015/02/18 17:53:03, qinmin wrote:
> > have you tested this on ICS devices?
> 
> ICS devices should exit at line 94, right? This is just the test from Base's
> line 58.

I see. Sorry i missed that.

[ddorwin, 2015-02-18 23:46:11]

The CQ bit was checked by ddorwin@chromium.org

[commit-bot: I haz the power, 2015-02-18 23:47:50]

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

[commit-bot: I haz the power, 2015-02-19 00:21:34]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-02-19 00:22:38]

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