Introduces an additional extension loader that load extra extensions based on per-extension json fi…

chrome/browser/extensions/external_filesystem_extension_loader_linux.cc

+// Copyright (c) 2012 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 "base/bind.h"
+#include "base/dir_reader_posix.h"
+#include "base/json/json_file_value_serializer.h"
+#include "base/json/json_string_value_serializer.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/memory/scoped_handle.h"
+#include "base/string_util.h"
+#include "base/utf_string_conversions.h"
+#include "base/values.h"
+#include "base/version.h"
+#include "chrome/browser/extensions/external_extension_provider_impl.h"
+#include "chrome/browser/extensions/external_filesystem_extension_loader_linux.h"
+#include "content/public/browser/browser_thread.h"
+
+
+using content::BrowserThread;
+
+namespace {
+
+// The search path that contains information about external extensions.
+#if defined(GOOGLE_CHROME_BUILD)
+const char kFilepathPrefExtensions[] = "/usr/share/google-chrome/extensions";

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

Take a look at src/chrome/common/chrome_paths.h .

It defines a mechanism to get a path based on a constant in that file.  Instead
of hard-coding a path, add it in the above file and use PathService::Get() to
fetch the right path in this file.

Is there a standard that specifies what should be in /usr/share ?  A comment
pointing people to such a standard is a good way to avoid arguments about what
the path should be.

+#else
+const char kFilepathPrefExtensions[] = "/usr/share/chromium/extensions";
+#endif
+
+
+// Caller takes ownership of the returned dictionary.
+DictionaryValue* ExtractPrefs(const FilePath& path,

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

This code looks a lot like the code that reads external_extensions.json .  Is
there a reason you can't have your code and the old code call a common utility
function?

+                              base::ValueSerializer* serializer) {
+  std::string error_msg;
+  Value* extensions = serializer->Deserialize(NULL, &error_msg);
+  if (!extensions) {
+    LOG(WARNING) << "Unable to deserialize json data: " << error_msg
+                 << " In file " << path.value() << " .";
+  } else {
+    if (!extensions->IsType(Value::TYPE_DICTIONARY)) {
+      LOG(WARNING) << "Expected a JSON dictionary in file "
+                   << path.value() << " .";
+    } else {
+      return static_cast<DictionaryValue*>(extensions);
+    }
+  }
+  return new DictionaryValue;
+}
+
+bool HasValidJsonExtension(const FilePath & filepath) {
+  return filepath.Extension() == FilePath::StringType(".json");
+}
+
+bool IsExcludedFromJsonPrefsCandidates(const FilePath & filepath) {

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

Why is this needed?  I think you are looking in a different directory than the
one that holds external_extension.json, right?

+  return filepath.BaseName()
+                 .value() == FilePath::StringType("external_extension.json");
+}
+
+DictionaryValue* ReadJsonPrefsFile(const FilePath & json_file) {
+  JSONFileValueSerializer serializer(json_file);
+  DictionaryValue* parsed_json_prefs = ExtractPrefs(json_file, &serializer);
+
+  return parsed_json_prefs;
+}
+
+std::set<FilePath> GetPrefsCandidatesFilesFromFolder(
+      const FilePath & external_extension_search_path) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  std::set<FilePath> external_extension_paths;
+
+  if (!file_util::PathExists(external_extension_search_path)) {
+    // Does not have to exist.
+    DVLOG(1) << "Extension search path not found";
+    return external_extension_paths;
+  }
+
+  base::DirReaderPosix
+    reader(external_extension_search_path.value().c_str());

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

Chrome style is four spaces when continuing from the previous line.  

+
+  if (!reader.IsValid()) {
+    LOG(ERROR) << "Can not read external extensions path '"
+               << external_extension_search_path.LossyDisplayName()
+               << "' although it appears to exist.";
+    return external_extension_paths;
+  }
+
+  while (reader.Next()) {
+    const FilePath filename(reader.name());
+
+    DVLOG(1) << "Checking for file: " << reader.name();
+
+    if (   HasValidJsonExtension(filename)

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

Style: No space between "(" and "Has".

+        && !IsExcludedFromJsonPrefsCandidates(filename)) {
+      external_extension_paths.insert(filename);
+    } else {
+      DVLOG(1) << "Not considering: " << reader.name();
+    }
+  }
+
+  return external_extension_paths;
+}
+
+}  // namespace
+
+

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

Use one blank line between functions.

+
+
+void ExternalFilesystemExtensionLoader::StartLoading() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&ExternalFilesystemExtensionLoader::LoadOnFileThread, this));
+}
+
+
+void ExternalFilesystemExtensionLoader::LoadOnFileThread() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  scoped_ptr<DictionaryValue> prefs(new DictionaryValue);
+
+  const FilePath
+    EXTENSION_SEARCH_PATH = FilePath(kFilepathPrefExtensions);

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

4 spaces.

+
+  // First list the potential .json candidates (excluding some like
+  //  reserved names like "external_extension.json" ...
+  std::set<FilePath>
+    candidates = GetPrefsCandidatesFilesFromFolder(EXTENSION_SEARCH_PATH);

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

4 spaces.

+
+  if (candidates.empty()) {
+      DVLOG(1) << "Extension candidates list empty";

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

Indent is 2 spaces.

+      return;
+  }
+
+  // For each file read the json description & build the proper
+  //  associated prefs
+  for (std::set<FilePath>::const_iterator it = candidates.begin()

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

semicolons belong on the same line as the statement they end.

+         ; it != candidates.end()
+         ; ++it) {
+    FilePath
+      extension_candidate_path = EXTENSION_SEARCH_PATH.Append(*it);
+
+    FilePath::StringType
+      id = extension_candidate_path.RemoveExtension().BaseName().value();
+
+    DictionaryValue*
+      json_file_content = ReadJsonPrefsFile(extension_candidate_path);
+
+    DVLOG(1) << "Reading json file: " << extension_candidate_path.value();
+
+    if (NULL != json_file_content) {
+      DVLOG(1) << "Adding extension with id: " << id;
+
+      prefs->Set(id, json_file_content);
+    }
+  }
+

[Sam Kerner (Chrome), 2012/04/13 01:35:27]

What happens if:
* The id is not valid (Extension::IsIdValid() returns false)
* The version is missing or malformed.
* The path to the extension is relative (something like foo/bar.crx), what
directory do you resolve from?  
* The CRX file does not exist.

I think these cases might be taken care of by existing code that reads |prefs|,
but please verify this.

+  prefs_.reset(prefs.release());
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&ExternalFilesystemExtensionLoader::LoadFinished, this));
+}
+
+