Encrypted Media: No NeedKey if --enable-encrypted-media is not set.

Encrypted Media: No NeedKey if --enable-encrypted-media is not set.

When --enable-encrypted-media is not set, NeedKey event should not be fired from media pipeline. Also, the decoder should declare decode error or unsupported error instead of waiting for decryptor to be set/initialized.

BUG=
TEST=tested EME test page with --enable-encrypted-media set and not set.


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

[xhwang, 2012-11-23 08:03:44]

PTAL!

[scherkus (not reviewing), 2012-11-27 23:17:42]

https://codereview.chromium.org/11414138/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11414138/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:191: if (encrypted_media_enabled) {
I believe you can simplify a lot of this CL by:
  - Defining kEnableEncryptedMedia in webkit/ instead of content/
  - Call CommandLine::ForCurrentProcess()->HasSwitch() here

Would that work?

[xhwang, 2012-11-29 02:24:14]

PTAL!

https://codereview.chromium.org/11414138/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11414138/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:191: if (encrypted_media_enabled) {
On 2012/11/27 23:17:43, scherkus wrote:
> I believe you can simplify a lot of this CL by:
>   - Defining kEnableEncryptedMedia in webkit/ instead of content/
>   - Call CommandLine::ForCurrentProcess()->HasSwitch() here
> 
> Would that work?

Done.

[scherkus (not reviewing), 2012-11-29 02:48:00]

+stuartmorgan for chrome/browser/DEPS question

https://codereview.chromium.org/11414138/diff/4001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/11414138/diff/4001/chrome/browser/DEPS#newcode43
chrome/browser/DEPS:43: "+webkit/media",
stuartmorgan: can you comment on our usage?

it's for a flag to enable a web feature at runtime so it _seems_ that'd be in
line w/ the bug mentioning permitting chrome/renderer/ usage (unless I
misunderstood what you mean there!)

perhaps we can tighten up the DEPS to only the media_switches.cc file?

https://codereview.chromium.org/11414138/diff/4001/webkit/media/media_switche...
File webkit/media/media_switches.cc (right):

https://codereview.chromium.org/11414138/diff/4001/webkit/media/media_switche...
webkit/media/media_switches.cc:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
year

https://codereview.chromium.org/11414138/diff/4001/webkit/media/media_switches.h
File webkit/media/media_switches.h (right):

https://codereview.chromium.org/11414138/diff/4001/webkit/media/media_switche...
webkit/media/media_switches.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
year

[stuartmorgan, 2012-11-29 09:07:35]

On 2012/11/29 02:48:00, scherkus wrote:
> it's for a flag to enable a web feature at runtime so it _seems_ that'd be in
> line w/ the bug mentioning permitting chrome/renderer/ usage (unless I
> misunderstood what you mean there!)

It means the chrome/renderer/ directory. Dependencies from chrome/browser are
not in line with that :)

iOS builds only the user agent code out of webkit, and even that will be moved
out at some point; the decision was that iOS should not build anything in
webkit/. If you add references to constants in webkit/media/ to files in
chrome/browser that iOS builds, you'll break the iOS build. That's part of the
motivation for locking down DEPS.

Also, my understanding is that webkit/ is deprecated in general, so beyond the
dependency issues I suspect we don't want to add new files here at all. +Darin
to weigh in.

[xhwang, 2012-11-29 16:52:18]

On 2012/11/29 09:07:35, stuartmorgan wrote:
> On 2012/11/29 02:48:00, scherkus wrote:
> > it's for a flag to enable a web feature at runtime so it _seems_ that'd be
in
> > line w/ the bug mentioning permitting chrome/renderer/ usage (unless I
> > misunderstood what you mean there!)
> 
> It means the chrome/renderer/ directory. Dependencies from chrome/browser are
> not in line with that :)
> 
> iOS builds only the user agent code out of webkit, and even that will be moved
> out at some point; the decision was that iOS should not build anything in
> webkit/. If you add references to constants in webkit/media/ to files in
> chrome/browser that iOS builds, you'll break the iOS build. That's part of the
> motivation for locking down DEPS.
> 
> Also, my understanding is that webkit/ is deprecated in general, so beyond the
> dependency issues I suspect we don't want to add new files here at all. +Darin
> to weigh in.

This flag is used in webkit/media. But if this change in chrome/browser/DEPS
doesn't make sense, I can move the flag declaration/definition to
media/base/media_switches.* and include it from webkit/media.

[scherkus (not reviewing), 2012-11-29 18:08:26]

On 2012/11/29 16:52:18, xhwang wrote:
> On 2012/11/29 09:07:35, stuartmorgan wrote:
> > On 2012/11/29 02:48:00, scherkus wrote:
> > > it's for a flag to enable a web feature at runtime so it _seems_ that'd be
> in
> > > line w/ the bug mentioning permitting chrome/renderer/ usage (unless I
> > > misunderstood what you mean there!)
> > 
> > It means the chrome/renderer/ directory. Dependencies from chrome/browser
are
> > not in line with that :)
> > 
> > iOS builds only the user agent code out of webkit, and even that will be
moved
> > out at some point; the decision was that iOS should not build anything in
> > webkit/. If you add references to constants in webkit/media/ to files in
> > chrome/browser that iOS builds, you'll break the iOS build. That's part of
the
> > motivation for locking down DEPS.
> > 
> > Also, my understanding is that webkit/ is deprecated in general, so beyond
the
> > dependency issues I suspect we don't want to add new files here at all.
+Darin
> > to weigh in.
> 
> This flag is used in webkit/media. But if this change in chrome/browser/DEPS
> doesn't make sense, I can move the flag declaration/definition to
> media/base/media_switches.* and include it from webkit/media.

Thanks for the explanation stuartmorgan!

xhwang: that seems OK. chrome/browser/DEPS already specifies media/base/ for
media switches (although perhaps we should tighten up that dependency to only
list that file -- it's also not entirely clear if chrome/browser/ should be
depending on media/base either :P)

[xhwang, 2012-11-29 19:48:06]

On 2012/11/29 18:08:26, scherkus wrote:
> On 2012/11/29 16:52:18, xhwang wrote:
> > On 2012/11/29 09:07:35, stuartmorgan wrote:
> > > On 2012/11/29 02:48:00, scherkus wrote:
> > > > it's for a flag to enable a web feature at runtime so it _seems_ that'd
be
> > in
> > > > line w/ the bug mentioning permitting chrome/renderer/ usage (unless I
> > > > misunderstood what you mean there!)
> > > 
> > > It means the chrome/renderer/ directory. Dependencies from chrome/browser
> are
> > > not in line with that :)
> > > 
> > > iOS builds only the user agent code out of webkit, and even that will be
> moved
> > > out at some point; the decision was that iOS should not build anything in
> > > webkit/. If you add references to constants in webkit/media/ to files in
> > > chrome/browser that iOS builds, you'll break the iOS build. That's part of
> the
> > > motivation for locking down DEPS.
> > > 
> > > Also, my understanding is that webkit/ is deprecated in general, so beyond
> the
> > > dependency issues I suspect we don't want to add new files here at all.
> +Darin
> > > to weigh in.
> > 
> > This flag is used in webkit/media. But if this change in chrome/browser/DEPS
> > doesn't make sense, I can move the flag declaration/definition to
> > media/base/media_switches.* and include it from webkit/media.
> 
> Thanks for the explanation stuartmorgan!
> 
> xhwang: that seems OK. chrome/browser/DEPS already specifies media/base/ for
> media switches (although perhaps we should tighten up that dependency to only
> list that file -- it's also not entirely clear if chrome/browser/ should be
> depending on media/base either :P)

Done. PTAL again!

[scherkus (not reviewing), 2012-11-29 20:50:36]

lgtm

[xhwang, 2012-11-29 21:23:08]

jam@: could you please do an OWNERS review on content/ ?

[jam, 2012-11-29 21:55:38]

lgtm

https://codereview.chromium.org/11414138/diff/7003/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/11414138/diff/7003/content/browser/DEPS#newcode7
content/browser/DEPS:7: "+media/base/media_switches.h",  # For
switches::kEnableEncryptedMedia.
nit: just make this "+media/base" and combine with above line

[xhwang, 2012-11-29 22:17:05]

https://codereview.chromium.org/11414138/diff/7003/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/11414138/diff/7003/content/browser/DEPS#newcode7
content/browser/DEPS:7: "+media/base/media_switches.h",  # For
switches::kEnableEncryptedMedia.
On 2012/11/29 21:55:38, John Abd-El-Malek wrote:
> nit: just make this "+media/base" and combine with above line

Done.

[commit-bot: I haz the power, 2012-11-29 22:17:38]

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

[commit-bot: I haz the power, 2012-11-29 22:59:06]

Retried try job too often on linux_rel for step(s) media_unittests

[commit-bot: I haz the power, 2012-11-30 06:30:27]

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

[commit-bot: I haz the power, 2012-11-30 08:28:34]

Change committed as 170422