Chromium Code Reviews
Help | Chromium Project | Sign in
(626)

Issue 9963120: Introduces an additional extension loader that load extra extensions based on per-extension json fi… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by Alexandre Abreu
Modified:
3 months, 1 week ago
CC:
chromium-reviews_chromium.org, Aaron Boodman, mihaip+watch_chromium.org
Base URL:
https://src.chromium.org/svn/trunk/src/
Visibility:
Public.

Description

Modifies 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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+262 lines, -100 lines) Lint Patch
M AUTHORS View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -1 line 0 comments 0 errors Download
M chrome/browser/extensions/external_extension_provider_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +14 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/extensions/external_pref_extension_loader.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +27 lines, -6 lines 0 comments 0 errors Download
M chrome/browser/extensions/external_pref_extension_loader.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +130 lines, -38 lines 0 comments 0 errors Download
M chrome/common/chrome_paths.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments 0 errors Download
M chrome/common/chrome_paths.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +18 lines, -0 lines 0 comments 0 errors Download
M chrome/common/extensions/docs/external_extensions.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 9 chunks +32 lines, -27 lines 0 comments 0 errors Download
M chrome/common/extensions/docs/static/external_extensions.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 9 chunks +33 lines, -28 lines 0 comments 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 29
Sam Kerner (Chrome)
Alexandre, if you have not yet read our wiki page on contributing code, please take ...
2 years ago #1
Alexandre Abreu
On 2012/04/13 01:35:27, Sam Kerner (Chrome) wrote: > Alexandre, if you have not yet read ...
2 years ago #2
Sam Kerner (Chrome)
A few more comments. You may notice that we are a bit rigid on style. ...
2 years ago #3
Alexandre Abreu
http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/external_extension_provider_impl.cc File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/9963120/diff/10016/chrome/browser/extensions/external_extension_provider_impl.cc#newcode42 chrome/browser/extensions/external_extension_provider_impl.cc:42: #if defined(OS_LINUX) On 2012/04/13 19:35:13, Sam Kerner (Chrome) wrote: ...
2 years ago #4
Sam Kerner (Chrome)
Two minor nits, otherwise looks good. I submitted try jobs for this patch. They will ...
2 years ago #5
Sam Kerner (Chrome)
Hmm, looks like the try results will not show up on the code review, but ...
2 years ago #6
Alexandre Abreu
http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/external_extension_util.cc File chrome/browser/extensions/external_extension_util.cc (right): http://codereview.chromium.org/9963120/diff/19003/chrome/browser/extensions/external_extension_util.cc#newcode18 chrome/browser/extensions/external_extension_util.cc:18: // Caller takes ownership of the returned dictionary. On ...
2 years ago #7
Alexandre Abreu
On 2012/04/13 22:36:10, Sam Kerner (Chrome) wrote: > Hmm, looks like the try results will ...
2 years ago #8
Sam Kerner (Chrome)
LGTM I will commit this change on your behalf. Docs need to be updated to ...
1 year, 12 months ago #9
Sam Kerner (Chrome)
When I apply this patch to trunk, I am getting several failures, because the code ...
1 year, 12 months ago #10
Alexandre Abreu
On 2012/04/18 15:59:33, Sam Kerner (Chrome) wrote: > When I apply this patch to trunk, ...
1 year, 12 months ago #11
Alexandre Abreu
On 2012/04/18 14:56:45, Sam Kerner (Chrome) wrote: > > Docs need to be updated to ...
1 year, 12 months ago #12
Sam Kerner (Chrome)
+mkearney to review the extension documentation changes.
1 year, 11 months ago #13
Finnur
I'm assuming this CL didn't change from what I reviewed when Sam posted it (http://codereview.chromium.org/10179002/). ...
1 year, 11 months ago #14
Alexandre Abreu
> But before I do I have what I think is an important question that ...
1 year, 11 months ago #15
Finnur
> I am not too sure about that. > Although, the two preference classes do ...
1 year, 11 months ago #16
Alexandre Abreu
Uploaded a new CL following the approach suggested by finnur
1 year, 11 months ago #17
Finnur
I like it! We are very close. The below is mostly just a bunch of ...
1 year, 11 months ago #18
Alexandre Abreu
http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/external_pref_extension_loader.cc File chrome/browser/extensions/external_pref_extension_loader.cc (right): http://codereview.chromium.org/9963120/diff/44003/chrome/browser/extensions/external_pref_extension_loader.cc#newcode58 chrome/browser/extensions/external_pref_extension_loader.cc:58: // Extracts/expect a file content in json format. On ...
1 year, 11 months ago #19
Finnur
Very good. Almost there... http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/external_pref_extension_loader.h File chrome/browser/extensions/external_pref_extension_loader.h (right): http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/external_pref_extension_loader.h#newcode55 chrome/browser/extensions/external_pref_extension_loader.h:55: // Extracts the information contained ...
1 year, 11 months ago #20
Alexandre Abreu
http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/external_pref_extension_loader.h File chrome/browser/extensions/external_pref_extension_loader.h (right): http://codereview.chromium.org/9963120/diff/57001/chrome/browser/extensions/external_pref_extension_loader.h#newcode55 chrome/browser/extensions/external_pref_extension_loader.h:55: // Extracts the information contained in a external_extension.json file ...
1 year, 11 months ago #21
Finnur
Only two left (from last list). http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html#newcode188 chrome/common/extensions/docs/static/external_extensions.html:188: <li> Search for ...
1 year, 11 months ago #22
Alexandre Abreu
http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html#newcode188 chrome/common/extensions/docs/static/external_extensions.html:188: <li> Search for the string <b>Can not read external ...
1 year, 11 months ago #23
Finnur
http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html#newcode241 chrome/common/extensions/docs/static/external_extensions.html:241: remove the metadata from the preferences file Not quite... ...
1 year, 11 months ago #24
Alexandre Abreu
http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html File chrome/common/extensions/docs/static/external_extensions.html (right): http://codereview.chromium.org/9963120/diff/57001/chrome/common/extensions/docs/static/external_extensions.html#newcode241 chrome/common/extensions/docs/static/external_extensions.html:241: remove the metadata from the preferences file On 2012/04/27 ...
1 year, 11 months ago #25
Finnur
LGTM. I'm running out of time for today, but I can try this change on ...
1 year, 11 months ago #26
Alexandre Abreu
On 2012/04/27 15:44:37, Finnur wrote: > LGTM. I'm running out of time for today, but ...
1 year, 11 months ago #27
Finnur
OK, I've tested this and it doesn't quite work. There were compile errors on Windows, ...
1 year, 11 months ago #28
Alexandre Abreu
1 year, 11 months ago #29
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,
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434