Add a component updater for the CLD2 data file.

chrome/browser/component_updater/cld_component_installer.cc

Index: chrome/browser/component_updater/cld_component_installer.cc
diff --git a/chrome/browser/component_updater/cld_component_installer.cc b/chrome/browser/component_updater/cld_component_installer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5fa87acfc641dacbfc682ba6a0ba93b1760ccc0c
--- /dev/null
+++ b/chrome/browser/component_updater/cld_component_installer.cc
@@ -0,0 +1,149 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/component_updater/cld_component_installer.h"
+
+#include "base/bind.h"
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/lazy_instance.h"
+#include "base/path_service.h"
+#include "base/platform_file.h"
+#include "chrome/browser/component_updater/default_component_installer.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/ssl/ssl_config_service.h"
+
+using component_updater::ComponentUpdateService;
+using content::BrowserThread;
+
+namespace {
+

[Sorin Jianu, 2014/04/18 17:46:14]

These lines of code don't have to be indented inside the anon namespace. At
least, I've never indented them.

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

Done.

+  // Once we have acquired a valid file from the component installer, we need to
+  // make the path available to other parts of the system such as the
+  // translation libraries. We create a global to hold onto the path, and a
+  // lock to guard it.

[Sorin Jianu, 2014/04/18 17:46:14]

Two ideas here. If the file path is guaranteed to be retrieved from the UI
thread only, perhaps we could avoid the complexity of locking. The
ComponentReady call runs on the UI thread.

Second idea is a suggestion to consider whether using g_browser_process to store
the CLD file path instead of the global member makes sense. We are not fond of
g_browser_process nor the globals. We are ok either way we go, after considering
all options.

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

Access to the path takes place in the blocking pool, not the UI thread. This is
because we have to do some file access immediately, which is forbidden on the UI
thread in most cases. So I think we still need the lock.

I wasn't aware of g_browser_process, but after looking at it I don't feel like
it adds much value to move things there. That said there is precedent within for
the CRL updater and PNACL updater. I think I will defer to you guys on this: As
the owners of the component updater, would you prefer that I keep my global
handle in g_browser_process? If so, I'll move it; if not, I'm inclined to leave
it where it is.

+  base::LazyInstance<base::Lock> s_file_lock_ = LAZY_INSTANCE_INITIALIZER;
+
+  struct CLDFilePathWrapper {
+    CLDFilePathWrapper() {
+      value = NULL;
+    }
+    base::FilePath* value;

[Sorin Jianu, 2014/04/18 17:46:14]

Can we use a value type here instead of a pointer type? Doing so can simplify
the setter and the getter slightly, as we don't have to allocate and deallocate
the member.

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

I didn't realize filepath was just a stringtype. Ha! OK, yeah, I guess we can
get away with a value type. I'll change this.

+  };
+  base::LazyInstance<CLDFilePathWrapper>::Leaky s_file_;
+
+}
+
+namespace component_updater {
+
+// kPublicKeySHA256 is the SHA256 hash of the SubjectPublicKeyInfo of the key
+// that's used to sign generated CRL sets.

[waffles, 2014/04/18 17:48:02]

Hash + comment are wrong, as discussed.

I know you know this, but I want to leave a comment on this patchset for the
record/diffs.

+// The extension id is: dpedmmgabcgnikllifiidmijgoiihfgf
+const uint8 kPublicKeySHA256[32] = {
+  0xe8, 0xce, 0xcf, 0x42, 0x06, 0xd0, 0x93, 0x49,
+  0x6d, 0xd9, 0x89, 0xe1, 0x41, 0x04, 0x86, 0x4a,
+  0x8f, 0xbd, 0x86, 0x12, 0xb9, 0x58, 0x9b, 0xfb,
+  0x4f, 0xbb, 0x1b, 0xa9, 0xd3, 0x85, 0x37, 0xef
+};
+
+const char kCldManifestName[] = "CLD2 Data";
+
+class CldComponentInstallerTraits : public ComponentInstallerTraits {
+ public:
+  CldComponentInstallerTraits();
+  virtual ~CldComponentInstallerTraits() {}
+
+ private:
+  // The following methods override ComponentInstallerTraits.
+  virtual bool CanAutoUpdate() const OVERRIDE;
+  virtual bool OnCustomInstall(const base::DictionaryValue& manifest,
+                               const base::FilePath& install_dir) OVERRIDE;
+  virtual bool VerifyInstallation(
+      const base::FilePath& install_dir) const OVERRIDE;
+  virtual void ComponentReady(
+      const base::Version& version,
+      const base::FilePath& path,
+      scoped_ptr<base::DictionaryValue> manifest) OVERRIDE;
+  virtual base::FilePath GetBaseDirectory() const OVERRIDE;
+  virtual void GetHash(std::vector<uint8>* hash) const OVERRIDE;
+  virtual std::string GetName() const OVERRIDE;
+
+  void SetLatestCldDataFile(const base::FilePath& path);
+  DISALLOW_COPY_AND_ASSIGN(CldComponentInstallerTraits);
+};
+
+CldComponentInstallerTraits::CldComponentInstallerTraits() {
+}
+
+bool CldComponentInstallerTraits::CanAutoUpdate() const {
+  return true;
+}
+
+bool CldComponentInstallerTraits::OnCustomInstall(
+    const base::DictionaryValue& manifest,
+    const base::FilePath& install_dir) {
+  return true; // Nothing custom here.
+}
+
+void CldComponentInstallerTraits::ComponentReady(
+    const base::Version& version,
+    const base::FilePath& path,
+    scoped_ptr<base::DictionaryValue> manifest) {
+  base::FilePath source_path = path.Append(chrome::kCLDDataFilename);
+  SetLatestCldDataFile(source_path);
+}
+
+bool CldComponentInstallerTraits::VerifyInstallation(
+    const base::FilePath& install_dir) const {
+  // We can't really do much to verify the CLD2 data file. In theory we could
+  // read the headers, but that won't do much other than tell us whether or
+  // not the headers are valid. So just check if the file exists.
+  return base::PathExists(install_dir.Append(chrome::kCLDDataFilename));
+}
+
+base::FilePath CldComponentInstallerTraits::GetBaseDirectory() const {
+  base::FilePath result;
+  PathService::Get(chrome::DIR_COMPONENT_CLD2, &result);
+  return result;
+}
+
+void CldComponentInstallerTraits::GetHash(
+    std::vector<uint8>* hash) const {

[Sorin Jianu, 2014/04/18 17:46:14]

does this fit on one line?

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

........... maybe. :)

+  hash->assign(kPublicKeySHA256,
+               kPublicKeySHA256 + arraysize(kPublicKeySHA256));
+}
+
+std::string CldComponentInstallerTraits::GetName() const {
+  return kCldManifestName;
+}
+
+void RegisterCldComponent(ComponentUpdateService* cus) {
+  scoped_ptr<ComponentInstallerTraits> traits(
+      new CldComponentInstallerTraits);

[waffles, 2014/04/18 17:48:02]

We prefer "new CldComponentInstallerTraits())"

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

Sorry, copy/paste cruft. Done.

+  // |cus| will take ownership of |installer| during installer->Register(cus).
+  DefaultComponentInstaller* installer
+      = new DefaultComponentInstaller(traits.Pass());
+  installer->Register(cus);
+}
+
+void CldComponentInstallerTraits::SetLatestCldDataFile(
+    const base::FilePath& path) {
+  base::AutoLock lock(s_file_lock_.Get());
+  base::FilePath* old = s_file_.Get().value;

[Sorin Jianu, 2014/04/18 17:46:14]

Is there a reason why we don't just:
delete  s_file_.Get().value;
s_file_.Get().value = new base::FilePath(path);

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

Sorry, this was leftover from when I was trying to do this work without a lock.
We're not going to use a pointer anyhow, so no issue.

+  s_file_.Get().value = new base::FilePath(path);
+  if (old != NULL) delete old;

[Sorin Jianu, 2014/04/18 17:46:14]

We can delete a null pointer, no need for the check.

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

My C++ is rusty, I thought this would be a segfault. We won't be using a pointer
anymore anyhow, so hooray.

[Andrew Hayden (chromium.org), 2014/04/22 16:10:14]

Will go away when I stop using a pointer. Sorry, didn't know there as an
implicit null check in delete... my c++ is rusty.

+}
+
+bool GetLatestCldDataFile(base::FilePath* path) {
+  base::AutoLock lock(s_file_lock_.Get());
+  base::FilePath* cached = s_file_.Get().value;
+  if (cached == NULL || !base::PathExists(*cached)) return false;
+  *path = *cached;
+  return true;
+}
+
+}  // namespace component_updater