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

chrome/browser/extensions/external_pref_extension_loader.cc

Index: chrome/browser/extensions/external_pref_extension_loader.cc
===================================================================
--- chrome/browser/extensions/external_pref_extension_loader.cc	(revision 133542)
+++ chrome/browser/extensions/external_pref_extension_loader.cc	(working copy)
@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/external_pref_extension_loader.h"
 
 #include "base/bind.h"
+#include "base/dir_reader_posix.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/json/json_file_value_serializer.h"
@@ -19,31 +20,71 @@
 
 namespace {
 
+std::set<FilePath> GetPrefsCandidateFilesFromFolder(
+      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.
+    return external_extension_paths;
+  }
+
+  base::DirReaderPosix
+      reader(external_extension_search_path.value().c_str());
+
+  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());
+
+    if (filename.MatchesExtension(".json")) {
+      external_extension_paths.insert(filename);
+    } else {
+      DVLOG(1) << "Not considering: " << reader.name()
+               << " (does not have a .json extension)";
+    }
+  }
+
+  return external_extension_paths;
+}
+
+// Extracts/expect a file content in json format.

[Finnur, 2012/04/27 10:50:28]

Suggest:
Extracts extension information from a json file serialized by |serializer|.
|path| is only used for informational purposes (outputted when an error occurs).

Also: To make it clearer that |serializer| is more important I would actually
switch the param order (serializer, path) not (path, serializer).

[Alexandre Abreu, 2012/04/27 14:05:40]

Done.

+// An empty dictionary is returned in case of failure (e.g. invalid
+// path or json content).
 // Caller takes ownership of the returned dictionary.
-DictionaryValue* ExtractPrefs(const FilePath& path,
-                              base::ValueSerializer* serializer) {
+DictionaryValue* ExtractExtensionPrefs(
+    const FilePath& path,
+    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);
-    }
+                 << " in file " << path.value() << ".";
+    return new DictionaryValue;
   }
+
+  DictionaryValue* ext_dictionary = NULL;
+  if (extensions->GetAsDictionary(&ext_dictionary))
+    return ext_dictionary;
+
+  LOG(WARNING) << "Expected a JSON dictionary in file "
+               << path.value() << ".";
   return new DictionaryValue;
 }
 
-}  // namespace
+}  // namespace {
 
-ExternalPrefExtensionLoader::ExternalPrefExtensionLoader(int base_path_key,
+ExternalPrefExtensionLoader::ExternalPrefExtensionLoader(int base_path_id,
                                                          Options options)
-    : base_path_key_(base_path_key),
-      options_(options){
+    : base_path_id_(base_path_id),
+      options_(options) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
@@ -61,23 +102,55 @@
       base::Bind(&ExternalPrefExtensionLoader::LoadOnFileThread, this));
 }
 
-DictionaryValue* ExternalPrefExtensionLoader::ReadJsonPrefsFile() {
-  // TODO(skerner): Some values of base_path_key_ will cause
+void ExternalPrefExtensionLoader::LoadOnFileThread() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  // TODO(skerner): Some values of base_path_id_ will cause
   // PathService::Get() to return false, because the path does
   // not exist.  Find and fix the build/install scripts so that
   // this can become a CHECK().  Known examples include chrome
   // OS developer builds and linux install packages.
   // Tracked as crbug.com/70402 .
-  if (!PathService::Get(base_path_key_, &base_path_)) {
-    return NULL;
+  if (!PathService::Get(base_path_id_, &base_path_)) {
+    return;
   }

[Finnur, 2012/04/27 10:50:28]

nit: Avoid braces for single line if clauses.

[Alexandre Abreu, 2012/04/27 14:05:40]

Done.

 
+  scoped_ptr<DictionaryValue> prefs(new DictionaryValue);
+
+  ReadExternalExtensionPrefFile(prefs.get());

[Finnur, 2012/04/27 10:50:28]

Can you add a warning, something like:
if (!prefs.empty())
  LOG(WARNING) << "You are using an old-style extension deployment method
(external_extensions.json), which will soon be deprecated.";

Or something to that effect...
Perhaps add the link to:
http://code.google.com/chrome/extensions/external_extensions.html

[Alexandre Abreu, 2012/04/27 14:05:40]

Done.

+  ReadStandaloneExtensionPrefFiles(prefs.get());

[Finnur, 2012/04/27 10:50:28]

Just to be clear: The format of the |prefs| dictionary does not change with this
changelist, correct? All that happens is that a few more objects get added to
the dictionary if <id>.json files appear, right?

[Alexandre Abreu, 2012/04/27 14:05:40]

Yes,

+
+  prefs_.reset(prefs.release());
+  if (!prefs_.get())
+    prefs_.reset(new DictionaryValue());
+
+  if (base_path_id_ == chrome::DIR_EXTERNAL_EXTENSIONS) {
+    UMA_HISTOGRAM_COUNTS_100("Extensions.ExternalJsonCount",
+                             prefs_->size());
+  }
+
+  // If we have any records to process, then we must have
+  // read at least one .json file.  If so, then we should have
+  // set |base_path_|.
+  if (!prefs_->empty())
+    CHECK(!base_path_.empty());
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&ExternalPrefExtensionLoader::LoadFinished, this));
+}
+
+void ExternalPrefExtensionLoader::ReadExternalExtensionPrefFile(
+      DictionaryValue* prefs) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  CHECK(NULL != prefs);
+
   FilePath json_file = base_path_.Append(
       FILE_PATH_LITERAL("external_extensions.json"));
 
   if (!file_util::PathExists(json_file)) {
     // This is not an error.  The file does not exist by default.
-    return NULL;
+    return;
   }
 
   if (IsOptionSet(ENSURE_PATH_CONTROLLED_BY_ADMIN)) {
@@ -100,31 +173,47 @@
   }
 
   JSONFileValueSerializer serializer(json_file);
-  DictionaryValue* parsed_json_prefs = ExtractPrefs(json_file, &serializer);
-  return parsed_json_prefs;
+  DictionaryValue * ext_prefs = ExtractExtensionPrefs(json_file, &serializer);
+  if (NULL != ext_prefs)

[Finnur, 2012/04/27 10:50:28]

nit: if (ext_prefs)

[Alexandre Abreu, 2012/04/27 14:05:40]

Done.

+    prefs->MergeDictionary(ext_prefs);
 }
 
-void ExternalPrefExtensionLoader::LoadOnFileThread() {
+void ExternalPrefExtensionLoader::ReadStandaloneExtensionPrefFiles(
+      DictionaryValue* prefs) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  CHECK(NULL != prefs);
 
-  prefs_.reset(ReadJsonPrefsFile());
-  if (!prefs_.get())
-    prefs_.reset(new DictionaryValue());
-
-  if (base_path_key_ == chrome::DIR_EXTERNAL_EXTENSIONS) {
-    UMA_HISTOGRAM_COUNTS_100("Extensions.ExternalJsonCount",
-                             prefs_->size());
+  // First list the potential .json candidates.
+  std::set<FilePath>
+      candidates = GetPrefsCandidateFilesFromFolder(base_path_);
+  if (candidates.empty()) {
+    DVLOG(1) << "Extension candidates list empty";
+    return;
   }
 
-  // If we have any records to process, then we must have
-  // read the .json file.  If we read the .json file, then
-  // we were should have set |base_path_|.
-  if (!prefs_->empty())
-    CHECK(!base_path_.empty());
+  // For each file read the json description & build the proper
+  // associated prefs.
+  for (std::set<FilePath>::const_iterator it = candidates.begin();
+       it != candidates.end();
+       ++it) {
+    FilePath extension_candidate_path = base_path_.Append(*it);
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&ExternalPrefExtensionLoader::LoadFinished, this));
+    FilePath::StringType id =
+        extension_candidate_path.RemoveExtension().BaseName().value();
+
+    DVLOG(1) << "Reading json file: " << extension_candidate_path.value();
+
+    JSONFileValueSerializer serializer(extension_candidate_path);
+    DictionaryValue* ext_prefs =
+        ExtractExtensionPrefs(extension_candidate_path,
+                              &serializer);
+
+    if (NULL != ext_prefs) {

[Finnur, 2012/04/27 10:50:28]

nit: if (ext_prefs)

[Alexandre Abreu, 2012/04/27 14:05:40]

Done.

+      DVLOG(1) << "Adding extension with id: " << id;
+

[Finnur, 2012/04/27 10:50:28]

nit: delete this extra linebreak?

[Alexandre Abreu, 2012/04/27 14:05:40]

Done.

+      prefs->Set(id, ext_prefs);
+    }
+  }
 }
 
 ExternalTestingExtensionLoader::ExternalTestingExtensionLoader(
@@ -134,7 +223,7 @@
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   JSONStringValueSerializer serializer(json_data);
   FilePath fake_json_path = fake_base_path.AppendASCII("fake.json");
-  testing_prefs_.reset(ExtractPrefs(fake_json_path, &serializer));
+  testing_prefs_.reset(ExtractExtensionPrefs(fake_json_path, &serializer));
 }
 
 void ExternalTestingExtensionLoader::StartLoading() {