Media Capabilities: add plumbing between Blink and media/blink/.

Media Capabilities: add plumbing between Blink and media/blink/.

So much yummy plumbing! :)

Also fixing a typo in an IDL.

BUG=690380
R=chcunningham@chromium.org

Review-Url: https://codereview.chromium.org/2706313002
Cr-Commit-Position: refs/heads/master@{#456536}
Committed: https://chromium.googlesource.com/chromium/src/+/fb77ac6ce92d2d64c8ac514a7ad08a32db3c5e82

[mlamouri (slow - plz ping), 2017-02-21 21:14:18]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 21:15:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-02-21 21:17:19]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2017-02-21 21:17:19]

jochen@, PTAL at:
  content/child/blink_platform_impl.cc
  content/child/blink_platform_impl.h
  third_party/WebKit/public/platform/DEPS
  third_party/WebKit/public/platform/Platform.h

chcunningham@, PTAL at:
  media/blink/BUILD.gn
  media/blink/webmediacapabilitiesclient_impl.cc
  media/blink/webmediacapabilitiesclient_impl.h

Both of you should feel free to look at the */modules/media_capabilities/*
changes.

jochen@, I understand that the DEPS change might be controversial. Happy to move
this to public/platform/modules/media_capabilities/. I guess, ideally, I should
not expose base::Optional to Blink but given the types, I'm not sure if it's
worth doing crazy plumbing to use base::Optional outside of Blink and
WTF::Optional inside.

[commit-bot: I haz the power, 2017-02-21 21:22:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 21:22:30]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[haraken, 2017-02-22 00:01:28]

I'm fine with exposing base::Optional to Blink, because WTF::Optional is already
a very thin wrapper of base::Optional.

However, it's a bit unfortunate that you need to expose those dictionaries via
public APIs. Would there be any option to move some parts of media/blink/ into
modules/media_capabilities/ and thus avoid exposing many things via the public
APIs?

[jochen (gone - plz use gerrit), 2017-02-22 08:59:26]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org

[jochen (gone - plz use gerrit), 2017-02-22 08:59:26]

deferring to haraken

[mlamouri (slow - plz ping), 2017-02-24 14:47:25]

On 2017/02/22 at 00:01:28, haraken wrote:
> I'm fine with exposing base::Optional to Blink, because WTF::Optional is
already a very thin wrapper of base::Optional.
> 
> However, it's a bit unfortunate that you need to expose those dictionaries via
public APIs. Would there be any option to move some parts of media/blink/ into
modules/media_capabilities/ and thus avoid exposing many things via the public
APIs?

Unfortunately, we will have to ultimately call a media/base/ class (MediaClient)
that can't be accessed from Blink. The only part of //media allowed to talk to
Blink is media/blink/. Because of this, what we can move to Blink is going to be
limited :(

An alternative would be to create a mojo service to directly poke inside the
//media code base. However, we don't have a need for an out of process service
here so I'm not sure if it would make much sense to do this just to reduce
plumbing. WDYT?

[dglazkov, 2017-02-24 17:58:39]

On 2017/02/24 at 14:47:25, mlamouri wrote:
> On 2017/02/22 at 00:01:28, haraken wrote:
> > I'm fine with exposing base::Optional to Blink, because WTF::Optional is
already a very thin wrapper of base::Optional.
> > 
> > However, it's a bit unfortunate that you need to expose those dictionaries
via public APIs. Would there be any option to move some parts of media/blink/
into modules/media_capabilities/ and thus avoid exposing many things via the
public APIs?
> 
> Unfortunately, we will have to ultimately call a media/base/ class
(MediaClient) that can't be accessed from Blink. The only part of //media
allowed to talk to Blink is media/blink/. Because of this, what we can move to
Blink is going to be limited :(
> 
> An alternative would be to create a mojo service to directly poke inside the
//media code base. However, we don't have a need for an out of process service
here so I'm not sure if it would make much sense to do this just to reduce
plumbing. WDYT?

In an ideal world/architecture, would MediaClient live in blink? As in, per
https://docs.google.com/spreadsheets/d/1VIINt17Dg2cJjPpoJ_HY3HI0uLpidql-1u8pB...,
would it be "in platform/ & use Mojo"? Looking at it briefly, there are Widevine
bits in there and I am not sure how that would work. What I am trying to dig at
here, what's the right end state? Perhaps we if we looked at the end state, it
would be more clear on what the intermediate state could be and that would make
more clear whether this patch advances to that intermediate state or diverges
from it.

[chcunningham, 2017-02-28 01:32:57]

> An alternative would be to create a mojo service to directly poke inside the
> //media code base.

I think this is an interesting idea that could reduce some boiler-plate, but I
share Mounir's concerns. MediaClient will be in the render process for the
foreseeable future. I'm not mojo-saavy enough to know how inefficient it may be
to use mojo services purely as a in-process modularization-tool. Can someone
with more expertise comment?

> would MediaClient live in blink?

I doubt it. MediaClient is a pretty low level Media API and media should not
depend on Blink. Client embedders like Chrome/Cast are simultaneously a media
platforms, offering a custom set of decryption/decoding capabilities which they
describe via MediaClient.

[mlamouri (slow - plz ping), 2017-02-28 18:04:49]

On 2017/02/24 at 17:58:39, dglazkov wrote:
> On 2017/02/24 at 14:47:25, mlamouri wrote:
> > On 2017/02/22 at 00:01:28, haraken wrote:
> > > I'm fine with exposing base::Optional to Blink, because WTF::Optional is
already a very thin wrapper of base::Optional.
> > > 
> > > However, it's a bit unfortunate that you need to expose those dictionaries
via public APIs. Would there be any option to move some parts of media/blink/
into modules/media_capabilities/ and thus avoid exposing many things via the
public APIs?
> > 
> > Unfortunately, we will have to ultimately call a media/base/ class
(MediaClient) that can't be accessed from Blink. The only part of //media
allowed to talk to Blink is media/blink/. Because of this, what we can move to
Blink is going to be limited :(
> > 
> > An alternative would be to create a mojo service to directly poke inside the
//media code base. However, we don't have a need for an out of process service
here so I'm not sure if it would make much sense to do this just to reduce
plumbing. WDYT?
> 
> In an ideal world/architecture, would MediaClient live in blink? As in, per
https://docs.google.com/spreadsheets/d/1VIINt17Dg2cJjPpoJ_HY3HI0uLpidql-1u8pB...,
would it be "in platform/ & use Mojo"? Looking at it briefly, there are Widevine
bits in there and I am not sure how that would work. What I am trying to dig at
here, what's the right end state? Perhaps we if we looked at the end state, it
would be more clear on what the intermediate state could be and that would make
more clear whether this patch advances to that intermediate state or diverges
from it.

MediaClient is in media/base/ which is quite far from Blink. It's not even
allowed to access Blink code (media/blink/) is doing the glue. I don't think
there are any plans to get MediaClient to move to Blink and I don't think this
module should assume this will happen.

One thing that I think would be interesting is to have
third_party/WebKit/platform/media/ instead of //media/blink/ and that directory
would be allowed to access //media/ and
//third_party/WebKit/modules/media_capabilities/. That would reduce the
boilerplate by dropping the need for Web* objects. Basically, in the long run,
//media/blink/ could be deprecated in favour of this. Does that sound
reasonable?

[haraken, 2017-03-01 01:39:57]

On 2017/02/28 18:04:49, mlamouri wrote:
> On 2017/02/24 at 17:58:39, dglazkov wrote:
> > On 2017/02/24 at 14:47:25, mlamouri wrote:
> > > On 2017/02/22 at 00:01:28, haraken wrote:
> > > > I'm fine with exposing base::Optional to Blink, because WTF::Optional is
> already a very thin wrapper of base::Optional.
> > > > 
> > > > However, it's a bit unfortunate that you need to expose those
dictionaries
> via public APIs. Would there be any option to move some parts of media/blink/
> into modules/media_capabilities/ and thus avoid exposing many things via the
> public APIs?
> > > 
> > > Unfortunately, we will have to ultimately call a media/base/ class
> (MediaClient) that can't be accessed from Blink. The only part of //media
> allowed to talk to Blink is media/blink/. Because of this, what we can move to
> Blink is going to be limited :(
> > > 
> > > An alternative would be to create a mojo service to directly poke inside
the
> //media code base. However, we don't have a need for an out of process service
> here so I'm not sure if it would make much sense to do this just to reduce
> plumbing. WDYT?
> > 
> > In an ideal world/architecture, would MediaClient live in blink? As in, per
>
https://docs.google.com/spreadsheets/d/1VIINt17Dg2cJjPpoJ_HY3HI0uLpidql-1u8pB...,
> would it be "in platform/ & use Mojo"? Looking at it briefly, there are
Widevine
> bits in there and I am not sure how that would work. What I am trying to dig
at
> here, what's the right end state? Perhaps we if we looked at the end state, it
> would be more clear on what the intermediate state could be and that would
make
> more clear whether this patch advances to that intermediate state or diverges
> from it.
> 
> MediaClient is in media/base/ which is quite far from Blink. It's not even
> allowed to access Blink code (media/blink/) is doing the glue. I don't think
> there are any plans to get MediaClient to move to Blink and I don't think this
> module should assume this will happen.
> 
> One thing that I think would be interesting is to have
> third_party/WebKit/platform/media/ instead of //media/blink/ and that
directory
> would be allowed to access //media/ and
> //third_party/WebKit/modules/media_capabilities/. That would reduce the
> boilerplate by dropping the need for Web* objects. Basically, in the long run,
> //media/blink/ could be deprecated in favour of this. Does that sound
> reasonable?

This makes a lot of more sense!

[chcunningham, 2017-03-01 18:25:14]

On 2017/03/01 01:39:57, haraken wrote:
> On 2017/02/28 18:04:49, mlamouri wrote:
> > On 2017/02/24 at 17:58:39, dglazkov wrote:
> > > On 2017/02/24 at 14:47:25, mlamouri wrote:
> > > > On 2017/02/22 at 00:01:28, haraken wrote:
> > > > > I'm fine with exposing base::Optional to Blink, because WTF::Optional
is
> > already a very thin wrapper of base::Optional.
> > > > > 
> > > > > However, it's a bit unfortunate that you need to expose those
> dictionaries
> > via public APIs. Would there be any option to move some parts of
media/blink/
> > into modules/media_capabilities/ and thus avoid exposing many things via the
> > public APIs?
> > > > 
> > > > Unfortunately, we will have to ultimately call a media/base/ class
> > (MediaClient) that can't be accessed from Blink. The only part of //media
> > allowed to talk to Blink is media/blink/. Because of this, what we can move
to
> > Blink is going to be limited :(
> > > > 
> > > > An alternative would be to create a mojo service to directly poke inside
> the
> > //media code base. However, we don't have a need for an out of process
service
> > here so I'm not sure if it would make much sense to do this just to reduce
> > plumbing. WDYT?
> > > 
> > > In an ideal world/architecture, would MediaClient live in blink? As in,
per
> >
>
https://docs.google.com/spreadsheets/d/1VIINt17Dg2cJjPpoJ_HY3HI0uLpidql-1u8pB...,
> > would it be "in platform/ & use Mojo"? Looking at it briefly, there are
> Widevine
> > bits in there and I am not sure how that would work. What I am trying to dig
> at
> > here, what's the right end state? Perhaps we if we looked at the end state,
it
> > would be more clear on what the intermediate state could be and that would
> make
> > more clear whether this patch advances to that intermediate state or
diverges
> > from it.
> > 
> > MediaClient is in media/base/ which is quite far from Blink. It's not even
> > allowed to access Blink code (media/blink/) is doing the glue. I don't think
> > there are any plans to get MediaClient to move to Blink and I don't think
this
> > module should assume this will happen.
> > 
> > One thing that I think would be interesting is to have
> > third_party/WebKit/platform/media/ instead of //media/blink/ and that
> directory
> > would be allowed to access //media/ and
> > //third_party/WebKit/modules/media_capabilities/. That would reduce the
> > boilerplate by dropping the need for Web* objects. Basically, in the long
run,
> > //media/blink/ could be deprecated in favour of this. Does that sound
> > reasonable?
> 
> This makes a lot of more sense!

SGTM

[mlamouri (slow - plz ping), 2017-03-07 11:25:11]

Description was changed from

==========
Media Capabilities: add plumbing between Blink and media/blink/.

So much yummy plumbing! :)

Also fixing a typo in an IDL.

BUG=690380
R=chcunningham@chromium.org

public interfaces
==========

to

==========
Media Capabilities: add plumbing between Blink and media/blink/.

So much yummy plumbing! :)

Also fixing a typo in an IDL.

BUG=690380
R=chcunningham@chromium.org
==========

[mlamouri (slow - plz ping), 2017-03-07 11:25:57]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 11:26:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 11:29:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 11:29:21]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2017-03-07 11:38:32]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 11:38:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 12:42:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 12:42:55]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[haraken, 2017-03-07 12:45:02]

I chatted with mlamouri offline and understood that it's hard to remove the Web*
abstractions.

- VideoConfiguration must exist in modules/ because VideoConfiguration is
defined in a WebIDL.

- /media/* must exist in Chromium (even though we can move /media/blink/ to
Blink) because low-level media things need to be implemented in Chromium (they
cannot be moved to Blink).

- We need to pass the VideoConfiguration from modules/ to /media/*. At this
point, we cannot avoid introducing Web* types.

LGTM.

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.h
(right):

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.h:24:
using WebType = std::unique_ptr<WebMediaDecodingAbility>;

Is this used?

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.h:25:
static MediaDecodingAbility* take(ScriptPromiseResolver*,

Is the ScriptPromiseResolver* parameter needed?

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaCapabilitiesQueryCallbacks.h
(right):

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaCapabilitiesQueryCallbacks.h:15:
WebCallbacks<std::unique_ptr<WebMediaDecodingAbility>, void>;

I'd put this in WebMediaDecodingAbility.h.

[mlamouri (slow - plz ping), 2017-03-07 14:22:17]

Review comments applied.

jochen@, PTAL at content/
chcunningham@, PTAL at media/

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.h
(right):

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.h:24:
using WebType = std::unique_ptr<WebMediaDecodingAbility>;
On 2017/03/07 at 12:45:02, haraken wrote:
> Is this used?

This is used by CallbackPromiseAdapter in MediaCapabilities.cpp

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.h:25:
static MediaDecodingAbility* take(ScriptPromiseResolver*,
On 2017/03/07 at 12:45:02, haraken wrote:
> Is the ScriptPromiseResolver* parameter needed?

ditto

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaCapabilitiesQueryCallbacks.h
(right):

https://codereview.chromium.org/2706313002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaCapabilitiesQueryCallbacks.h:15:
WebCallbacks<std::unique_ptr<WebMediaDecodingAbility>, void>;
On 2017/03/07 at 12:45:02, haraken wrote:
> I'd put this in WebMediaDecodingAbility.h.

Done.

[chcunningham, 2017-03-08 02:43:10]

Looks pretty good!

> We need to pass the VideoConfiguration from modules/ to /media/*. At this
point, we cannot avoid introducing Web* types.

I'm curious if the roadmap for blink's onion soup effort will allow us to avoid
this later on?

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.cpp
(right):

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.cpp:14:
return new MediaDecodingAbility(std::move(webMediaDecodingAbility));
I notice this class breaks the pattern of the *Configuration classes where we
are nesting the Web* object here rather than doing a converter function. Perhaps
thats standard practice for objects that originate in blink/platform and bubble
up - but figured I'd ask.

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaConfiguration.h
(right):

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaConfiguration.h:25:
base::Optional<WebAudioConfiguration> audioConfiguration;
Do you mean for these to be optional?

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/media_capabilities/WebVideoConfiguration.h
(right):

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/media_capabilities/WebVideoConfiguration.h:16:
WebString contentType;
I think the pattern for other media contentType passing is to split up the
"audio/webm" from the "codecs='opus'" prior to pushing into the blink platform
layer (and further). For e.g. HTMLMediaElement does this with canPlayType here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...

Another view of this is from media/'s MimeUtil, where codecs and container are
already separated.
https://cs.chromium.org/chromium/src/media/base/mime_util.h?rcl=15e2840426713...

[jochen (gone - plz use gerrit), 2017-03-08 18:52:30]

lgtm

[mlamouri (slow - plz ping), 2017-03-10 11:25:39]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 11:25:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-03-10 11:26:42]

chcunningham@, Onion Soup will unfortunately not help with the issues we have.

PTAL

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.cpp
(right):

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaDecodingAbility.cpp:14:
return new MediaDecodingAbility(std::move(webMediaDecodingAbility));
On 2017/03/08 at 02:43:10, chcunningham wrote:
> I notice this class breaks the pattern of the *Configuration classes where we
are nesting the Web* object here rather than doing a converter function. Perhaps
thats standard practice for objects that originate in blink/platform and bubble
up - but figured I'd ask.

That's following the pattern of CallbackPromiseAdapter. ::take() is called with
::WebType automatically,

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaConfiguration.h
(right):

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/media_capabilities/WebMediaConfiguration.h:25:
base::Optional<WebAudioConfiguration> audioConfiguration;
On 2017/03/08 at 02:43:10, chcunningham wrote:
> Do you mean for these to be optional?

Yes. Per spec, one of them must be present but video only is fine such as audio
only.

Added comment about this.

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/media_capabilities/WebVideoConfiguration.h
(right):

https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/media_capabilities/WebVideoConfiguration.h:16:
WebString contentType;
On 2017/03/08 at 02:43:10, chcunningham wrote:
> I think the pattern for other media contentType passing is to split up the
"audio/webm" from the "codecs='opus'" prior to pushing into the blink platform
layer (and further). For e.g. HTMLMediaElement does this with canPlayType here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...
> 
> Another view of this is from media/'s MimeUtil, where codecs and container are
already separated.
>
https://cs.chromium.org/chromium/src/media/base/mime_util.h?rcl=15e2840426713...

I don't think it really matters where the check is made at the moment. I think
it would make more sense for the MimeUtil to implement the codec splitting logic
to avoid duplication. Though, we might need to reject if the `codec` parameter
isn't present. I would say that changing the struct will have a low cost and we
can revise this later. WDYT?

[commit-bot: I haz the power, 2017-03-10 11:52:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 11:52:16]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[chcunningham, 2017-03-13 17:04:52]

LGTM 

>
https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
> File
>
third_party/WebKit/public/platform/modules/media_capabilities/WebVideoConfiguration.h
> (right):
> 
>
https://codereview.chromium.org/2706313002/diff/60001/third_party/WebKit/publ...
>
third_party/WebKit/public/platform/modules/media_capabilities/WebVideoConfiguration.h:16:
> WebString contentType;
> On 2017/03/08 at 02:43:10, chcunningham wrote:
> > I think the pattern for other media contentType passing is to split up the
> "audio/webm" from the "codecs='opus'" prior to pushing into the blink platform
> layer (and further). For e.g. HTMLMediaElement does this with canPlayType
here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...
> > 
> > Another view of this is from media/'s MimeUtil, where codecs and container
are
> already separated.
> >
>
https://cs.chromium.org/chromium/src/media/base/mime_util.h?rcl=15e2840426713...
> 
> I don't think it really matters where the check is made at the moment. I think
> it would make more sense for the MimeUtil to implement the codec splitting
logic
> to avoid duplication. Though, we might need to reject if the `codec` parameter
> isn't present. I would say that changing the struct will have a low cost and
we
> can revise this later. WDYT?

Sounds good.

[mlamouri (slow - plz ping), 2017-03-13 18:25:18]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-03-13 18:25:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jochen@chromium.org, chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/2706313002/#ps100001
(title: "fix win compile")

[commit-bot: I haz the power, 2017-03-13 18:26:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 18:58:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 18:58:21]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[mlamouri (slow - plz ping), 2017-03-13 20:56:22]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-03-13 20:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, haraken@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2706313002/#ps120001
(title: "oups")

[commit-bot: I haz the power, 2017-03-13 20:57:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 23:22:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489438582824240,
"parent_rev": "712524e1b297823b7f50ba08ebb4397b7a781253", "commit_rev":
"fb77ac6ce92d2d64c8ac514a7ad08a32db3c5e82"}

[commit-bot: I haz the power, 2017-03-13 23:23:09]

Description was changed from

==========
Media Capabilities: add plumbing between Blink and media/blink/.

So much yummy plumbing! :)

Also fixing a typo in an IDL.

BUG=690380
R=chcunningham@chromium.org
==========

to

==========
Media Capabilities: add plumbing between Blink and media/blink/.

So much yummy plumbing! :)

Also fixing a typo in an IDL.

BUG=690380
R=chcunningham@chromium.org

Review-Url: https://codereview.chromium.org/2706313002
Cr-Commit-Position: refs/heads/master@{#456536}
Committed:
https://chromium.googlesource.com/chromium/src/+/fb77ac6ce92d2d64c8ac514a7ad0...
==========

[commit-bot: I haz the power, 2017-03-13 23:23:10]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/fb77ac6ce92d2d64c8ac514a7ad0...

[agrieve, 2017-03-16 14:25:05]

On 2017/03/13 23:23:10, commit-bot: I haz the power wrote:
> Committed patchset #7 (id:120001) as
>
https://chromium.googlesource.com/chromium/src/+/fb77ac6ce92d2d64c8ac514a7ad0...

FYI - this commit added 16kb to the native library on Android. A bit sad for
"plumbing", so thought I'd check in to see if this was expected?

Graph:
https://chromeperf.appspot.com/report?sid=5d55db44beb13bb1e813882381fe82b47aa...

[mlamouri (slow - plz ping), 2017-03-16 14:29:34]

On 2017/03/16 at 14:25:05, agrieve wrote:
> On 2017/03/13 23:23:10, commit-bot: I haz the power wrote:
> > Committed patchset #7 (id:120001) as
> >
https://chromium.googlesource.com/chromium/src/+/fb77ac6ce92d2d64c8ac514a7ad0...
> 
> FYI - this commit added 16kb to the native library on Android. A bit sad for
"plumbing", so thought I'd check in to see if this was expected?
> 
> Graph:
https://chromeperf.appspot.com/report?sid=5d55db44beb13bb1e813882381fe82b47aa...

Unless the addition of //media/blinks in the deps for content/child/ could have
this effect, I have no idea why this would happen. There isn't much code in
here.

[mlamouri (slow - plz ping), 2017-03-16 14:40:25]

On 2017/03/16 at 14:29:34, mlamouri wrote:
> On 2017/03/16 at 14:25:05, agrieve wrote:
> > On 2017/03/13 23:23:10, commit-bot: I haz the power wrote:
> > > Committed patchset #7 (id:120001) as
> > >
https://chromium.googlesource.com/chromium/src/+/fb77ac6ce92d2d64c8ac514a7ad0...
> > 
> > FYI - this commit added 16kb to the native library on Android. A bit sad for
"plumbing", so thought I'd check in to see if this was expected?
> > 
> > Graph:
https://chromeperf.appspot.com/report?sid=5d55db44beb13bb1e813882381fe82b47aa...
> 
> Unless the addition of //media/blinks in the deps for content/child/ could
have this effect, I have no idea why this would happen. There isn't much code in
here.

Maybe we should discuss this on a bug though?