Reset the default quota limit for extensions to 5MB, but add a...

Reset the default quota limit for extensions to 5MB, but add a
unlimited_quota permission that extensions can use to request
unlimited quota.

TEST=ExtensionsServiceTest.StorageQuota
BUG=27688
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40190

[Aaron Boodman, 2010-02-25 21:29:33]

Have not reviewed unit test yet..

http://codereview.chromium.org/660113/diff/25/30
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/660113/diff/25/30#newcode472
chrome/browser/extensions/extensions_service.cc:472:
UTF8ToUTF16(std::string(chrome::kExtensionScheme) + "_" +
Little nit: could the code that makes these identifiers in the first place
expose this as a helper?

http://codereview.chromium.org/660113/diff/25/33
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/660113/diff/25/33#newcode121
chrome/common/extensions/extension.cc:121: const char*
Extension::kUnlimitedQuotaPermission = "unlimited_quota";
api naming nit: I prefer "unlimited_storage".

http://codereview.chromium.org/660113/diff/25/26
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/660113/diff/25/26#newcode198
webkit/database/database_tracker.cc:198: if (!LazyInit())
Is it possible to factor this so the lazy init is not necessary? It seems easy
to forget.

[Erik does not do reviews, 2010-02-25 21:46:29]

I'll defer to Michael and Aaron.  Thanks for taking doing this work.

Erik


On Thu, Feb 25, 2010 at 1:22 PM, <dumi@chromium.org> wrote:

> Reviewers: michaeln, Aaron Boodman, Erik Kay,
>
> Description:
> Reset the default quota limit for extensions to 5MB, but add a
> unlimited_quota permission that extensions can use to request
> unlimited quota.
>
> TEST=ExtensionsServiceTest.StorageQuota
> BUG=27688
>
>
>
> Please review this at http://codereview.chromium.org/660113
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/extensions/extensions_service.cc
>  M     chrome/browser/extensions/extensions_service_unittest.cc
>  M     chrome/common/extensions/extension.h
>  M     chrome/common/extensions/extension.cc
>  A
> chrome/test/data/extensions/storage_quota/limited_quota/1.0/manifest.json
>  A
> chrome/test/data/extensions/storage_quota/unlimited_quota/1.0/manifest.json
>  M     webkit/database/database_tracker.h
>  M     webkit/database/database_tracker.cc
>
>
>

[michaeln, 2010-02-25 22:04:06]

http://codereview.chromium.org/660113/diff/25/30
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/660113/diff/25/30#newcode479
chrome/browser/extensions/extensions_service.cc:479: kint64max));
This works, but it really doesn't feel right to persist this data in a database
system specific quota table or to re-write the row each time the browser
launches for each extension with this priv.

Multiple write xactions at startup go against starting up quickly. I'd like to
see this CL massaged so we aren't doing that. A simple way of doing that would
be to add a SetOriginQuotaOverride method that keeps this data in memory only,
and use that method here.

Eventually, it may be good to refactor things to pull quota management (and
rights management) out of tracking (and get rid of the quota table). I wonder if
we can do that sooner rather than later?

[dumi, 2010-02-25 22:48:26]

http://codereview.chromium.org/660113/diff/25/30
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/660113/diff/25/30#newcode472
chrome/browser/extensions/extensions_service.cc:472:
UTF8ToUTF16(std::string(chrome::kExtensionScheme) + "_" +
On 2010/02/25 21:29:33, Aaron Boodman wrote:
> Little nit: could the code that makes these identifiers in the first place
> expose this as a helper?

done. added a method to DatabaseUtil to do this.

http://codereview.chromium.org/660113/diff/25/30#newcode479
chrome/browser/extensions/extensions_service.cc:479: kint64max));
On 2010/02/25 22:04:06, michaeln wrote:
> This works, but it really doesn't feel right to persist this data in a
database
> system specific quota table or to re-write the row each time the browser
> launches for each extension with this priv.
> 
> Multiple write xactions at startup go against starting up quickly. I'd like to
> see this CL massaged so we aren't doing that. A simple way of doing that would
> be to add a SetOriginQuotaOverride method that keeps this data in memory only,
> and use that method here.
> 
> Eventually, it may be good to refactor things to pull quota management (and
> rights management) out of tracking (and get rid of the quota table). I wonder
if
> we can do that sooner rather than later?

I don't think this slows down the startup, because this is done on the FILE
thread. I don't know if it's worth adding a in-memory storage mechanism --
database_tracker already caches all kind of information and updates/reloads it
as necessary. I think the right solution might be to add a row to the quota
table when the extension is _installed_, not loaded; and delete it when the
extension is uninstalled.

And yes, we should probably unify quota management for DBs, local storage, and
appcache and have a separate sub-system that deals with that, but I think that's
way beyond the scope of this CL.

http://codereview.chromium.org/660113/diff/25/33
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/660113/diff/25/33#newcode121
chrome/common/extensions/extension.cc:121: const char*
Extension::kUnlimitedQuotaPermission = "unlimited_quota";
On 2010/02/25 21:29:33, Aaron Boodman wrote:
> api naming nit: I prefer "unlimited_storage".

done.

http://codereview.chromium.org/660113/diff/25/26
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/660113/diff/25/26#newcode198
webkit/database/database_tracker.cc:198: if (!LazyInit())
On 2010/02/25 21:29:33, Aaron Boodman wrote:
> Is it possible to factor this so the lazy init is not necessary? It seems easy
> to forget.

let me talk to michael a bit about this. in any case, we should probably do this
in a separate CL, as it would involve more change to database_tracker.

[michaeln, 2010-02-26 00:24:34]

http://codereview.chromium.org/660113/diff/25/30
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/660113/diff/25/30#newcode479
chrome/browser/extensions/extensions_service.cc:479: kint64max));
> I don't think this slows down the startup, because this is done on the FILE
> thread.

When i start chrome on my non-super-computer at home, it takes a while to read
from disk the data needed to draw the new tab page and to let the magic location
bar mind reading logic work. Adding new file io is going to further slow that
stuff down.

And the file io we're talking about adding is silly, rewriting the same data on
each restart, its just not needed and it hurts startup perf.

I don't think we should do that.

[michaeln, 2010-02-26 01:23:06]

http://codereview.chromium.org/660113/diff/1039/1044
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/660113/diff/1039/1044#newcode474
chrome/browser/extensions/extensions_service.cc:474: chrome::kExtensionScheme,
extension->id(), 0);
Should we use extension.url() + WebSecurityOrigin here to compute this
origin_identifier?

Maybe the 'util' method could be...
  string16 DatabaseUtil::GetOriginIdentifier(GURL& url);
... and have that implemented in terms of WebSecurityOrigin.

[dumi, 2010-02-26 02:10:46]

http://codereview.chromium.org/660113/diff/1039/1044
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/660113/diff/1039/1044#newcode474
chrome/browser/extensions/extensions_service.cc:474: chrome::kExtensionScheme,
extension->id(), 0);
On 2010/02/26 01:23:07, michaeln wrote:
> Should we use extension.url() + WebSecurityOrigin here to compute this
> origin_identifier?
> 
> Maybe the 'util' method could be...
>   string16 DatabaseUtil::GetOriginIdentifier(GURL& url);
> ... and have that implemented in terms of WebSecurityOrigin.

done.

[michaeln, 2010-02-26 02:36:47]

this works for me, i haven't looked at the test cases

http://codereview.chromium.org/660113/diff/79/80
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/660113/diff/79/80#newcode376
webkit/database/database_tracker.cc:376: in_memory_quotas_.end())
maybe look here first?

[dumi, 2010-02-26 02:58:31]

http://codereview.chromium.org/660113/diff/79/80
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/660113/diff/79/80#newcode376
webkit/database/database_tracker.cc:376: in_memory_quotas_.end())
On 2010/02/26 02:36:47, michaeln wrote:
> maybe look here first?

done.

[Paweł Hajdan Jr., 2010-02-26 09:51:37]

Drive-by with minor test comments.

http://codereview.chromium.org/660113/diff/94/100
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/660113/diff/94/100#newcode1644
chrome/browser/extensions/extensions_service_unittest.cc:1644: static void
GetQuotas(webkit_database::DatabaseTracker* db_tracker,
nit: Please add a comment what this function does, also the name could include
"Database" to make it more clear.

http://codereview.chromium.org/660113/diff/94/100#newcode1671
chrome/browser/extensions/extensions_service_unittest.cc:1671:
EXPECT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
nit: This should be ASSERT_TRUE, the following code is going to fail if this
fails.

[michaeln, 2010-02-26 20:04:25]

http://codereview.chromium.org/660113/diff/94/100
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/660113/diff/94/100#newcode1690
chrome/browser/extensions/extensions_service_unittest.cc:1690:
ChromeThread::FILE, FROM_HERE,
In these unittests, the FILE thread and the UI thread are one and the same (see
ExtensionsServiceTestBase() ctor). So i think you can just query/grope the
dbtracker here directly w/o task posting stuff to verify that the
ExtensionService has tweeked the quota for the unlimited origin.

[dumi, 2010-02-26 20:58:10]

http://codereview.chromium.org/660113/diff/94/100
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/660113/diff/94/100#newcode1644
chrome/browser/extensions/extensions_service_unittest.cc:1644: static void
GetQuotas(webkit_database::DatabaseTracker* db_tracker,
On 2010/02/26 09:51:37, Paweł Hajdan Jr. wrote:
> nit: Please add a comment what this function does, also the name could include
> "Database" to make it more clear.

moved this code inside the test method and removed this function.

http://codereview.chromium.org/660113/diff/94/100#newcode1671
chrome/browser/extensions/extensions_service_unittest.cc:1671:
EXPECT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
On 2010/02/26 09:51:37, Paweł Hajdan Jr. wrote:
> nit: This should be ASSERT_TRUE, the following code is going to fail if this
> fails.

done.

http://codereview.chromium.org/660113/diff/94/100#newcode1690
chrome/browser/extensions/extensions_service_unittest.cc:1690:
ChromeThread::FILE, FROM_HERE,
On 2010/02/26 20:04:25, michaeln wrote:
> In these unittests, the FILE thread and the UI thread are one and the same
(see
> ExtensionsServiceTestBase() ctor). So i think you can just query/grope the
> dbtracker here directly w/o task posting stuff to verify that the
> ExtensionService has tweeked the quota for the unlimited origin. 

done.

[michaeln, 2010-02-26 22:38:29]

LGTM