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

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, 12 months ago by Alexandre Abreu
Modified:
1 year, 2 months ago
CC:
chromium-reviews, 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) Patch
M AUTHORS View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -1 line 0 comments 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 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 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 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 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 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 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 Download
Trybot results:
Commit: CQ not working?

Messages

Total messages: 29 (0 generated)
Sam Kerner (Chrome)
Alexandre, if you have not yet read our wiki page on contributing code, please take ...
2 years, 11 months ago (2012-04-13 01:35:27 UTC) #1
Alexandre Abreu
On 2012/04/13 01:35:27, Sam Kerner (Chrome) wrote: > Alexandre, if you have not yet read ...
2 years, 11 months ago (2012-04-13 18:26:59 UTC) #2
Sam Kerner (Chrome)
A few more comments. You may notice that we are a bit rigid on style. ...
2 years, 11 months ago (2012-04-13 19:35:13 UTC) #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, 11 months ago (2012-04-13 21:52:38 UTC) #4
Sam Kerner (Chrome)
Two minor nits, otherwise looks good. I submitted try jobs for this patch. They will ...
2 years, 11 months ago (2012-04-13 22:11:02 UTC) #5
Sam Kerner (Chrome)
Hmm, looks like the try results will not show up on the code review, but ...
2 years, 11 months ago (2012-04-13 22:36:10 UTC) #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, 11 months ago (2012-04-13 23:24:46 UTC) #7
Alexandre Abreu
On 2012/04/13 22:36:10, Sam Kerner (Chrome) wrote: > Hmm, looks like the try results will ...
2 years, 11 months ago (2012-04-13 23:45:16 UTC) #8
Sam Kerner (Chrome)
LGTM I will commit this change on your behalf. Docs need to be updated to ...
2 years, 11 months ago (2012-04-18 14:56:45 UTC) #9
Sam Kerner (Chrome)
When I apply this patch to trunk, I am getting several failures, because the code ...
2 years, 11 months ago (2012-04-18 15:59:33 UTC) #10
Alexandre Abreu
On 2012/04/18 15:59:33, Sam Kerner (Chrome) wrote: > When I apply this patch to trunk, ...
2 years, 11 months ago (2012-04-20 16:15:59 UTC) #11
Alexandre Abreu
On 2012/04/18 14:56:45, Sam Kerner (Chrome) wrote: > > Docs need to be updated to ...
2 years, 11 months ago (2012-04-20 18:46:11 UTC) #12
Sam Kerner (Chrome)
+mkearney to review the extension documentation changes.
2 years, 11 months ago (2012-04-23 17:05:08 UTC) #13
Finnur
I'm assuming this CL didn't change from what I reviewed when Sam posted it (http://codereview.chromium.org/10179002/). ...
2 years, 11 months ago (2012-04-25 13:33:45 UTC) #14
Alexandre Abreu
> But before I do I have what I think is an important question that ...
2 years, 11 months ago (2012-04-25 15:31:05 UTC) #15
Finnur
> I am not too sure about that. > Although, the two preference classes do ...
2 years, 11 months ago (2012-04-25 16:11:27 UTC) #16
Alexandre Abreu
Uploaded a new CL following the approach suggested by finnur
2 years, 11 months ago (2012-04-26 16:30:25 UTC) #17
Finnur
I like it! We are very close. The below is mostly just a bunch of ...
2 years, 11 months ago (2012-04-27 10:50:27 UTC) #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 ...
2 years, 11 months ago (2012-04-27 14:05:40 UTC) #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 ...
2 years, 11 months ago (2012-04-27 14:29:43 UTC) #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 ...
2 years, 11 months ago (2012-04-27 14:46:52 UTC) #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 ...
2 years, 11 months ago (2012-04-27 14:56:09 UTC) #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 ...
2 years, 11 months ago (2012-04-27 15:03:13 UTC) #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... ...
2 years, 11 months ago (2012-04-27 15:34:16 UTC) #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 ...
2 years, 11 months ago (2012-04-27 15:40:38 UTC) #25
Finnur
LGTM. I'm running out of time for today, but I can try this change on ...
2 years, 11 months ago (2012-04-27 15:44:37 UTC) #26
Alexandre Abreu
On 2012/04/27 15:44:37, Finnur wrote: > LGTM. I'm running out of time for today, but ...
2 years, 11 months ago (2012-04-27 15:46:09 UTC) #27
Finnur
OK, I've tested this and it doesn't quite work. There were compile errors on Windows, ...
2 years, 11 months ago (2012-04-30 13:29:49 UTC) #28
Alexandre Abreu
2 years, 11 months ago (2012-04-30 14:18:46 UTC) #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 cf4c24d