Extract supported codecs from CDM component manifest.

Extract supported codecs from CDM component manifest.

Look for "x-cdm-codecs" in the manifest and set its value in
WebPluginInfo::WebPluginMimeType::additional_param_* where it can be used to
determine the types supported.

The manifest is read when registering existing components in addition to new
installs so that the manifest information is always available.

Also updated the non-component path so the behavior is consistent for the
consumer, which will be updated in a separate CL.

BUG=311370
TEST=canPlayType() with various manifest configurations

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

[ddorwin, 2013-10-24 22:17:03]

xhwang, please review everything.
waffles, please review chrome/browser/component_updater, especially the shared
code.

https://codereview.chromium.org/42003002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/42003002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.h:203: // Deserialize
the CRX manifest. The top level must be a dictionary.
Is there a better place to put this utility function that's needed in multiple
places? (I put it here because CUS is used throughout this directory.)

[waffles, 2013-10-24 23:07:06]

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.cc:144: // to
FinishRegistration()?
There was the thought that the installer would be the authority on what version
was installed - fingerprint is a stronger identifier than version, so it was
assumed that the installer would need to know about that as well, but in
practice all it does it hand it off to CUS for update checks, as you observe.

It is fine to pass current_fingerprint_ and current_manifest_ to
FinishRegistration rather than save it.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.cc:195: // to UI in
Install()?
The thinking was that people would likely want to manipulate those files, e.g.
read the manifest.

What's the motivation for reading the manifest in DefaultComponentInstaller
instead of in Widevine's ComponentReady?

[ddorwin, 2013-10-25 00:06:05]

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.cc:144: // to
FinishRegistration()?
On 2013/10/24 23:07:06, waffles wrote:
> There was the thought that the installer would be the authority on what
version
> was installed - fingerprint is a stronger identifier than version, so it was
> assumed that the installer would need to know about that as well, but in
> practice all it does it hand it off to CUS for update checks, as you observe.
> 
> It is fine to pass current_fingerprint_ and current_manifest_ to
> FinishRegistration rather than save it.

Okay, I'll change the TODO to be for me to remove them.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.cc:195: // to UI in
Install()?
On 2013/10/24 23:07:06, waffles wrote:
> The thinking was that people would likely want to manipulate those files, e.g.
> read the manifest.

Assuming we're going to pass in the manifest like we do now, does it make sense
to call ComponentReady() on the UI thread, which it is already on in most cases,
and let individual Traits post if necessary?
> 
> What's the motivation for reading the manifest in DefaultComponentInstaller
> instead of in Widevine's ComponentReady?

Responsibility separation and consistency. The Traits interface and the WV
subclass don't know about the manifest location, name, format, etc., and it
doesn't make sense to duplicate that knowledge in each subclass. In the install
case, the DictionaryValue is passed to the Traits (because the manifest has
already been parsed). We could parse it in the WV class, but that would mean
unnecessarily re-parsing it in the install case. That is clearly much less
frequent of a case, but it still feels like the wrong design.

The downside is that we may unnecessarily read and parse the manifest for other
components. I originally made the parsing optional based on a Traits property,
but then I found we were already reading the fingerprint from disk for every
component. I decided to KISS and worry about optimization later if necessary.
(Currently there are no other subclasses.)

[waffles, 2013-10-25 16:53:41]

https://codereview.chromium.org/42003002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/42003002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.h:203: // Deserialize
the CRX manifest. The top level must be a dictionary.
On 2013/10/24 22:17:03, ddorwin wrote:
> Is there a better place to put this utility function that's needed in multiple
> places? (I put it here because CUS is used throughout this directory.)

component_updater_service.h is (supposed to be) the public interface to this
module; but I agree that component_unpacker.h doesn't really seem appropriate
either.

More generally I wish we had a class dedicated to understanding .crx items and
the various format details surrounding them. Until that day I would rather have
this in component_unpacker.h or component_updater_service_internal.h than in
component_updater_service.h; cpu@ might have an opinion here, too. (You'll need
him for OWNERS review.)

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.cc:195: // to UI in
Install()?
I'm OK with passing the manifest; it's fine with me to post to UI, too.

It's true that Widevine is the only user of this code, but PNaCl will be soon
(next week?), now that it is unblocked on on-demand.

On 2013/10/25 00:06:05, ddorwin wrote:
> On 2013/10/24 23:07:06, waffles wrote:
> > The thinking was that people would likely want to manipulate those files,
e.g.
> > read the manifest.
> 
> Assuming we're going to pass in the manifest like we do now, does it make
sense
> to call ComponentReady() on the UI thread, which it is already on in most
cases,
> and let individual Traits post if necessary?
> > 
> > What's the motivation for reading the manifest in DefaultComponentInstaller
> > instead of in Widevine's ComponentReady?
> 
> Responsibility separation and consistency. The Traits interface and the WV
> subclass don't know about the manifest location, name, format, etc., and it
> doesn't make sense to duplicate that knowledge in each subclass. In the
install
> case, the DictionaryValue is passed to the Traits (because the manifest has
> already been parsed). We could parse it in the WV class, but that would mean
> unnecessarily re-parsing it in the install case. That is clearly much less
> frequent of a case, but it still feels like the wrong design.
> 
> The downside is that we may unnecessarily read and parse the manifest for
other
> components. I originally made the parsing optional based on a Traits property,
> but then I found we were already reading the fingerprint from disk for every
> component. I decided to KISS and worry about optimization later if necessary.
> (Currently there are no other subclasses.)

[xhwang, 2013-10-25 17:33:21]

Looking pretty good. Just a few comments.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_unpacker.cc:178: if
(!installer->Install(*manifest, unpack_path_)) {
Can we change the Install() to take scoped_ptr<base::DictionaryValue>, then we
can avoid some deepcopy downstream. Unfortunately this means that we need to
change ComponentInstaller and all it's implementors.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:1033: return
static_cast<base::DictionaryValue*>(root.release());
hmm, It'll be nice if we can return a scoped_ptr here. But we need to downcast
which isn't supported in scoped_ptr, so we'll have:

return
scoped_ptr<base::DictionaryValue>(static_cast<base::DictionaryValue*>(root.release()))

Or we can do something like:

base::DictionaryValue* dictionary;
if (!root->GetAsDictionary(dictionary))
  return NULL;

root.release();
return scoped_ptr<base::DictionaryValue>(dictionary);

But this is still not satisfactory... Thoughts?

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:203: // Deserialize
the CRX manifest. The top level must be a dictionary.
s/Deserialize/Deserializes

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:204:
base::DictionaryValue* ReadComponentManifest(const base::FilePath& unpack_path);
who owns the returned dictionary value?

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.h:54: //
|install_dir| is the path to the install directory for this version.
comment order matches param order.

https://codereview.chromium.org/42003002/diff/50001/chrome/common/chrome_cont...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/common/chrome_cont...
chrome/common/chrome_content_client.cc:296: base::ASCIIToUTF16(",") +
do we have comments anywhere about the format? e.g. delimited with ","

https://codereview.chromium.org/42003002/diff/50001/chrome/common/chrome_cont...
chrome/common/chrome_content_client.cc:303: #if
defined(WIDEVINE_CDM_AVC1_SUPPORT_AVAILABLE)
Per offline discussion, add a TODO about renaming the macros?

[ddorwin, 2013-10-25 19:03:39]

cpu, please OWNERS review component_updater/.
thestig, please OWNERS review the one file in chrome/common/.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_unpacker.cc:178: if
(!installer->Install(*manifest, unpack_path_)) {
On 2013/10/25 17:33:22, xhwang wrote:
> Can we change the Install() to take scoped_ptr<base::DictionaryValue>, then we
> can avoid some deepcopy downstream. Unfortunately this means that we need to
> change ComponentInstaller and all it's implementors.

Added TODO in other file. I don't want to add more files to this CL.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:1033: return
static_cast<base::DictionaryValue*>(root.release());
On 2013/10/25 17:33:22, xhwang wrote:
> hmm, It'll be nice if we can return a scoped_ptr here. But we need to downcast
> which isn't supported in scoped_ptr, so we'll have:
> 
> return
>
scoped_ptr<base::DictionaryValue>(static_cast<base::DictionaryValue*>(root.release()))
> 
> Or we can do something like:
> 
> base::DictionaryValue* dictionary;
> if (!root->GetAsDictionary(dictionary))
>   return NULL;
> 
> root.release();
> return scoped_ptr<base::DictionaryValue>(dictionary);
> 
> But this is still not satisfactory... Thoughts?

I independently came up with that same long return statement (+ .Pass()). It's
not great, but from an API perspective, it's better to return the scoped_ptr.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:203: // Deserialize
the CRX manifest. The top level must be a dictionary.
On 2013/10/25 17:33:22, xhwang wrote:
> s/Deserialize/Deserializes

Done.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:204:
base::DictionaryValue* ReadComponentManifest(const base::FilePath& unpack_path);
On 2013/10/25 17:33:22, xhwang wrote:
> who owns the returned dictionary value?

Done.

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.cc:195: // to UI in
Install()?
On 2013/10/25 16:53:42, waffles wrote:
> I'm OK with passing the manifest; it's fine with me to post to UI, too.
Updated TODO.
> 
> It's true that Widevine is the only user of this code, but PNaCl will be soon
> (next week?), now that it is unblocked on on-demand.
Do you have an indication of what they intend to do in ComponentReady? Maybe
I'll wait and see before addressing the TODO.
> 
> On 2013/10/25 00:06:05, ddorwin wrote:
> > On 2013/10/24 23:07:06, waffles wrote:
> > > The thinking was that people would likely want to manipulate those files,
> e.g.
> > > read the manifest.
> > 
> > Assuming we're going to pass in the manifest like we do now, does it make
> sense
> > to call ComponentReady() on the UI thread, which it is already on in most
> cases,
> > and let individual Traits post if necessary?
> > > 
> > > What's the motivation for reading the manifest in
DefaultComponentInstaller
> > > instead of in Widevine's ComponentReady?
> > 
> > Responsibility separation and consistency. The Traits interface and the WV
> > subclass don't know about the manifest location, name, format, etc., and it
> > doesn't make sense to duplicate that knowledge in each subclass. In the
> install
> > case, the DictionaryValue is passed to the Traits (because the manifest has
> > already been parsed). We could parse it in the WV class, but that would mean
> > unnecessarily re-parsing it in the install case. That is clearly much less
> > frequent of a case, but it still feels like the wrong design.
> > 
> > The downside is that we may unnecessarily read and parse the manifest for
> other
> > components. I originally made the parsing optional based on a Traits
property,
> > but then I found we were already reading the fingerprint from disk for every
> > component. I decided to KISS and worry about optimization later if
necessary.
> > (Currently there are no other subclasses.)
>

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.h:54: //
|install_dir| is the path to the install directory for this version.
On 2013/10/25 17:33:22, xhwang wrote:
> comment order matches param order.

Done.

https://codereview.chromium.org/42003002/diff/50001/chrome/common/chrome_cont...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/common/chrome_cont...
chrome/common/chrome_content_client.cc:296: base::ASCIIToUTF16(",") +
On 2013/10/25 17:33:22, xhwang wrote:
> do we have comments anywhere about the format? e.g. delimited with ","

Done. Added constant and simplified this.

https://codereview.chromium.org/42003002/diff/50001/chrome/common/chrome_cont...
chrome/common/chrome_content_client.cc:303: #if
defined(WIDEVINE_CDM_AVC1_SUPPORT_AVAILABLE)
On 2013/10/25 17:33:22, xhwang wrote:
> Per offline discussion, add a TODO about renaming the macros?

Done.

https://codereview.chromium.org/42003002/diff/190001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/42003002/diff/190001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:94:
scoped_ptr<base::DictionaryValue> ReadManifest(
I put this function back where it was, so you may want to diff this file against
base.

[xhwang, 2013-10-25 19:19:48]

lgtm

[waffles, 2013-10-25 19:43:44]

lgtm

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
File chrome/browser/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/42003002/diff/50001/chrome/browser/component_...
chrome/browser/component_updater/default_component_installer.cc:195: // to UI in
Install()?
On 2013/10/25 19:03:40, ddorwin wrote:
> On 2013/10/25 16:53:42, waffles wrote:
> > I'm OK with passing the manifest; it's fine with me to post to UI, too.
> Updated TODO.
> > 
> > It's true that Widevine is the only user of this code, but PNaCl will be
soon
> > (next week?), now that it is unblocked on on-demand.
> Do you have an indication of what they intend to do in ComponentReady? Maybe
> I'll wait and see before addressing the TODO.
They'll call PathService::OverridePath and then some stuff on UI; so I think
it's safe to switch this to UI before then.

> > 
> > On 2013/10/25 00:06:05, ddorwin wrote:
> > > On 2013/10/24 23:07:06, waffles wrote:
> > > > The thinking was that people would likely want to manipulate those
files,
> > e.g.
> > > > read the manifest.
> > > 
> > > Assuming we're going to pass in the manifest like we do now, does it make
> > sense
> > > to call ComponentReady() on the UI thread, which it is already on in most
> > cases,
> > > and let individual Traits post if necessary?
> > > > 
> > > > What's the motivation for reading the manifest in
> DefaultComponentInstaller
> > > > instead of in Widevine's ComponentReady?
> > > 
> > > Responsibility separation and consistency. The Traits interface and the WV
> > > subclass don't know about the manifest location, name, format, etc., and
it
> > > doesn't make sense to duplicate that knowledge in each subclass. In the
> > install
> > > case, the DictionaryValue is passed to the Traits (because the manifest
has
> > > already been parsed). We could parse it in the WV class, but that would
mean
> > > unnecessarily re-parsing it in the install case. That is clearly much less
> > > frequent of a case, but it still feels like the wrong design.
> > > 
> > > The downside is that we may unnecessarily read and parse the manifest for
> > other
> > > components. I originally made the parsing optional based on a Traits
> property,
> > > but then I found we were already reading the fingerprint from disk for
every
> > > component. I decided to KISS and worry about optimization later if
> necessary.
> > > (Currently there are no other subclasses.)
> > 
>

[Lei Zhang, 2013-10-25 20:47:13]

chrome/common lgtm, and the number of macros is indeed too damn high.

[cpu_(ooo_6.6-7.5), 2013-10-25 23:06:22]

lgtm

https://codereview.chromium.org/42003002/diff/190001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/42003002/diff/190001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:104: DLOG(ERROR) <<
"Failed to deserialize " << manifest.MaybeAsASCII() << ": "
please remove the DLOG

https://codereview.chromium.org/42003002/diff/190001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:111: return
scoped_ptr<base::DictionaryValue>(
With more pointer safety comes great verbosity. Nonplussed but I'll let it pass.

[ddorwin, 2013-10-25 23:27:26]

https://codereview.chromium.org/42003002/diff/190001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/42003002/diff/190001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:104: DLOG(ERROR) <<
"Failed to deserialize " << manifest.MaybeAsASCII() << ": "
On 2013/10/25 23:06:23, cpu wrote:
> please remove the DLOG

I removed it in the interest of time, but I'd like to put it back in a future
CL.
I find this particular DLOG to be very helpful since |error| contains the cause
of the parsing failure (e.g. trailing comma). I found this useful while manually
testing various scenarios.
WDYT?

[commit-bot: I haz the power, 2013-10-25 23:32:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/42003002/360001

[commit-bot: I haz the power, 2013-10-26 04:15:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/42003002/360001

[commit-bot: I haz the power, 2013-10-26 09:49:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/42003002/360001

[commit-bot: I haz the power, 2013-10-26 10:52:13]

Change committed as 231197