Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9963120:
Introduces an additional extension loader that load extra extensions based on per-extension json fi… (Closed)
|
Created:
8 years, 8 months ago by Alexandre Abreu Modified:
6 years, 11 months ago CC:
chromium-reviews, Aaron Boodman, mihaip+watch_chromium.org Base URL:
https://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionModifies the external extension preferences loader to load extra extensions based on standalone per-extension json files
searched in /usr/share/[chromium|google-chrome]/extensions. The json file is formatted as such:
<extension-id>.json
and
{
"external_crx": "<path>",
"external_vrsion": "x.x"
}
Also modifies the document to account for that change and to deprecate the external_extension.json files uses.
BUG=75174
TEST=None
Patch Set 1 #Patch Set 2 : #
Total comments: 13
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 30
Patch Set 7 : #Patch Set 8 : #
Total comments: 4
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 43
Patch Set 14 : #Patch Set 15 : #
Total comments: 35
Patch Set 16 : #
Total comments: 16
Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Messages
Total messages: 29 (0 generated)
Alexandre, if you have not yet read our wiki page on contributing code, please take a look: http://www.chromium.org/developers/contributing-code That page has a link to the Individual Contributor License Agreement. If you have not submitted one yet, please do. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc (right): http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:27: const char kFilepathPrefExtensions[] = "/usr/share/google-chrome/extensions"; 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. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:34: DictionaryValue* ExtractPrefs(const FilePath& path, 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? http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:56: bool IsExcludedFromJsonPrefsCandidates(const FilePath & filepath) { Why is this needed? I think you are looking in a different directory than the one that holds external_extension.json, right? http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:81: reader(external_extension_search_path.value().c_str()); Chrome style is four spaces when continuing from the previous line. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:95: if ( HasValidJsonExtension(filename) Style: No space between "(" and "Has". http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:108: Use one blank line between functions. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:124: EXTENSION_SEARCH_PATH = FilePath(kFilepathPrefExtensions); 4 spaces. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:129: candidates = GetPrefsCandidatesFilesFromFolder(EXTENSION_SEARCH_PATH); 4 spaces. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:132: DVLOG(1) << "Extension candidates list empty"; Indent is 2 spaces. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:138: for (std::set<FilePath>::const_iterator it = candidates.begin() semicolons belong on the same line as the statement they end. http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:158: 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. http://codereview.chromium.org/9963120/diff/4001/chrome/chrome_browser_extens... File chrome/chrome_browser_extensions.gypi (right): http://codereview.chromium.org/9963120/diff/4001/chrome/chrome_browser_extens... chrome/chrome_browser_extensions.gypi:405: 'browser/extensions/external_filesystem_extension_loader_linux.cc', We try to keep the list of source files alphabetized. http://codereview.chromium.org/9963120/diff/4001/chrome/chrome_browser_extens... chrome/chrome_browser_extensions.gypi:542: ['exclude', '^browser/extensions/external_filesystem_extension_loader_linux.h'] I think you don't want this on Mac OS or Chrome OS. Instead of excluding it from any platform, I recommend including it on every platform and using #if defined (OS_LINUX) around the code that initiates the reading of the source.
On 2012/04/13 01:35:27, Sam Kerner (Chrome) wrote: > Alexandre, if you have not yet read our wiki page on contributing code, please > take a look: > > http://www.chromium.org/developers/contributing-code > > That page has a link to the Individual Contributor License Agreement. If you > have not submitted one yet, please do. I did some time ago, I haven't heard any news though ... I don't know how to get feeback from that > > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc > (right): > > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:27: > const char kFilepathPrefExtensions[] = "/usr/share/google-chrome/extensions"; > 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. Done. > 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. I talked to the person that apparently took over fta's work for Ubuntu's chromium build (Christian Dywan) and he is fine with the chosen path AFAIK and it follows FHS recommandations (http://www.pathname.com/fhs/). > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:34: > DictionaryValue* ExtractPrefs(const FilePath& path, > 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? No reason, besides the fact that I wanted the changes to be the less invasive as possible (for my first contrib). Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:56: bool > IsExcludedFromJsonPrefsCandidates(const FilePath & filepath) { > Why is this needed? I think you are looking in a different directory than the > one that holds external_extension.json, right? After some thoughts, not really needed. Removed. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:81: > reader(external_extension_search_path.value().c_str()); > Chrome style is four spaces when continuing from the previous line. Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:95: if ( > HasValidJsonExtension(filename) > Style: No space between "(" and "Has". Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:108: > Use one blank line between functions. Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:124: > EXTENSION_SEARCH_PATH = FilePath(kFilepathPrefExtensions); > 4 spaces. Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:129: > candidates = GetPrefsCandidatesFilesFromFolder(EXTENSION_SEARCH_PATH); > 4 spaces. Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:132: > DVLOG(1) << "Extension candidates list empty"; > Indent is 2 spaces. Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:138: for > (std::set<FilePath>::const_iterator it = candidates.begin() > semicolons belong on the same line as the statement they end. Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:158: > 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. Yes, it is taken care of by the logic that is more specific to the extension provider that's why I kept it like this w/o all the duplicate checks/validations. > http://codereview.chromium.org/9963120/diff/4001/chrome/chrome_browser_extens... > File chrome/chrome_browser_extensions.gypi (right): > > http://codereview.chromium.org/9963120/diff/4001/chrome/chrome_browser_extens... > chrome/chrome_browser_extensions.gypi:405: > 'browser/extensions/external_filesystem_extension_loader_linux.cc', > We try to keep the list of source files alphabetized. Done. > http://codereview.chromium.org/9963120/diff/4001/chrome/chrome_browser_extens... > chrome/chrome_browser_extensions.gypi:542: ['exclude', > '^browser/extensions/external_filesystem_extension_loader_linux.h'] > I think you don't want this on Mac OS or Chrome OS. Instead of excluding it > from any platform, I recommend including it on every platform and using > > #if defined (OS_LINUX) > > around the code that initiates the reading of the source. Done. I am not too sure about this one, since I added a path-enum in the chrome_paths that is not surrounded by any specific #define flag and therefore might give the impression that the path/functionality is avail across OSes. But this is a somewhat minor issue and quite debatable. Thank you.
A few more comments. You may notice that we are a bit rigid on style. This makes writing code slightly harder at first, but it makes reading others code far easier. If you have not seen the style guide, take a look: http://dev.chromium.org/developers/coding-style http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:42: #if defined(OS_LINUX) You only use this include on non-chrome os linux, so how about: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:43: #include "chrome/browser/extensions/external_filesystem_extension_loader_linux.h" Includes should be in alphabetical order, so this goes before external_registry_extension_loader_win.h . http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:367: service, Consider passing in the path constant to the constructor of ExternalFilesystemExtensionLoader(). http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:20: base::ValueSerializer* serializer) { Style nit: "const" and "base::" should line up in the same column. That might make the line with "base::" longer than 80 characters. If so, you can indent all parameters by four spaces, like this: DictionaryValue* ExternalExtensionUtil::ExtractPrefs( const FilePath& path, base::ValueSerializer* serializer) { http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.h (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:11: class ListValue; I think you don't use DictionaryValue or ListValue. If so, please remove. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:15: class FilePath; File path is declared both inside namespace base and outside of any namespace. Looking at other files (grep 'class FilePath' ~/cr/gitMac/src/chrome/browser/extensions/ ), I think the convention is to use FilePath over base::FilePath. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:21: , base::ValueSerializer* serializer); Style nit: comma belongs on the previous line, and indentation should make "const" and "base::" start in the same column. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:28: DictionaryValue* ExtractPrefs(const FilePath& path, The utility method makes this obsolete? http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:58: const FilePath & external_extension_search_path) { Style nit: No space between "FilePath" and "&". http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:87: DVLOG(1) << "Not considering: " << reader.name(); Say why you are not considering a file. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:108: if (!PathService::Get(chrome::DIR_SINGLE_EXTERNAL_EXTENSIONS Style nit: Coma should be on previous line. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:114: // First list the potential .json candidates (excluding some like I think the last part of this comment is out of date. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:116: testing_prefs_.reset(ExternalExtensionUtil::ExtractPrefs(fake_json_path Coma should be on previous line. http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.cc File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.... chrome/common/chrome_paths.cc:77: // definition files. A comment with a link to a standard will help avoid a flamewar when a specific linux distribution demands that we use their preferred path :) You mentioned http://www.pathname.com/fhs/ , which seems like a good choice. http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.h File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.... chrome/common/chrome_paths.h:59: DIR_SINGLE_EXTERNAL_EXTENSIONS, // Directory for 'per-extension' definition > I am not too sure about this one, since I added a path-enum in the > chrome_paths > that is not surrounded by any specific #define flag and therefore > might give the > impression that the path/functionality is avail across OSes. But > this is a somewhat minor issue and quite debatable. Yea, these things are debatable, and we debate them a lot on mailing lists and IRC. The team generally works to be consistent, and this makes it easier to read new areas of the code. In this case, I think the consistant thing to do is to wrap this constant in an #if defined(OS_LINUX) && !defined(OS_CHROME_OS), to ensure that it can not be used on other platforms. This will mean you can't use the path constant as-is in ExternalFilesystemExtensionLoader. Class ExternalPrefExtensionLoader gets around this problem by passing the constant as an integer to the constructor. The constant appears in external_extension_provider_impl.cc line 318.
http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:42: #if defined(OS_LINUX) On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > You only use this include on non-chrome os linux, so how about: > > #if defined(OS_LINUX) && !defined(OS_CHROMEOS) Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:43: #include "chrome/browser/extensions/external_filesystem_extension_loader_linux.h" On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Includes should be in alphabetical order, so this goes before > external_registry_extension_loader_win.h . Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:367: service, On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Consider passing in the path constant to the constructor of > ExternalFilesystemExtensionLoader(). Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:20: base::ValueSerializer* serializer) { On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Style nit: "const" and "base::" should line up in the same column. That might > make the line with "base::" longer than 80 characters. If so, you can indent > all parameters by four spaces, like this: > > DictionaryValue* ExternalExtensionUtil::ExtractPrefs( > const FilePath& path, > base::ValueSerializer* serializer) { Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.h (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:11: class ListValue; On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > I think you don't use DictionaryValue or ListValue. If so, please remove. Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:15: class FilePath; On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > I think the convention is to use > FilePath over base::FilePath. yes, from what I see http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:21: , base::ValueSerializer* serializer); On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Style nit: comma belongs on the previous line, and indentation should make > "const" and "base::" start in the same column. Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:28: DictionaryValue* ExtractPrefs(const FilePath& path, On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > The utility method makes this obsolete? Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:58: const FilePath & external_extension_search_path) { On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Style nit: No space between "FilePath" and "&". Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:87: DVLOG(1) << "Not considering: " << reader.name(); On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Say why you are not considering a file. > Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:108: if (!PathService::Get(chrome::DIR_SINGLE_EXTERNAL_EXTENSIONS On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Style nit: Coma should be on previous line. Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:114: // First list the potential .json candidates (excluding some like On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > I think the last part of this comment is out of date. Done. http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:116: testing_prefs_.reset(ExternalExtensionUtil::ExtractPrefs(fake_json_path On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > Coma should be on previous line. Done. http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.cc File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.... chrome/common/chrome_paths.cc:77: // definition files. On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > A comment with a link to a standard will help avoid a flamewar when a specific > linux distribution demands that we use their preferred path :) > > You mentioned http://www.pathname.com/fhs/ , which seems like a good choice. Done. http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.h File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/9963120/diff/10016/chrome/common/chrome_paths.... chrome/common/chrome_paths.h:59: DIR_SINGLE_EXTERNAL_EXTENSIONS, // Directory for 'per-extension' definition On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: > > I am not too sure about this one, since I added a path-enum in the > > chrome_paths > > that is not surrounded by any specific #define flag and therefore > > might give the > > impression that the path/functionality is avail across OSes. But > > this is a somewhat minor issue and quite debatable. > > Yea, these things are debatable, and we debate them a lot on mailing lists and > IRC. The team generally works to be consistent, and this makes it easier to > read new areas of the code. In this case, I think the consistant thing to do is > to wrap this constant in an #if defined(OS_LINUX) && !defined(OS_CHROME_OS), to > ensure that it can not be used on other platforms. > > This will mean you can't use the path constant as-is in > ExternalFilesystemExtensionLoader. Class ExternalPrefExtensionLoader gets > around this problem by passing the constant as an integer to the constructor. > The constant appears in external_extension_provider_impl.cc line 318. > > Done.
Two minor nits, otherwise looks good. I submitted try jobs for this patch. They will try to compile and run tests on several platforms. When they start, you will see yellow boxes at the bottom of your patch at http://codereview.chromium.org/9963120 . Eventually, they will turn green on success or red on failure. Take a look at the results (by clicking the boxes) when they come. Compile failures are certainly a problem. Test failures are more subtile: Some tests are flaky, and you need to look at what they test to decide if your change broke them. http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.cc (right): http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:18: // Caller takes ownership of the returned dictionary. Now that this is in its own file, a comment explaining what it does and who uses it would be helpful. http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.h (right): http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:18: static base::DictionaryValue* ExtractPrefs(const FilePath& path , on wrong line.
Hmm, looks like the try results will not show up on the code review, but get emailed to me, because I did not create the issue. Will forward them as they arrive. On 2012/04/13 22:11:02, Sam Kerner (Chrome) wrote: > Two minor nits, otherwise looks good. > > I submitted try jobs for this patch. They will try to compile and run tests on > several platforms. When they start, you will see yellow boxes at the bottom of > your patch at http://codereview.chromium.org/9963120 . Eventually, they will > turn green on success or red on failure. Take a look at the results (by > clicking the boxes) when they come. Compile failures are certainly a problem. > Test failures are more subtile: Some tests are flaky, and you need to look at > what they test to decide if your change broke them. > > http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... > File chrome/browser/extensions/external_extension_util.cc (right): > > http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... > chrome/browser/extensions/external_extension_util.cc:18: // Caller takes > ownership of the returned dictionary. > Now that this is in its own file, a comment explaining what it does and who uses > it would be helpful. > > http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... > File chrome/browser/extensions/external_extension_util.h (right): > > http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... > chrome/browser/extensions/external_extension_util.h:18: static > base::DictionaryValue* ExtractPrefs(const FilePath& path > , on wrong line.
http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.cc (right): http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:18: // Caller takes ownership of the returned dictionary. On 2012/04/13 22:11:02, Sam Kerner (Chrome) wrote: > Now that this is in its own file, a comment explaining what it does and who uses > it would be helpful. Done. http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.h (right): http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:18: static base::DictionaryValue* ExtractPrefs(const FilePath& path On 2012/04/13 22:11:02, Sam Kerner (Chrome) wrote: > , on wrong line. Done.
On 2012/04/13 22:36:10, Sam Kerner (Chrome) wrote: > Hmm, looks like the try results will not show up on the code review, but get > emailed to me, because I did not create the issue. Will forward them as they > arrive. I saw a few test failures but none really that I could relate directly or indirectly to my changes. Not sure if these are somewhat dependant on the environment and therefore prone to occasional failures.
LGTM I will commit this change on your behalf. Docs need to be updated to explain how to use the new source. Can you create a new change that edits src/chrome/common/extensions/docs/static/external_extensions.html to explain how to use the new source? I would make this a new top level section, between "Using the Windows registry" and "Updating and uninstalling". Our documentation is built by editing the files in src/chrome/common/extensions/docs/static, then running a script that produces the files one directory level higher. To edit external_extensions.html: 1) Build all chrome targets. One of them is used by the document generation script. 2) Edit src/chrome/common/extensions/docs/static/external_extensions.html . 3) From src/, run chrome/common/extensions/docs/build/build.py . 4) Take a look at src/chrome/common/extensions/docs/external_extensions.html in a browser. (Note that the path does not contain "static", because this is the output of the build script). 5) If you are happy with the docs, upload a change including both static/external_extensions.html and external_extensions.html, and request a review. Otherwise, goto step 2. On 2012/04/13 23:45:16, Alexandre Abreu wrote: > On 2012/04/13 22:36:10, Sam Kerner (Chrome) wrote: > > Hmm, looks like the try results will not show up on the code review, but get > > emailed to me, because I did not create the issue. Will forward them as they > > arrive. > > I saw a few test failures but none really that I could relate directly or > indirectly > to my changes. Not sure if these are somewhat dependant on the environment and > therefore > prone to occasional failures.
When I apply this patch to trunk, I am getting several failures, because the code being patched has changed. Please run gclient sync, resolve any conflicts, and upload a new patch set. On 2012/04/18 14:56:45, Sam Kerner (Chrome) wrote: > LGTM > > I will commit this change on your behalf. > > Docs need to be updated to explain how to use the new source. Can you create a > new change that edits > src/chrome/common/extensions/docs/static/external_extensions.html > to explain how to use the new source? I would make this a new top level > section, between "Using the Windows registry" and "Updating and uninstalling". > > Our documentation is built by editing the files in > src/chrome/common/extensions/docs/static, then running a script that produces > the files one directory level higher. To edit external_extensions.html: > > 1) Build all chrome targets. One of them is used by the document generation > script. > 2) Edit src/chrome/common/extensions/docs/static/external_extensions.html . > 3) From src/, run chrome/common/extensions/docs/build/build.py . > 4) Take a look at src/chrome/common/extensions/docs/external_extensions.html in > a browser. (Note that the path does not contain "static", because this is the > output of the build script). > 5) If you are happy with the docs, upload a change including both > static/external_extensions.html and external_extensions.html, and request a > review. Otherwise, goto step 2. > > > > On 2012/04/13 23:45:16, Alexandre Abreu wrote: > > On 2012/04/13 22:36:10, Sam Kerner (Chrome) wrote: > > > Hmm, looks like the try results will not show up on the code review, but get > > > emailed to me, because I did not create the issue. Will forward them as > they > > > arrive. > > > > I saw a few test failures but none really that I could relate directly or > > indirectly > > to my changes. Not sure if these are somewhat dependant on the environment and > > therefore > > prone to occasional failures.
On 2012/04/18 15:59:33, Sam Kerner (Chrome) wrote: > When I apply this patch to trunk, I am getting several failures, because the > code being patched has changed. Please run gclient sync, resolve any conflicts, > and upload a new patch set. This one works for me for a very up-to-date tree
On 2012/04/18 14:56:45, Sam Kerner (Chrome) wrote: > > Docs need to be updated to explain how to use the new source. Can you create a > new change that edits > src/chrome/common/extensions/docs/static/external_extensions.html > to explain how to use the new source? I would make this a new top level > section, between "Using the Windows registry" and "Updating and uninstalling". > > Our documentation is built by editing the files in > src/chrome/common/extensions/docs/static, then running a script that produces > the files one directory level higher. To edit external_extensions.html: > > 1) Build all chrome targets. One of them is used by the document generation > script. > 2) Edit src/chrome/common/extensions/docs/static/external_extensions.html . > 3) From src/, run chrome/common/extensions/docs/build/build.py . > 4) Take a look at src/chrome/common/extensions/docs/external_extensions.html in > a browser. (Note that the path does not contain "static", because this is the > output of the build script). > 5) If you are happy with the docs, upload a change including both > static/external_extensions.html and external_extensions.html, and request a > review. Otherwise, goto step 2. Done. I added the two .html (static and plain) to the CL.
+mkearney to review the extension documentation changes.
I'm assuming this CL didn't change from what I reviewed when Sam posted it (http://codereview.chromium.org/10179002/). At least it didn't look different. So, I'm continuing from where we left off there... But before I do I have what I think is an important question that might make some of my comments below redundant (and change your approach somewhat). The thing is, I'm starting to believe, after looking at this again, that a better approach to this problem is to modify ExternalPrefExtensionLoader::ReadJsonPrefsFile() to have it not just load external_extensions.json but also all the individual <my_id>.json files it finds in the same directory, parse them and add their contents to the same dictionary (parsed_json_prefs) before returning from that function. You would still add the extra Linux special directory and a new ExternalPrefExtensionLoader (but not the ExternalFilesystemExtensionLoaderLinux). In fact, this approach could be made cross-platform so other platforms would benefit from this too. What do you think? Also, a comment on the changelist description: BUG=foo not BUG:foo TEST= also needs to be filled out. I could have sworn we had tests for the External Extension functionality but now I don't see them. If there are no tests then "TEST=None" would do. :/ http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:368: new ExternalFilesystemExtensionLoader( As mentioned, I think a new loader is not required, only a new provider. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:24: << " In file " << path.value() << " ."; There is still a whitespace before period in the log. Don't capitalize |In|. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:28: DictionaryValue * ext_dictionary = NULL; DictionaryValue* not DictionaryValue * http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:31: } nit: We don't use braces if the |if| clause (or the |else| clause) is one line long. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:34: << path.value() << " ."; Same here (remove space before period). http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.h (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:16: class ExternalExtensionUtil { I still don't see a reason why this is a class and not just a namespace called ExternalExtensionUtil? http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:25: base::ValueSerializer* serializer); Can you add WARN_UNUSED_RESULT to the end (before the semicolon)? http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:70: {} We prefer: func() { } over func() {} Also, while I'm a fan of keeping the ctor and dtor together the style guide says declaration order and definition order should match (this recently came up on the chromium-dev mailing list), so this needs to move down below StartLoading (line 79). http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:79: const FilePath ExternalFilesystemExtensionLoader::GetBaseCrxFilePath() { Um... where is this function used and why is it important that we access this member only on the UI thread? http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:121: &serializer); When splitting arguments between lines, we prefer: function(foo, bar); ... or (depending on which is more readable) ... function( foo, bar); ... over ... function(foo, bar); http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_filesystem_extension_loader_linux.h (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:12: class ExternalFilesystemExtensionLoader : public ExternalExtensionLoader { Classes need class-level documentation. Doesn't have to be long, just a couple of sentences that describe what they do. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:16: // to this path. Suggest: |base_path_id| is the resource id of the base path containing the json description of which extensions to load. File paths to extension files are resolved relative to this path. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:17: explicit ExternalFilesystemExtensionLoader(int base_path_key); Can you rename this to |base_path_id|? http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:29: void LoadOnFileThread(); This should be documented also. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:31: // See the constructor's base_path_key. Suggest: The resource id of the base path containing the json file containing which extensions to load. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:32: int base_path_key_; Same here: base_path_id. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:34: // Concrete path corresponding to the base_path_key_. Suggest: The path containing the json file containing which extensions to load. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:40: #endif This is missing the // CHROME_BROWSER_EXTENSIONS_EXTERNAL_FILESYSTEM_EXTENSION_LOADER_LINUX_H_ http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:83: &serializer); nit: This fits on one line. No? http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.cc File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.... chrome/common/chrome_paths.cc:77: // definition files. Suggest: The path to the external extension <id>.json files. http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.h File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.... chrome/common/chrome_paths.h:61: // installed when chrome is run. OK, this is an improvement, but I think we can still improve on it. You should probably name this variable after the path that it identifies, and not the purpose for which you want to use it. I suggest: DIR_EXTERNAL_EXTENSIONS_LINUX_USER_SHARE, or something to that effect. Same goes for the comment. "The /usr/share/ directory containing the <extension id>.json files that list what extensions to install. Getting this path does not create it." http://codereview.chromium.org/9963120/diff/37001/chrome/common/extensions/do... File chrome/common/extensions/docs/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/37001/chrome/common/extensions/do... chrome/common/extensions/docs/external_extensions.html:210: <a href="#per-extension-install">Using per-extension preference files (Linux only)</a> If you augment the current Pref provider instead of adding a new loader, as I suggested, then you can actually change the docs to just talk about your method (I think it is an improvement over the external_extensions.json method and will become the default). We still would need to support external_extension.json, but we don't need to point people to it if there is a cleaner way... The same comment goes for all the changes in this file.
> But before I do I have what I think is an important question that might make > some of my comments below redundant (and change your approach somewhat). The > thing is, I'm starting to believe, after looking at this again, that a better > approach to this problem is to modify > ExternalPrefExtensionLoader::ReadJsonPrefsFile() to have it not just load > external_extensions.json but also all the individual <my_id>.json files it finds > in the same directory, parse them and add their contents to the same dictionary > (parsed_json_prefs) before returning from that function. I am not too sure about that. Although, the two preference classes do share some bits, they actually have different constraints that might justify some separation, e.g.: * one extension definition per-json file in one case more that one in the other case, * differences in the expected format (extension id), * more than one 'base path' (one for the external_extension.json file and another one corresponding to '/usr/share') which changes quite a bit the 'semantics' of the ExternalExtensionLoader class, etc. To me this kind of justified to "physically" isolate the two (besides the fact that I didn't want my changes to be too invasive into another 'stable' feature). > In fact, this approach could be made > cross-platform so other platforms would benefit from this too. IMHO, this would be a nice side effect but for the reason stated above I am not sure that it would be that beneficial at this point. Would there be a need to make it available to other "platforms" both classes would benefit from being properly refactored, http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:24: << " In file " << path.value() << " ."; On 2012/04/25 13:33:45, Finnur wrote: > There is still a whitespace before period in the log. > Don't capitalize |In|. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:28: DictionaryValue * ext_dictionary = NULL; On 2012/04/25 13:33:45, Finnur wrote: > DictionaryValue* > not > DictionaryValue * Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:31: } On 2012/04/25 13:33:45, Finnur wrote: > nit: We don't use braces if the |if| clause (or the |else| clause) is one line > long. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.cc:34: << path.value() << " ."; On 2012/04/25 13:33:45, Finnur wrote: > Same here (remove space before period). Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_util.h (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:16: class ExternalExtensionUtil { On 2012/04/25 13:33:45, Finnur wrote: > I still don't see a reason why this is a class and not just a namespace called > ExternalExtensionUtil? Agree, as in many other places, I tried to follow what seemed to be "Chromium's way". I have seen this in some other places hence the same pattern ... Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_util.h:25: base::ValueSerializer* serializer); On 2012/04/25 13:33:45, Finnur wrote: > Can you add WARN_UNUSED_RESULT to the end (before the semicolon)? Sorry missed that. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_filesystem_extension_loader_linux.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:70: {} On 2012/04/25 13:33:45, Finnur wrote: > We prefer: > func() { > } > > over > > func() > {} > > Also, while I'm a fan of keeping the ctor and dtor together the style guide says > declaration order and definition order should match (this recently came up on > the chromium-dev mailing list), so this needs to move down below StartLoading > (line 79). Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:79: const FilePath ExternalFilesystemExtensionLoader::GetBaseCrxFilePath() { On 2012/04/25 13:33:45, Finnur wrote: > Um... where is this function used it is used in external_extension_provider_impl.cc, if for a given standalone extension, the actual extension's crx path is a relative path. The 'base CRX file path' is then used as a base path. > and why is it important that we access this member only on the UI thread? It is a 'required' semantic as specified by the ExternalExtensionLoader class: // Instances are created on the UI thread and expect public method calls from // the UI thread. Some subclasses introduce new methods that are executed on the // FILE thread. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.cc:121: &serializer); On 2012/04/25 13:33:45, Finnur wrote: > When splitting arguments between lines, we prefer: > > function(foo, > bar); > > ... or (depending on which is more readable) ... > > function( > foo, bar); > > ... over ... > > function(foo, > bar); Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_filesystem_extension_loader_linux.h (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:12: class ExternalFilesystemExtensionLoader : public ExternalExtensionLoader { On 2012/04/25 13:33:45, Finnur wrote: > Classes need class-level documentation. Doesn't have to be long, just a couple > of sentences that describe what they do. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:16: // to this path. On 2012/04/25 13:33:45, Finnur wrote: > Suggest: |base_path_id| is the resource id of the base path containing the json > description of which extensions to load. File paths to extension files are > resolved relative to this path. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:17: explicit ExternalFilesystemExtensionLoader(int base_path_key); On 2012/04/25 13:33:45, Finnur wrote: > Can you rename this to |base_path_id|? Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:29: void LoadOnFileThread(); On 2012/04/25 13:33:45, Finnur wrote: > This should be documented also. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:31: // See the constructor's base_path_key. On 2012/04/25 13:33:45, Finnur wrote: > Suggest: The resource id of the base path containing the json file containing > which extensions to load. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:32: int base_path_key_; On 2012/04/25 13:33:45, Finnur wrote: > Same here: base_path_id. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:34: // Concrete path corresponding to the base_path_key_. On 2012/04/25 13:33:45, Finnur wrote: > Suggest: The path containing the json file containing which extensions to load. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_filesystem_extension_loader_linux.h:40: #endif On 2012/04/25 13:33:45, Finnur wrote: > This is missing the > // CHROME_BROWSER_EXTENSIONS_EXTERNAL_FILESYSTEM_EXTENSION_LOADER_LINUX_H_ Done. http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:83: &serializer); On 2012/04/25 13:33:45, Finnur wrote: > nit: This fits on one line. No? Done. http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.cc File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.... chrome/common/chrome_paths.cc:77: // definition files. On 2012/04/25 13:33:45, Finnur wrote: > Suggest: The path to the external extension <id>.json files. Done. http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.h File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.... chrome/common/chrome_paths.h:61: // installed when chrome is run. On 2012/04/25 13:33:45, Finnur wrote: > OK, this is an improvement, but I think we can still improve on it. You should > probably name this variable after the path > that it identifies, and not the purpose for which you want to use it. I suggest: > DIR_EXTERNAL_EXTENSIONS_LINUX_USER_SHARE, or something to that effect. > > Same goes for the comment. > > "The /usr/share/ directory containing the <extension id>.json files that list > what extensions to install. Getting this path does not create it." mmh, I am really not too sure about that one. Although /usr/share seems like a reasonable path it is really sort of an implementation detail. I am not too fond of naming something based on the "theoretical" value that it has a given moment in time or even the OS the it is current associated to.
> I am not too sure about that.
> Although, the two preference classes do share some bits, they actually have
> different constraints that might justify some separation, e.g.:
> * one extension definition per-json file in one case more that one in the
other case,
> * differences in the expected format (extension id),
The formats are not materially different:
external_extensions.json:
{
"aaaaaaaaaabbbbbbbbbbcccccccccc": {
"external_crx": "<path>",
"external_version": "x.x"
},
// and possibly more extensions
}
aaaaaaaaaabbbbbbbbbbcccccccccc.json
{
"external_crx": "<path>",
"external_vrsion": "x.x"
}
You'd need to browse the directory for *.json files (possibly resolve the path
-- and I'm not even sure that's needed), but once you have the data and add it
to the dictionary then I believe the (already committed) code takes care of the
rest. You already have written the code that enumerates the <id>.json files to
grab the id, path and version and that's probably the biggest piece of the
puzzle.
> * more than one 'base path' (one for the external_extension.json file and
> another one corresponding to '/usr/share') which changes quite a bit the
> 'semantics' of the ExternalExtensionLoader class, etc.
We already support having more than one base path. That's why the
ExternalExtensionLoader takes the id of a directory as a parameter. It is so
that we can look for external_extensions.json in multiple directories and have
platform-specific directories. You'd still need to create a new loader, of
course, but not of the new type you just added, but the general
ExternalExtensionLoader and specify with it the new Linux dirextory id you just
created. With this approach you can put <id>.json files where we currently have
external_extension.json and we can deprecate external_extension.json, which is a
good thing because individual id.json files are much easier to deal with than
modifying external_extension.json.
> To me this kind of justified to "physically" isolate the two (besides the fact
> that I didn't want my changes to be too invasive into another 'stable'
feature).
I understand your concern, but we shouldn't let the fact that something hasn't
changed in a while deter us from doing what is right.
http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths.h
File chrome/common/chrome_paths.h (right):
http://codereview.chromium.org/9963120/diff/37001/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:61: // installed when chrome is run.
OK, fine.
How about this comment then?
"The directory containing the <extension id>.json files that list what
extensions to install. Getting this path does not create it."
Uploaded a new CL following the approach suggested by finnur
I like it! We are very close. The below is mostly just a bunch of nits and requests to change docs (and comments) and add a warning. General comment: Please update the changelist description in Rietveld as it is now out of date (you aren't adding a new loader type). http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.cc (right): http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:58: // Extracts/expect a file content in json format. 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). http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:116: } nit: Avoid braces for single line if clauses. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:120: ReadExternalExtensionPrefFile(prefs.get()); 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 http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:121: ReadStandaloneExtensionPrefFiles(prefs.get()); 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? http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:177: if (NULL != ext_prefs) nit: if (ext_prefs) http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:211: if (NULL != ext_prefs) { nit: if (ext_prefs) http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:213: nit: delete this extra linebreak? http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.h (right): http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:17: // A specialization of the ExternalExtensionLoader that uses a json file to update comment ("a json file") http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:52: // Must be dispatched/called on the file thread. nit: Drop the word dispatched. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:55: // Tries to read and extract the information contained in a Suggest: s/Tries to read and extract/Extracts/. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:56: // external_extension.json file. Add: ... regarding what external extensions to install. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:57: // |prefs| will be modified to receive the extracted extension Nit: Merge this with line 56 (don't add a linebreak before this line). http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:62: // Tries to read and extract the information contained in Suggest: s/Tries to read and extract/Extracts/. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:65: // <extension id>.json Suggest: ... contained in standalone external extension json files (<extension id>.json) regarding what external extensions to install. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:71: // The resource id of the base path containing the json file containing Second 'containing' should be 'with the information about'. http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... File chrome/common/extensions/docs/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... chrome/common/extensions/docs/external_extensions.html:210: <a href="#per-extension-install">Using per-extension preference files (Linux only)</a> Um... I would tackle the documentation differently. We still have only two methods (Prefs and Registry). So, I would drop this special chapter and instead modify the Linux section of the Preferences to say that a new directory has been added. Then remove all references to external_extensions.json (from the docs only) and just talk about <extension_id.json> files. I would also make a note in the doc of the fact that external_extensions.json files are deprecated (even though we continue to support them). Sort of like the Windows note at the top of Using a preferences file in http://code.google.com/chrome/extensions/external_extensions.html. http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... chrome/common/extensions/docs/external_extensions.html:251: Google Chrome supports three ways of Two. And revert the next two changes (line 257, 260). http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... chrome/common/extensions/docs/external_extensions.html:455: </li> Part of this segment should be deleted and parts (the new directory, the new file naming and format scheme, etc) merged elsewhere.
http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.cc (right): http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:58: // Extracts/expect a file content in json format. On 2012/04/27 10:50:28, Finnur wrote: > 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). Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:116: } On 2012/04/27 10:50:28, Finnur wrote: > nit: Avoid braces for single line if clauses. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:120: ReadExternalExtensionPrefFile(prefs.get()); On 2012/04/27 10:50:28, Finnur wrote: > 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 Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:121: ReadStandaloneExtensionPrefFiles(prefs.get()); On 2012/04/27 10:50:28, Finnur wrote: > 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? Yes, http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:177: if (NULL != ext_prefs) On 2012/04/27 10:50:28, Finnur wrote: > nit: if (ext_prefs) Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:211: if (NULL != ext_prefs) { On 2012/04/27 10:50:28, Finnur wrote: > nit: if (ext_prefs) Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.cc:213: On 2012/04/27 10:50:28, Finnur wrote: > nit: delete this extra linebreak? Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.h (right): http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:52: // Must be dispatched/called on the file thread. On 2012/04/27 10:50:28, Finnur wrote: > nit: Drop the word dispatched. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:55: // Tries to read and extract the information contained in a On 2012/04/27 10:50:28, Finnur wrote: > Suggest: s/Tries to read and extract/Extracts/. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:56: // external_extension.json file. On 2012/04/27 10:50:28, Finnur wrote: > Add: ... regarding what external extensions to install. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:57: // |prefs| will be modified to receive the extracted extension On 2012/04/27 10:50:28, Finnur wrote: > Nit: Merge this with line 56 (don't add a linebreak before this line). Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:62: // Tries to read and extract the information contained in On 2012/04/27 10:50:28, Finnur wrote: > Suggest: s/Tries to read and extract/Extracts/. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:65: // <extension id>.json On 2012/04/27 10:50:28, Finnur wrote: > Suggest: ... contained in standalone external extension json files (<extension > id>.json) regarding what external extensions to install. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:71: // The resource id of the base path containing the json file containing On 2012/04/27 10:50:28, Finnur wrote: > Second 'containing' should be 'with the information about'. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... File chrome/common/extensions/docs/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... chrome/common/extensions/docs/external_extensions.html:210: <a href="#per-extension-install">Using per-extension preference files (Linux only)</a> On 2012/04/27 10:50:28, Finnur wrote: > Um... I would tackle the documentation differently. We still have only two > methods (Prefs and Registry). So, I would drop this special chapter and instead > modify the Linux section of the Preferences to say that a new directory has been > added. > > Then remove all references to external_extensions.json (from the docs only) and > just talk about <extension_id.json> files. > > I would also make a note in the doc of the fact that external_extensions.json > files are deprecated (even though we continue to support them). Sort of like the > Windows note at the top of Using a preferences file in > http://code.google.com/chrome/extensions/external_extensions.html. Done. http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... chrome/common/extensions/docs/external_extensions.html:251: Google Chrome supports three ways of On 2012/04/27 10:50:28, Finnur wrote: > Two. And revert the next two changes (line 257, 260). Done. http://codereview.chromium.org/9963120/diff/44003/chrome/common/extensions/do... chrome/common/extensions/docs/external_extensions.html:455: </li> On 2012/04/27 10:50:28, Finnur wrote: > Part of this segment should be deleted and parts (the new directory, the new > file naming and format scheme, etc) merged elsewhere. Done.
Very good. Almost there... http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.h (right): http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:55: // Extracts the information contained in a external_extension.json file nit: a -> an (I missed that last time) http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:90: still supported for now. Suggest: Previous versions of Google Chrome used an <code>external_extensions.json</code> file to specify which extensions to install. This file has been deprecated in favor of individual <code>.json</code> files, one per extension. http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:182: <p>On Mac OS, the external extensions files for all users is only read if file system permissions prevent unprivelaged users from changing it. If you do not see external extensions installed when Chrome is launched, there may be a permissions problem with the external extensions preferences files. To see if this is the problem, follow these steps:</p> s/is/are s/unprivelaged/unprivileged/ http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:188: <li> Search for the string <b>Can not read external extensions</b>. If there is a problem reading the external extensions file, you will find an error message. Look for another error message directly above it, which should explain the issue. For example, if you see the following error: s/file/files/ s/find/see/ http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:241: remove the metadata from the preferences file ... remove your preference file (aaaaaaaaaabbbbbbbbbbcccccccccc.json) or the metadata from the registry. http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:269: is the extension's ID) are in the wrong location or the specified extension IDs are not valid</li> Suggest: The .json file (<code>aaaaaaaaaabbbbbbbbbbcccccccccc.json</code>) is in the wrong location or the ID specified does not match the extension ID.
http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/e... File chrome/browser/extensions/external_pref_extension_loader.h (right): http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/e... chrome/browser/extensions/external_pref_extension_loader.h:55: // Extracts the information contained in a external_extension.json file On 2012/04/27 14:29:43, Finnur wrote: > nit: a -> an (I missed that last time) Done. http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:90: still supported for now. On 2012/04/27 14:29:43, Finnur wrote: > Suggest: Previous versions of Google Chrome used an > <code>external_extensions.json</code> file to specify which extensions to > install. This file has been deprecated in favor of individual <code>.json</code> > files, one per extension. Done. http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:182: <p>On Mac OS, the external extensions files for all users is only read if file system permissions prevent unprivelaged users from changing it. If you do not see external extensions installed when Chrome is launched, there may be a permissions problem with the external extensions preferences files. To see if this is the problem, follow these steps:</p> On 2012/04/27 14:29:43, Finnur wrote: > s/is/are > s/unprivelaged/unprivileged/ Done. http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:269: is the extension's ID) are in the wrong location or the specified extension IDs are not valid</li> On 2012/04/27 14:29:43, Finnur wrote: > Suggest: The .json file (<code>aaaaaaaaaabbbbbbbbbbcccccccccc.json</code>) is in > the wrong location or the ID specified does not match the extension ID. Done.
Only two left (from last list). http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:188: <li> Search for the string <b>Can not read external extensions</b>. If there is a problem reading the external extensions file, you will find an error message. Look for another error message directly above it, which should explain the issue. For example, if you see the following error: Missed a couple here. :) On 2012/04/27 14:29:43, Finnur wrote: > s/file/files/ > s/find/see/ http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:241: remove the metadata from the preferences file And this one. On 2012/04/27 14:29:43, Finnur wrote: > ... remove your preference file (aaaaaaaaaabbbbbbbbbbcccccccccc.json) or the > metadata from the registry.
http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:188: <li> Search for the string <b>Can not read external extensions</b>. If there is a problem reading the external extensions file, you will find an error message. Look for another error message directly above it, which should explain the issue. For example, if you see the following error: On 2012/04/27 14:56:09, Finnur wrote: > Missed a couple here. :) > > On 2012/04/27 14:29:43, Finnur wrote: > > s/file/files/ > > s/find/see/ > Done. http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:241: remove the metadata from the preferences file On 2012/04/27 14:56:09, Finnur wrote: > And this one. > > On 2012/04/27 14:29:43, Finnur wrote: > > ... remove your preference file (aaaaaaaaaabbbbbbbbbbcccccccccc.json) or the > > metadata from the registry. > Done.
http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:241: remove the metadata from the preferences file Not quite... Read it again. :) On 2012/04/27 15:03:13, Alexandre Abreu wrote: > On 2012/04/27 14:56:09, Finnur wrote: > > And this one. > > > > On 2012/04/27 14:29:43, Finnur wrote: > > > ... remove your preference file (aaaaaaaaaabbbbbbbbbbcccccccccc.json) or the > > > metadata from the registry. > > > > Done.
http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/do... chrome/common/extensions/docs/static/external_extensions.html:241: remove the metadata from the preferences file On 2012/04/27 15:34:16, Finnur wrote: > Not quite... Read it again. :) > > On 2012/04/27 15:03:13, Alexandre Abreu wrote: > > On 2012/04/27 14:56:09, Finnur wrote: > > > And this one. > > > > > > On 2012/04/27 14:29:43, Finnur wrote: > > > > ... remove your preference file (aaaaaaaaaabbbbbbbbbbcccccccccc.json) or > the > > > > metadata from the registry. > > > > > > > Done. > Oops, went too fast over this one. Done.
LGTM. I'm running out of time for today, but I can try this change on Monday if you feel it needs more baking. I wouldn't commit it without a run on the try servers, at least.
On 2012/04/27 15:44:37, Finnur wrote: > LGTM. I'm running out of time for today, but I can try this change on Monday if > you feel it needs more baking. I wouldn't commit it without a run on the try > servers, at least. For sure. Monday it is then unless you have time today,
OK, I've tested this and it doesn't quite work. There were compile errors on Windows, some because of FilePath::StringType is being defined to different things depending on platform and some because the CL wasn't compiled before updating after latest round of review comments. :/ Fear not, however, as I have updated the changelist to compile on Windows and tested it end to end on Windows: https://chromiumcodereview.appspot.com/10260010/ Please try it out on Linux. I can have Sam review my changes (only one file changed really) and check in that way, or if there is need to augment it further you can take my changelist and update your local copy and we can then continue on this thread.
On 2012/04/30 13:29:49, Finnur wrote: > OK, I've tested this and it doesn't quite work. There were compile errors on > Windows, some because of FilePath::StringType is being defined to different > things depending on platform and some because the CL wasn't compiled before > updating after latest round of review comments. :/ Yes, I went too fast on the latest-latest changes & forgot to test, thank you for fixing it, > Fear not, however, as I have updated the changelist to compile on Windows and > tested it end to end on Windows: > https://chromiumcodereview.appspot.com/10260010/ > Please try it out on Linux. Works for me as expected, > I can have Sam review my changes (only one file changed really) and check in > that way, or if there is need to augment it further you can take my changelist > and update your local copy and we can then continue on this thread. I looked at & tested the changes and they seem OK overall, |
