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

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

This change was written by Alexandre Abreu, and reviewed by Sam Kerner (skerner@chromium.org).   Submitting on Alexandre's behalf because he is not yet a committer.

BUG=75174
TEST=Existing external extensions prefs tests.

[commit-bot: I haz the power, 2012-04-23 16:50:30]

No reviewers yet.

[Sam Kerner (Chrome), 2012-04-23 17:31:38]

Finnur, I reviewed (and LGTMed) the following patch for an external contributor.
 I want to submit it on his behalf, but submit scripts are telling me that I
need approval from another committer.  Please take a look.

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

We're close, but I think it needs a little bit of polish and some style-fixes
before being submitted. 

Also, Sam: Your email is misspelled in the CL description and I believe
Alexandre doesn't need to be a committer as long as Alexandre submits the CL to
Rietveld and gets a green light from you. Then I think we should be able to use
the commit queue. But, we can do it this way too.

http://codereview.chromium.org/10179002/diff/2002/AUTHORS
File AUTHORS (right):

http://codereview.chromium.org/10179002/diff/2002/AUTHORS#newcode177
AUTHORS:177: Alexandre Abreu <wiss1976@gmail.com>
Has Alexandre signed the contributor license agreement?
http://dev.chromium.org/developers/contributing-code ?

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_util.cc (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:17: 
nit: remove extra line.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:20: //  to parse their
extension manifests.
nit: Extra space at front.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:21: // Caller takes
ownership of the returned dictionary.
This should be moved to the .h file. It is also a bit terse.
I like the fact that ownership is documented but I'm less enthusiastic about the
fact that |path| and |serializer| are not explained and that more emphasis is on
who uses the function than describing what it does. The function also has a very
generic name. Hmm.... maybe with the ExternalExtensionUtil prefix it is OK...

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:22: DictionaryValue*
ExternalExtensionUtil::ExtractPrefs(
Hmm... this function is actually, in large part, doing what
DictionaryValue::GetAsDictionary is meant to do. Is there a reason why you don't
just use that function instead of largely reimplementing it here?

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:29: << " In file " <<
path.value() << " .";
nit: Extra space before period (also on line 33).

Furthermore, for readability, please put a...
  return new DictionaryValue;
... at line 30 and get rid of the else clause nesting.

Same with line 32.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_util.h (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_UTIL_H__
This is old school. New files use single trailing underscore (not two).

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.h:16: class
ExternalExtensionUtil {
Files and classes that end in Util tend to accumulate junk over time. In this
case, however, I'm not even sure a separate file for this util function is
warranted since this function isn't shared with any other code. You could just
put it in a namespace {} at the .cc file where you use it and if we need to
share it elsewhere we can always move it.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.h:19: base::ValueSerializer*
serializer);
Can we use the WARN_ON_UNUSED_RESULT macro for this function?

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.h:20: };
It feels like this should be a namespace and not a class... No?

Also, the function is missing comments (see my comment in .cc file).

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc
(right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:22: 
nit: remove extra linebreak.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:29: }
I believe this is not needed, see FilePath::MatchesExtension(...)

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:31:
DictionaryValue* ReadJsonPrefsFile(const FilePath & json_file) {
nit: No space before &

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:31:
DictionaryValue* ReadJsonPrefsFile(const FilePath & json_file) {
Actually, is there a reason to create a helper around this? It is used once
below and doesn't really do much...

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:39:
std::set<FilePath> GetPrefsCandidatesFilesFromFolder(
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.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:47:
DVLOG(1) << "Extension search path not found";
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.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:64:
DVLOG(1) << "Checking for file: " << reader.name();
This seems like a normal condition that we don't need to log.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:81: :
base_path_key_(base_path_key) {
nit: Indentation should be 4 spaces, not 2.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:112: // 
associated prefs
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)

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:123:
json_file_content = ReadJsonPrefsFile(extension_candidate_path);
We prefer

foo bar =
    function();

over 

foo
    bar = function().

(3 cases above)

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:128:
DVLOG(1) << "Adding extension with id: " << id;
This seems like a bit too much logging to me...

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:131: }
If you remove the log you can remove the braces (not needed for one line if
clauses).

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:140: 
nit: Extra linebreak at end.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_filesystem_extension_loader_linux.h
(right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.h:14:
explicit ExternalFilesystemExtensionLoader(int base_path_key);
nit: document.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.h:22:
virtual ~ExternalFilesystemExtensionLoader() {}
Please move the implementation of this dtor to the .cc file.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.h:26: int
base_path_key_;
Document both of these.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:75: #if defined(OS_LINUX)
... and not OS_CHROMEOS ?

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:84: #endif
missing:  // defined(GOOGLE_CHROME_BUILD)

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:85: #endif  // defined(OS_LINUX)
... and not OS_CHROMEOS ?

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:377: #if defined(OS_LINUX)
Ditto.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths.h
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:57: #if defined(OS_LINUX)
... and not OS_CHROMEOS ?

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:60: // installed when chrome is run.
I don't understand what this means (in the absence of the context provided by
the rest of this CL). What *is* 'single external extensions'? It also doesn't
sound grammatically correct (single extension*s* -- unless we're talking about
marital status, which we are not) :) It would seem to me that we might have
multiple extensions specified in the json file, no? I guess I'm trying to point
out that the word single is misleading in this context.

Also, it seems like the word 'which' is missing from the comments... (or
something).

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:61: #endif
Missing:  // defined(OS_LINUX) && !defined(OS_CHROMEOS)

[Sam Kerner (Chrome), 2012-04-24 15:27:54]

Finnur raises some good points.  Alexandre, can you take this patch, address the
review comments, and send it for review to Finnur?

Sam

http://codereview.chromium.org/10179002/diff/2002/AUTHORS
File AUTHORS (right):

http://codereview.chromium.org/10179002/diff/2002/AUTHORS#newcode177
AUTHORS:177: Alexandre Abreu <wiss1976@gmail.com>
On 2012/04/24 13:03:24, Finnur wrote:
> Has Alexandre signed the contributor license agreement?
> http://dev.chromium.org/developers/contributing-code ?

Yes

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_util.h (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.h:16: class
ExternalExtensionUtil {
On 2012/04/24 13:03:24, Finnur wrote:
> Files and classes that end in Util tend to accumulate junk over time. In this
> case, however, I'm not even sure a separate file for this util function is
> warranted since this function isn't shared with any other code. You could just
> put it in a namespace {} at the .cc file where you use it and if we need to
> share it elsewhere we can always move it.

This is used in external_filesystem_extension_loader_linux.cc and
external_pref_extension_loader.cc .

[Alexandre Abreu, 2012-04-24 19:30:29]

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_util.cc (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:20: //  to parse their
extension manifests.
On 2012/04/24 13:03:24, Finnur wrote:
> nit: Extra space at front.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:21: // Caller takes
ownership of the returned dictionary.
On 2012/04/24 13:03:24, Finnur wrote:
> This should be moved to the .h file. It is also a bit terse.
> I like the fact that ownership is documented but I'm less enthusiastic about
the
> fact that |path| and |serializer| are not explained and that more emphasis is
on
> who uses the function than describing what it does. The function also has a
very
> generic name. Hmm.... maybe with the ExternalExtensionUtil prefix it is OK...

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:22: DictionaryValue*
ExternalExtensionUtil::ExtractPrefs(
On 2012/04/24 13:03:24, Finnur wrote:
> Hmm... this function is actually, in large part, doing what
> DictionaryValue::GetAsDictionary is meant to do. Is there a reason why you
don't
> just use that function instead of largely reimplementing it here?

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.cc:29: << " In file " <<
path.value() << " .";
On 2012/04/24 13:03:24, Finnur wrote:
> nit: Extra space before period (also on line 33).
> 
> Furthermore, for readability, please put a...
>   return new DictionaryValue;
> ... at line 30 and get rid of the else clause nesting.
> 
> Same with line 32.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_util.h (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_util.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_UTIL_H__
On 2012/04/24 13:03:24, Finnur wrote:
> This is old school. New files use single trailing underscore (not two).

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc
(right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:22: 
On 2012/04/24 13:03:24, Finnur wrote:
> nit: remove extra linebreak.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:29: }
On 2012/04/24 13:03:24, Finnur wrote:
> I believe this is not needed, see FilePath::MatchesExtension(...)

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:31:
DictionaryValue* ReadJsonPrefsFile(const FilePath & json_file) {
On 2012/04/24 13:03:24, Finnur wrote:
> Actually, is there a reason to create a helper around this? It is used once
> below and doesn't really do much...

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:39:
std::set<FilePath> GetPrefsCandidatesFilesFromFolder(
On 2012/04/24 13:03:24, Finnur wrote:
> nit: s/Candidates/Candidate/

Done.

> 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.

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.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:47:
DVLOG(1) << "Extension search path not found";
On 2012/04/24 13:03:24, Finnur wrote:
> 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.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:64:
DVLOG(1) << "Checking for file: " << reader.name();
On 2012/04/24 13:03:24, Finnur wrote:
> This seems like a normal condition that we don't need to log.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:81: :
base_path_key_(base_path_key) {
On 2012/04/24 13:03:24, Finnur wrote:
> nit: Indentation should be 4 spaces, not 2.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:112: // 
associated prefs
On 2012/04/24 13:03:24, Finnur wrote:
> 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)

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:123:
json_file_content = ReadJsonPrefsFile(extension_candidate_path);
On 2012/04/24 13:03:24, Finnur wrote:
> We prefer
> 
> foo bar =
>     function();
> 
> over 
> 
> foo
>     bar = function().
> 
> (3 cases above)

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:128:
DVLOG(1) << "Adding extension with id: " << id;
On 2012/04/24 13:03:24, Finnur wrote:
> This seems like a bit too much logging to me...

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

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:131: }
On 2012/04/24 13:03:24, Finnur wrote:
> If you remove the log you can remove the braces (not needed for one line if
> clauses).

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:140: 
On 2012/04/24 13:03:24, Finnur wrote:
> nit: Extra linebreak at end.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
File chrome/browser/extensions/external_filesystem_extension_loader_linux.h
(right):

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.h:14:
explicit ExternalFilesystemExtensionLoader(int base_path_key);
On 2012/04/24 13:03:24, Finnur wrote:
> nit: document.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.h:22:
virtual ~ExternalFilesystemExtensionLoader() {}
On 2012/04/24 13:03:24, Finnur wrote:
> Please move the implementation of this dtor to the .cc file.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/browser/extensions/e...
chrome/browser/extensions/external_filesystem_extension_loader_linux.h:26: int
base_path_key_;
On 2012/04/24 13:03:24, Finnur wrote:
> Document both of these.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:75: #if defined(OS_LINUX)
On 2012/04/24 13:03:24, Finnur wrote:
> ... and not OS_CHROMEOS ?

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:84: #endif
On 2012/04/24 13:03:24, Finnur wrote:
> missing:  // defined(GOOGLE_CHROME_BUILD)

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:85: #endif  // defined(OS_LINUX)
On 2012/04/24 13:03:24, Finnur wrote:
> ... and not OS_CHROMEOS ?

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:377: #if defined(OS_LINUX)
On 2012/04/24 13:03:24, Finnur wrote:
> Ditto.

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths.h
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:57: #if defined(OS_LINUX)
On 2012/04/24 13:03:24, Finnur wrote:
> ... and not OS_CHROMEOS ?

Done.

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:60: // installed when chrome is run.
On 2012/04/24 13:03:24, Finnur wrote:
> I don't understand what this means (in the absence of the context provided by
> the rest of this CL). What *is* 'single external extensions'? It also doesn't
> sound grammatically correct (single extension*s* -- unless we're talking about
> marital status, which we are not) :) It would seem to me that we might have
> multiple extensions specified in the json file, no? I guess I'm trying to
point
> out that the word single is misleading in this context.
> 
> Also, it seems like the word 'which' is missing from the comments... (or
> something).

Replaced by something that "sounded" better & better explanation (hopefully).

http://codereview.chromium.org/10179002/diff/2002/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:61: #endif
On 2012/04/24 13:03:24, Finnur wrote:
> Missing:  // defined(OS_LINUX) && !defined(OS_CHROMEOS)

Since it is not something specified for other #defined "clauses", I am not sure
I would add it for this one just for the sake of consistency.

[Alexandre Abreu, 2012-04-24 19:33:53]

My latest uploaded CL ended up there: http://codereview.chromium.org/9963120

since there have been a rebase, I am not sure how to link my uploads for this CL
to this new codereview page ...