Extract supported codecs from CDM component manifest.

chrome/browser/component_updater/default_component_installer.cc

Index: chrome/browser/component_updater/default_component_installer.cc
diff --git a/chrome/browser/component_updater/default_component_installer.cc b/chrome/browser/component_updater/default_component_installer.cc
index b610f2ee4267616b1299cdcbfa72e935b9e9bb41..f0d1b0fa369b95e7a7be290c2ce4d0360c8d49a4 100644
--- a/chrome/browser/component_updater/default_component_installer.cc
+++ b/chrome/browser/component_updater/default_component_installer.cc
@@ -17,6 +17,9 @@ namespace {
 const char kNullVersion[] = "0.0.0.0";
 }  // namespace
 
+ComponentInstallerTraits::~ComponentInstallerTraits() {
+}
+
 DefaultComponentInstaller::DefaultComponentInstaller(
     scoped_ptr<ComponentInstallerTraits> installer_traits)
     : current_version_(kNullVersion) {
@@ -76,7 +79,11 @@ bool DefaultComponentInstaller::Install(const base::DictionaryValue& manifest,
     return false;
   }
   current_version_ = version;
-  installer_traits_->ComponentReady(current_version_, GetInstallDirectory());
+  current_manifest_.reset(manifest.DeepCopy());
+  installer_traits_->ComponentReady(
+      current_version_,
+      GetInstallDirectory(),
+      scoped_ptr<base::DictionaryValue>(current_manifest_->DeepCopy()).Pass());
   return true;
 }
 
@@ -133,8 +140,17 @@ void DefaultComponentInstaller::StartRegistration(
 
   if (found) {
     current_version_ = latest_version;
+    // TODO(waffles): Why do we save these as members rather than passing them
+    // to FinishRegistration()?

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

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.

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

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

     base::ReadFileToString(latest_dir.AppendASCII("manifest.fingerprint"),
                            &current_fingerprint_);
+    current_manifest_.reset(ReadComponentManifest(latest_dir));
+    if (!current_manifest_) {
+      DLOG(ERROR) << "Failed to read manifest for "
+                  << installer_traits_->GetName() << " ("
+                  << base_dir.MaybeAsASCII() << ").";
+      return;
+    }
   }
 
   // Remove older versions of the component. None should be in use during
@@ -174,11 +190,17 @@ void DefaultComponentInstaller::FinishRegistration(
   }
 
   if (current_version_.CompareTo(base::Version(kNullVersion)) > 0) {
+    // TODO(waffles): Why is this function called on the FILE thread? The only
+    // implementation just posts back to the UI thread. Should we instead post
+    // to UI in Install()?

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

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]

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?
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:42]

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:

[ddorwin, 2013/10/25 19:03:40]

Updated TODO.
Do you have an indication of what they intend to do in ComponentReady? Maybe
I'll wait and see before addressing the TODO.

[waffles, 2013/10/25 19:43:45]

They'll call PathService::OverridePath and then some stuff on UI; so I think
it's safe to switch this to UI before then.

+    scoped_ptr<base::DictionaryValue> manifest_copy(
+        current_manifest_->DeepCopy());
     content::BrowserThread::PostTask(
         content::BrowserThread::FILE, FROM_HERE,
         base::Bind(&ComponentInstallerTraits::ComponentReady,
                    base::Unretained(installer_traits_.get()),
                    current_version_,
-                   GetInstallDirectory()));
+                   GetInstallDirectory(),
+                   base::Passed(&manifest_copy)));
   }
 }