Create an external source for extensions on linux that meets the needs of package managers.

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/file_path.h"
+#include "base/file_util.h"
+#include "base/json/json_file_value_serializer.h"
+#include "base/json/json_string_value_serializer.h"
+#include "base/memory/scoped_handle.h"
+#include "base/path_service.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_extension_util.h"
+#include "chrome/browser/extensions/external_filesystem_extension_loader_linux.h"
+#include "content/public/browser/browser_thread.h"
+
+

[Finnur, 2012/04/24 13:03:24]

nit: remove extra linebreak.

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+using content::BrowserThread;
+
+namespace {
+
+bool HasValidJsonExtension(const FilePath & filepath) {
+  return filepath.Extension() == FilePath::StringType(".json");
+}

[Finnur, 2012/04/24 13:03:24]

I believe this is not needed, see FilePath::MatchesExtension(...)

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+
+DictionaryValue* ReadJsonPrefsFile(const FilePath & json_file) {

[Finnur, 2012/04/24 13:03:24]

nit: No space before &

[Finnur, 2012/04/24 13:03:24]

Actually, is there a reason to create a helper around this? It is used once
below and doesn't really do much...

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+  JSONFileValueSerializer serializer(json_file);
+  DictionaryValue*
+      json_prefs = ExternalExtensionUtil::ExtractPrefs(json_file, &serializer);
+
+  return json_prefs;
+}
+
+std::set<FilePath> GetPrefsCandidatesFilesFromFolder(

[Finnur, 2012/04/24 13:03:24]

nit: s/Candidates/Candidate/
Also, my personal preference would be to pass in a std::set<FilePath>* as an
argument, rather than a return value, but that's just me.

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.
I'd rather have it as a return value, I am not too fond of pointer being passed
around
and passing it as an argument looks a bit "pedantic too me" since it requires a
separate
variable definition prior to calling the function. But this is debatable.

+      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";

[Finnur, 2012/04/24 13:03:24]

Do we need to log this? It seems like a normal condition... If it is useful then
we should log what the search path was.

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+    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());
+
+    DVLOG(1) << "Checking for file: " << reader.name();

[Finnur, 2012/04/24 13:03:24]

This seems like a normal condition that we don't need to log.

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+
+    if (HasValidJsonExtension(filename)) {
+      external_extension_paths.insert(filename);
+    } else {
+      DVLOG(1) << "Not considering: " << reader.name()
+               << " (does not have a .json extension)";
+    }
+  }
+
+  return external_extension_paths;
+}
+
+}  // namespace
+
+ExternalFilesystemExtensionLoader::ExternalFilesystemExtensionLoader(
+    int base_path_key)
+  : base_path_key_(base_path_key) {

[Finnur, 2012/04/24 13:03:24]

nit: Indentation should be 4 spaces, not 2.

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+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);
+
+  FilePath extension_search_path;
+  if (!PathService::Get(base_path_key_,
+                        &extension_search_path)) {
+    DVLOG(1) << "Could not retrieve per-extension search path";
+    return;
+  }
+
+  // First list the potential .json candidates
+  std::set<FilePath>
+      candidates = GetPrefsCandidatesFilesFromFolder(extension_search_path);
+  if (candidates.empty()) {
+    DVLOG(1) << "Extension candidates list empty";
+    return;
+  }
+
+  // For each file read the json description & build the proper
+  //  associated prefs

[Finnur, 2012/04/24 13:03:24]

nit: extra space at front. And you should in general end all comments with
period. (We've not been totally consistent on this, for some exceptions see
these short one-liners below):

// static
// namespace
// defined(OS_WIN)

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+  for (std::set<FilePath>::const_iterator it = candidates.begin();
+       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);

[Finnur, 2012/04/24 13:03:24]

We prefer

foo bar =
    function();

over 

foo
    bar = function().

(3 cases above)

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

+
+    DVLOG(1) << "Reading json file: " << extension_candidate_path.value();
+
+    if (NULL != json_file_content) {
+      DVLOG(1) << "Adding extension with id: " << id;

[Finnur, 2012/04/24 13:03:24]

This seems like a bit too much logging to me...

[Alexandre Abreu, 2012/04/24 19:30:30]

I removed some, for this particular one it is kind of useful.

+
+      prefs->Set(id, json_file_content);
+    }

[Finnur, 2012/04/24 13:03:24]

If you remove the log you can remove the braces (not needed for one line if
clauses).

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.

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

[Finnur, 2012/04/24 13:03:24]

nit: Extra linebreak at end.

[Alexandre Abreu, 2012/04/24 19:30:30]

Done.