Add GetUsageAndQuota() to Syncable API

Add extension API to get quota and usage of 'Syncable' storage for drive-backed File API

BUG=153501
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163234

[calvinlo, 2012-10-18 09:46:31]

I think this is ready for a first look.

[kinuko, 2012-10-18 13:00:57]

Looking good to me. Adding Mihai (mihaip@) for serious review in the extension
API part.

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:111: //
TODO(calvinlo): For now only gDrive cloud service is supported
nit: please end the comment with '.'.

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:149:
error_ = base::StringPrintf(kFileError, static_cast<int>(status));
Since the error code we get here is not a file error can we define/use a
different error message?  Ideally we want a method that converts from a quota
status code to a error message string but initially something like 'Quota error:
N' type message may work. (With a TODO comment + bug number to improve the error
message)

http://codereview.chromium.org/11146034/diff/4002/chrome/common/extensions/ap...
File chrome/common/extensions/api/sync_file_system.idl (right):

http://codereview.chromium.org/11146034/diff/4002/chrome/common/extensions/ap...
chrome/common/extensions/api/sync_file_system.idl:24: static void
getUsageAndQuota(DOMString serviceName,
Can you add a comment? I think we generate API documentation from this IDL file.

Also for now let's add [nodoc] comment for QuotaAndUsageCallback.

[Mihai Parparita -not on Chrome, 2012-10-18 20:10:56]

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:137: //
SendResponse must run on UI thread. Repost if coming from another thread.
When does this happen? Seems like you'd want to always know what your thread
you're on, instead of silently fixing things.

http://codereview.chromium.org/11146034/diff/4002/chrome/common/extensions/ap...
File chrome/common/extensions/api/sync_file_system.idl (right):

http://codereview.chromium.org/11146034/diff/4002/chrome/common/extensions/ap...
chrome/common/extensions/api/sync_file_system.idl:7: long usage;
Mentioning units (ideally in the field name) is a good idea.

[Mihai Parparita -not on Chrome, 2012-10-18 20:12:37]

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:156:
dict->SetInteger("usage", usage);
Instead of repeating the field names as strings, you can use the auto-generated
C++ class for GetUsageAndQuota::Results instead. It should be in
out/Debug/gen/chrome/common/extensions/api/sync_file_system.h, you can #include
as common/extensions/api/sync_file_system.h

[kinuko, 2012-10-19 04:10:49]

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:137: //
SendResponse must run on UI thread. Repost if coming from another thread.
On 2012/10/18 20:10:56, Mihai Parparita wrote:
> When does this happen? Seems like you'd want to always know what your thread
> you're on, instead of silently fixing things.

I think probably the comment is a bit misleading.
This callback is always to be called on IO thread first (since we pass this
callback to the quota manager on IO thread on line 122), but we want to forward
the result back to UI thread. Calvin, can we make the comment more explicit?

Also it's a bit off-topic but maybe we should stop using kControlThread but
should specifically use BrowserThread::UI here and elsewhere (and removing 37). 
Could you make the change too?

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:156:
dict->SetInteger("usage", usage);
On 2012/10/18 20:12:38, Mihai Parparita wrote:
> Instead of repeating the field names as strings, you can use the
auto-generated
> C++ class for GetUsageAndQuota::Results instead. It should be in
> out/Debug/gen/chrome/common/extensions/api/sync_file_system.h, you can
#include
> as common/extensions/api/sync_file_system.h

Nice! I expected we should have something like that.

[calvinlo, 2012-10-19 06:59:42]

Think I covered everything mentioned by both Kinuko and Mihai in this latest
patch.

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:111: //
TODO(calvinlo): For now only gDrive cloud service is supported
On 2012/10/18 13:00:59, kinuko wrote:
> nit: please end the comment with '.'.

Done.

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:137: //
SendResponse must run on UI thread. Repost if coming from another thread.
On 2012/10/19 04:10:49, kinuko wrote:
> On 2012/10/18 20:10:56, Mihai Parparita wrote:
> > When does this happen? Seems like you'd want to always know what your thread
> > you're on, instead of silently fixing things.
> 
> I think probably the comment is a bit misleading.
> This callback is always to be called on IO thread first (since we pass this
> callback to the quota manager on IO thread on line 122), but we want to
forward
> the result back to UI thread. Calvin, can we make the comment more explicit?
> 
> Also it's a bit off-topic but maybe we should stop using kControlThread but
> should specifically use BrowserThread::UI here and elsewhere (and removing
37). 
> Could you make the change too?
Updated comment, please double check. Also removed kControlThread and reference
BrowserThread::UI instead.

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:149:
error_ = base::StringPrintf(kFileError, static_cast<int>(status));
On 2012/10/18 13:00:59, kinuko wrote:
> Since the error code we get here is not a file error can we define/use a
> different error message?  Ideally we want a method that converts from a quota
> status code to a error message string but initially something like 'Quota
error:
> N' type message may work. (With a TODO comment + bug number to improve the
error
> message)

Added const char kQuotaError[] = "Quota error %d."; on line 36. Also added TODO
and bug entry 156791

http://codereview.chromium.org/11146034/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:156:
dict->SetInteger("usage", usage);
On 2012/10/19 04:10:49, kinuko wrote:
> On 2012/10/18 20:12:38, Mihai Parparita wrote:
> > Instead of repeating the field names as strings, you can use the
> auto-generated
> > C++ class for GetUsageAndQuota::Results instead. It should be in
> > out/Debug/gen/chrome/common/extensions/api/sync_file_system.h, you can
> #include
> > as common/extensions/api/sync_file_system.h
> 
> Nice! I expected we should have something like that.

I think I got this right now.

http://codereview.chromium.org/11146034/diff/4002/chrome/common/extensions/ap...
File chrome/common/extensions/api/sync_file_system.idl (right):

http://codereview.chromium.org/11146034/diff/4002/chrome/common/extensions/ap...
chrome/common/extensions/api/sync_file_system.idl:7: long usage;
On 2012/10/18 20:10:56, Mihai Parparita wrote:
> Mentioning units (ideally in the field name) is a good idea.

Good ideas. Done.

http://codereview.chromium.org/11146034/diff/4002/chrome/common/extensions/ap...
chrome/common/extensions/api/sync_file_system.idl:24: static void
getUsageAndQuota(DOMString serviceName,
On 2012/10/18 13:00:59, kinuko wrote:
> Can you add a comment? I think we generate API documentation from this IDL
file.
> 
> Also for now let's add [nodoc] comment for QuotaAndUsageCallback.

Done.

[kinuko, 2012-10-19 10:31:01]

lgtm (if it looks good to extension owners)

[Mihai Parparita -not on Chrome, 2012-10-19 22:20:55]

LGTM

http://codereview.chromium.org/11146034/diff/10002/chrome/browser/extensions/...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

http://codereview.chromium.org/11146034/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:139:
BrowserThread::PostTask(
Add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) here

http://codereview.chromium.org/11146034/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:146: 
And a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) here.

[calvinlo, 2012-10-22 03:53:29]

Added the DCHECKS. Running CQ now.

http://codereview.chromium.org/11146034/diff/10002/chrome/browser/extensions/...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

http://codereview.chromium.org/11146034/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:139:
BrowserThread::PostTask(
On 2012/10/19 22:20:55, Mihai Parparita wrote:
> Add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) here

Done.

http://codereview.chromium.org/11146034/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:146: 
On 2012/10/19 22:20:55, Mihai Parparita wrote:
> And a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) here.

Done.

[commit-bot: I haz the power, 2012-10-22 03:54:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/11146034/3006

[commit-bot: I haz the power, 2012-10-22 06:40:01]

Change committed as 163234