Not creating ClientUsageTracker for unsupported storage type.

Not creating ClientUsageTracker for unsupported storage type.

Extracted from https://chromiumcodereview.appspot.com/23240002/

Currently we create all usage trackers for each pair QuotaClient/StorageType.
Most clients only support one storage type and filesystem support variable
list of types (it doesn't have kStorageTypeSyncable in unittests).

If filesystem has no syncable storage backend it not useless to keep create ClientUsageTracker. This tracker fails when GetHostUsage is invoked. So let's don't create it.

BUG=281252
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221244

[michaeln, 2013-08-28 19:46:05]

+kinuko too

https://codereview.chromium.org/23545016/diff/1/webkit/browser/quota/usage_tr...
File webkit/browser/quota/usage_tracker.cc (right):

https://codereview.chromium.org/23545016/diff/1/webkit/browser/quota/usage_tr...
webkit/browser/quota/usage_tracker.cc:81: if ((*iter)->DoesSupport(type)) {
Thinning out the size of this map looks great.

I'm curious as to what goes wrong exactly when the set is not pruned in this
way? Is there another code change that could/should be made in addition to
thinning out the set?

[SeRya, 2013-08-29 09:07:25]

https://codereview.chromium.org/23545016/diff/1/webkit/browser/quota/usage_tr...
File webkit/browser/quota/usage_tracker.cc (right):

https://codereview.chromium.org/23545016/diff/1/webkit/browser/quota/usage_tr...
webkit/browser/quota/usage_tracker.cc:81: if ((*iter)->DoesSupport(type)) {
On 2013/08/28 19:46:05, michaeln wrote:
> Thinning out the size of this map looks great.
> 
> I'm curious as to what goes wrong exactly when the set is not pruned in this
> way? Is there another code change that could/should be made in addition to
> thinning out the set?

if you query
quota_manager->GetUsageTracker(kSyncableStorageType)->GetClientTracker(kFilesystem)->GetHostUsage(...)

it fails in method GetFileSystemBackend if syncable backend is not registered
(it happens in tests).

Stack is:
fileapi::FileSystemContext::GetFileSystemBackend()
fileapi::FileSystemContext::GetQuotaUtil()
fileapi::(anonymous namespace)::GetOriginsForHostOnFileThread()
base::internal::RunnableAdapter<>::Run()

It seems that all pieces of code using GetFileSystemBackend expect it may return
NULL so alternatively the following line could be removed from
GetFileSystemBackend:

NOTREACHED() << "Unknown filesystem type: " << type;

In addition to thinning out the set methods UsageTracker::UpdateUsageCache and
UsageTracker::SetUsageCacheEnabled could check result of GetClientTracker for
NULL. This methods calls are indirectly initiated by QuotaClient's what
understand that storage types they support so a tracker always exist for the
type they pass. This check looks unnecessary.

[kinuko, 2013-08-29 10:21:42]

looking good, thanks for fixing this up.

Do we have a bug number for this?  It'd be nice to track if there's one.

https://codereview.chromium.org/23545016/diff/1/webkit/browser/fileapi/file_s...
File webkit/browser/fileapi/file_system_quota_client.cc (right):

https://codereview.chromium.org/23545016/diff/1/webkit/browser/fileapi/file_s...
webkit/browser/fileapi/file_system_quota_client.cc:205: !=
file_system_types_.end();
It looks we should just rather check if a given type is supported sandbox
filesystem type or not, which can be checked by:
file_system_context_->IsSandboxFileSystem(type)

[SeRya, 2013-08-29 12:14:14]

https://codereview.chromium.org/23545016/diff/1/webkit/browser/fileapi/file_s...
File webkit/browser/fileapi/file_system_quota_client.cc (right):

https://codereview.chromium.org/23545016/diff/1/webkit/browser/fileapi/file_s...
webkit/browser/fileapi/file_system_quota_client.cc:205: !=
file_system_types_.end();
On 2013/08/29 10:21:42, kinuko wrote:
> It looks we should just rather check if a given type is supported sandbox
> filesystem type or not, which can be checked by:
> file_system_context_->IsSandboxFileSystem(type)

Good point. Only problem that IsSandboxFileSystem is implemented through
GetFileSystemBackend so it fails if <type> refers unregestered backend. I'm
fixing that.

[kinuko, 2013-09-02 14:04:59]

lgtm, sorry for the delay.

[commit-bot: I haz the power, 2013-09-02 14:44:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/23545016/2

[commit-bot: I haz the power, 2013-09-02 14:56:57]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[SeRya, 2013-09-02 15:19:05]

@michaeln, I still need owner's review for content/browser/indexed_db/* files.
Have a look, please.

[michaeln, 2013-09-03 21:28:37]

lgtm2 (sorry for the delay2)

[commit-bot: I haz the power, 2013-09-04 16:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/23545016/2

[commit-bot: I haz the power, 2013-09-04 20:10:48]

Change committed as 221244