Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(88)

Issue 7189029: Implement an initial extension settings API. (Closed)

Created:
9 years, 6 months ago by not at google - send to devlin
Modified:
9 years, 4 months ago
CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, PaweĊ‚ Hajdan Jr.
Visibility:
Public.

Description

Implement an initial extension settings API. Some general notes: - It's a lot of code for a single review, about 1.5K lines (plus tests, plus those generated docs, = 3.5K). Apologies. But it's close to the minimal amount of useful functionality. I've left some TODOs in the code to fix up soon. - No integration-style tests, but I'll start writing those now. Works from within a browser though. - Sync not hooked up yet, of course. - Ditto events. - Ditto thinking about incognito mode. - Ditto fun bugs like what happens if you set key "foo.bar" to "baz". - This is the first significant amount of C++ code I've ever written... so don't hold back. - The docs (i.e. my changes to extension_api.json) are a little incomplete, and I'm aware of that. A summary of the implementation: - There is an ExtensionSettings factory-type object which hands out ExtensionSettingsStorage areas for extensions. You may notice that I basically did the same thing that ExtensionPreferences does (so, lives with the profile, etc). - ExtensionSettingsStorage is a pure interface, with three implementations. - the main leveldb implementation. - a caching decorator, designed to sit on top of the leveldb implementation. - a "no-op" implementation which gives a trivial in-memory storage area when wrapped with a cache. This is used in the cases where the leveldb database fails to initialise (ExtensionSettings handles this). - and note that my plan is, when hooking up sync, that this will be implemented as another decorator. - ... the code is pretty well documented so not much else to say. For testing, turns out gtest is pretty snazzy. There are a bunch of test fixtrues in extension_settings_storage_unittest.h, so that 3 different configurations are tested: leveldb, leveldb + cache, and no-op + cache. BUG=47327 TEST=unit tests provided Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96159

Patch Set 1 #

Patch Set 2 : Add missing files #

Total comments: 26

Patch Set 3 : dgrogran comments #1 #

Patch Set 4 : Change where extension settings are saved, update TODO, api test #

Total comments: 45

Patch Set 5 : dgrogan comments #2, mihai comments #1 #

Total comments: 19

Patch Set 6 : Use base::Closure for storage callback, style fixes, mac/windows fixes #

Total comments: 26

Patch Set 7 : mpcomplete comments #1 #

Patch Set 8 : Remove core file #

Total comments: 19

Patch Set 9 : mpcomplete comments #2 #

Patch Set 10 : Sync to 95199 #

Patch Set 11 : Change to RefCountedThreadSafe #

Total comments: 3

Patch Set 12 : mpcomplete comments #3, TODO fix #

Patch Set 13 : Update extensions docs #

Patch Set 14 : Remove TODOs in preparation for submission #

Patch Set 15 : Make clang happy #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3677 lines, -1 line) Patch
M chrome/browser/extensions/extension_function_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_service.h View 1 2 3 4 5 6 7 8 9 5 chunks +11 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_service.cc View 1 2 3 4 5 6 7 8 9 3 chunks +7 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +98 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +162 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_api.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +78 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_api.cc View 1 2 3 4 5 6 7 8 1 chunk +107 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_apitest.cc View 1 2 3 4 1 chunk +17 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_cached_leveldb_storage_unittest.cc View 1 2 3 4 5 1 chunk +25 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_cached_noop_storage_unittest.cc View 1 2 3 4 5 1 chunk +25 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_leveldb_storage.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +59 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_leveldb_storage.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +505 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_leveldb_storage_unittest.cc View 1 2 3 4 5 1 chunk +25 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_noop_storage.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +34 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_noop_storage.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +72 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_storage.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +83 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_storage_cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +54 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_storage_cache.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +265 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_storage_unittest.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +65 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_settings_storage_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +289 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/extension_popup_controller_unittest.mm View 1 2 3 4 5 6 7 8 9 5 chunks +9 lines, -0 lines 0 comments Download
M chrome/chrome.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +12 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/common/extensions/api/extension_api.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +102 lines, -0 lines 0 comments Download
M chrome/common/extensions/docs/experimental.html View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
A chrome/common/extensions/docs/experimental.settings.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1322 lines, -0 lines 0 comments Download
M chrome/common/extensions/docs/samples.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/renderer/resources/renderer_extension_bindings.js View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/base/testing_profile.h View 1 2 3 4 5 6 7 8 9 3 chunks +5 lines, -1 line 0 comments Download
M chrome/test/base/testing_profile.cc View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/settings/manifest.json View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/settings/test.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +206 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
not at google - send to devlin
9 years, 6 months ago (2011-06-17 08:48:49 UTC) #1
dgrogan
I only looked at the leveldb stuff. It's pretty solid. http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings.cc#newcode107 ...
9 years, 6 months ago (2011-06-18 01:54:40 UTC) #2
not at google - send to devlin
Cheers. http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings.cc#newcode107 chrome/browser/extensions/extension_settings.cc:107: LOG(WARNING) << "Failed to create leveldb at " ...
9 years, 6 months ago (2011-06-20 05:33:11 UTC) #3
dgrogan
http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings.cc#newcode173 chrome/browser/extensions/extension_settings.cc:173: BrowserThread::FILE, On 2011/06/20 05:33:11, kalman1 wrote: > On 2011/06/18 ...
9 years, 6 months ago (2011-06-21 02:36:04 UTC) #4
Mihai Parparita -not on Chrome
For future reference, it seems like this patch could have been broken up a bit ...
9 years, 6 months ago (2011-06-21 23:40:11 UTC) #5
not at google - send to devlin
Thank Mihai, thanks again David. Btw, I have a list of style issues for myself ...
9 years, 6 months ago (2011-06-22 09:40:38 UTC) #6
not at google - send to devlin
http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/extension_settings.cc#newcode247 chrome/browser/extensions/extension_settings.cc:247: (new GetExistingStorageClosure(callback, storage))->Run(); > (and .: why not just ...
9 years, 6 months ago (2011-06-22 23:49:42 UTC) #7
Mihai Parparita -not on Chrome
http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings_leveldb_storage.cc File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right): http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings_leveldb_storage.cc#newcode200 chrome/browser/extensions/extension_settings_leveldb_storage.cc:200: FailOnFileThread("Failed"); On 2011/06/21 02:36:05, dgrogan wrote: > > like... ...
9 years, 6 months ago (2011-06-23 03:16:46 UTC) #8
not at google - send to devlin
http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/extension_settings.cc#newcode247 chrome/browser/extensions/extension_settings.cc:247: (new GetExistingStorageClosure(callback, storage))->Run(); Another self correction: the Closure classes ...
9 years, 6 months ago (2011-06-23 05:08:33 UTC) #9
not at google - send to devlin
http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings_leveldb_storage.cc File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right): http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/extension_settings_leveldb_storage.cc#newcode200 chrome/browser/extensions/extension_settings_leveldb_storage.cc:200: FailOnFileThread("Failed"); On 2011/06/23 03:16:46, Mihai Parparita wrote: > On ...
9 years, 6 months ago (2011-06-23 13:44:26 UTC) #10
Matt Perry
http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/extension_settings.cc#newcode165 chrome/browser/extensions/extension_settings.cc:165: &storage_objs_, What if ExtensionSettings is deleted before CreateStorageClosure::Done is ...
9 years, 6 months ago (2011-06-23 18:11:45 UTC) #11
Matt Perry
some more comments http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/extension_settings.cc#newcode56 chrome/browser/extensions/extension_settings.cc:56: &CreateStorageClosure::RunOnFileThread, base::Unretained(this))); nit: indent +2 http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/extension_settings_api.cc ...
9 years, 6 months ago (2011-06-23 19:14:10 UTC) #12
not at google - send to devlin
Thanks Matt. http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/extension_settings.cc#newcode165 chrome/browser/extensions/extension_settings.cc:165: &storage_objs_, On 2011/06/23 18:11:45, Matt Perry wrote: ...
9 years, 6 months ago (2011-06-27 08:51:02 UTC) #13
Matt Perry
http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/api/extension_api.json#newcode712 chrome/common/extensions/api/extension_api.json:712: "type": "any", On 2011/06/27 08:51:02, kalman1 wrote: > On ...
9 years, 5 months ago (2011-06-29 18:08:11 UTC) #14
not at google - send to devlin
I'm going to merge now, so the diff will presumably be quite different (it's been ...
9 years, 4 months ago (2011-08-03 06:36:51 UTC) #15
Matt Perry
http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/extension_settings.cc#newcode88 chrome/browser/extensions/extension_settings.cc:88: AddRef(); On 2011/08/03 06:36:51, kalman1 wrote: > On 2011/06/29 ...
9 years, 4 months ago (2011-08-03 19:33:52 UTC) #16
not at google - send to devlin
http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/extension_settings.cc File chrome/browser/extensions/extension_settings.cc (right): http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/extension_settings.cc#newcode88 chrome/browser/extensions/extension_settings.cc:88: AddRef(); On 2011/08/03 19:33:52, Matt Perry wrote: > On ...
9 years, 4 months ago (2011-08-03 22:06:55 UTC) #17
Matt Perry
LGTM, though I didn't review that closely post-merge. http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/api/extension_api.json#newcode780 chrome/common/extensions/api/extension_api.json:780: "description": ...
9 years, 4 months ago (2011-08-03 22:57:47 UTC) #18
not at google - send to devlin
merge was very clean. http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/api/extension_api.json#newcode780 chrome/common/extensions/api/extension_api.json:780: "description": "Callback with an empty ...
9 years, 4 months ago (2011-08-04 00:59:06 UTC) #19
not at google - send to devlin
Adding reviewers from OWNERS files for things outside extensions. Not sure if those without set ...
9 years, 4 months ago (2011-08-04 04:23:22 UTC) #20
Miranda Callahan
On 2011/08/04 04:23:22, kalman1 wrote: > Adding reviewers from OWNERS files for things outside extensions. ...
9 years, 4 months ago (2011-08-08 08:58:23 UTC) #21
not at google - send to devlin
I had to fiddle with a few destructors to make clang happy and compile. Fairly ...
9 years, 4 months ago (2011-08-10 07:42:42 UTC) #22
commit-bot: I haz the power
9 years, 4 months ago (2011-08-10 09:24:24 UTC) #23
Change committed as 96159

Powered by Google App Engine
This is Rietveld 408576698