Add 1st cut of FileSystemUsageTracker that tracks the usage changes in FileSystem API.

Add 1st cut of FileSystemUsageTracker that tracks the usage changes in FileSystem API.

For now it has no meaningful implementation yet; mostly just for
defining a few interfaces.

BUG=
TEST=FileSystemUsageTrackerTest.DummyTest

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

[kinuko, 2011-02-08 04:40:06]

On 2011/02/08 03:59:03, kinuko wrote:

There a few more changes I'm planning to make, but sending out for an early
review (just so as to make each change smaller).

[Paweł Hajdan Jr., 2011-02-08 11:25:49]

Drive-by with a tiny nit. No need to wait for me if you just convert to stack
allocation (for simplicity).

http://codereview.chromium.org/6426001/diff/8001/webkit/fileapi/file_system_u...
File webkit/fileapi/file_system_usage_tracker_unittest.cc (right):

http://codereview.chromium.org/6426001/diff/8001/webkit/fileapi/file_system_u...
webkit/fileapi/file_system_usage_tracker_unittest.cc:53:
scoped_ptr<ScopedTempDir> data_dir_;
nit: Why not stack-allocated?

[kinuko, 2011-02-08 13:36:12]

Changed the ScopedTempDir stack-allocated.  Thanks as always,

http://codereview.chromium.org/6426001/diff/8001/webkit/fileapi/file_system_u...
File webkit/fileapi/file_system_usage_tracker_unittest.cc (right):

http://codereview.chromium.org/6426001/diff/8001/webkit/fileapi/file_system_u...
webkit/fileapi/file_system_usage_tracker_unittest.cc:53:
scoped_ptr<ScopedTempDir> data_dir_;
On 2011/02/08 11:25:50, Paweł Hajdan Jr. wrote:
> nit: Why not stack-allocated?

Done.

[michaeln, 2011-02-09 00:25:36]

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.cc (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:38: if (tracker_)
can tracker_ ever be null?

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:44: void RunOnFileThread() {
can this method be private?

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:47: // TODO(dmikurube): add the code
that retrieves the origin usage here.
This could tie up the file thread for quite some time. Is it ok to monopolize it
for the duration?

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:69:
scoped_refptr<base::MessageLoopProxy> origin_message_loop_;
the term 'origin' in this data member could be confusing since its a different
meaning than the other use of 'origin' in this class... consider a different
name?

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:97: scoped_ptr<GetUsageCallback>
callback(callback_ptr);
I'm fuzzy on what is common practice on chrome with regard to who is responsible
for deleting 'callback' instances. I see in FileUtilProxy, they're deleting by
the Proxy class after it has invoked them which matches the semantics you have
here.

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.h (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:44: friend class GetUsageTask;
since it's an inner class, do you need the 'friend' keyword here?

[ericu, 2011-02-09 02:05:19]

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.cc (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:69:
scoped_refptr<base::MessageLoopProxy> origin_message_loop_;
On 2011/02/09 00:25:36, michaeln wrote:
> the term 'origin' in this data member could be confusing since its a different
> meaning than the other use of 'origin' in this class... consider a different
> name?

I think just changing it to "original_message_loop_" would be enough.  Or
caller_message_loop_ or something like that would work too.

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.h (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:27: class FileSystemUsageTracker {
What's the lifetime of this object?  Is there guaranteed to be exactly one per
profile?  Does it need to be deleted on any particular thread?  What about
requests in progress at destruction time?

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:47: void
UnregisterUsageTask(GetUsageTask* task);
I'd probably call these [Un]RegisterGetUsageTask, just for consistency.

[kinuko, 2011-02-09 05:22:36]

Thanks for your comments! Updated the patch.

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.cc (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:38: if (tracker_)
On 2011/02/09 00:25:36, michaeln wrote:
> can tracker_ ever be null?

Just a paranoid check... changed this to DCHECK.

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:44: void RunOnFileThread() {
On 2011/02/09 00:25:36, michaeln wrote:
> can this method be private?

I'm planning to add a base class and make make this method protected, but yes
for now.  Done.

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:47: // TODO(dmikurube): add the code
that retrieves the origin usage here.
On 2011/02/09 00:25:36, michaeln wrote:
> This could tie up the file thread for quite some time. Is it ok to monopolize
it
> for the duration?

In our current plan, the monopolization should only happen for the 1st time this
code runs (i.e. 1st run after the installation) and after crashes.  I'm also
thinking to call this in a less visible way (eagerly or lazily depending on how
the API is called), but for now it's just a single simple task and no one calls
this yet.

(The half-baked design doc:
https://docs.google.com/a/google.com/document/d/1EIFQDvGFo4pswyd5-v-o2tDNlhvG...)

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:69:
scoped_refptr<base::MessageLoopProxy> origin_message_loop_;
On 2011/02/09 02:05:22, ericu wrote:
> On 2011/02/09 00:25:36, michaeln wrote:
> > the term 'origin' in this data member could be confusing since its a
different
> > meaning than the other use of 'origin' in this class... consider a different
> > name?
> 
> I think just changing it to "original_message_loop_" would be enough.  Or
> caller_message_loop_ or something like that would work too.

Indeed.  Done.

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:97: scoped_ptr<GetUsageCallback>
callback(callback_ptr);
On 2011/02/09 00:25:36, michaeln wrote:
> I'm fuzzy on what is common practice on chrome with regard to who is
responsible
> for deleting 'callback' instances. I see in FileUtilProxy, they're deleting by
> the Proxy class after it has invoked them which matches the semantics you have
> here.

In this implementation I chose to make the tracker class takes the ownership of
callbacks.  It doesn't pass the callbacks to the tasks either.  Let me hear if
you have concerns on this design, thanks!

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.h (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:27: class FileSystemUsageTracker {
On 2011/02/09 02:05:22, ericu wrote:
> What's the lifetime of this object?  Is there guaranteed to be exactly one per
> profile?  Does it need to be deleted on any particular thread?  What about
> requests in progress at destruction time?

Added comments, as I interpreted your questions as so :)
For now it's simply owned by SandboxedFileSystemContext.  I may change this
design a bit if I find it doesn't work.  Any advise is welcome.
(I'll change the comment accordingly if I do so)

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:44: friend class GetUsageTask;
On 2011/02/09 00:25:36, michaeln wrote:
> since it's an inner class, do you need the 'friend' keyword here?

Well... I'm not sure if it works in all the compilers we support but some of the
existing classes do so so I figured that it should be ok.
http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#45

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:47: void
UnregisterUsageTask(GetUsageTask* task);
On 2011/02/09 02:05:22, ericu wrote:
> I'd probably call these [Un]RegisterGetUsageTask, just for consistency.

Agreed, except that the method names reflect the future plan - I'm planning to
have a base class called UsageTask. So I'd like to make them as is for now.

[kinuko, 2011-02-09 05:25:59]

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.cc (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.cc:44: void RunOnFileThread() {
On 2011/02/09 05:22:36, kinuko wrote:
> On 2011/02/09 00:25:36, michaeln wrote:
> > can this method be private?
> 
> I'm planning to add a base class and make make this method protected, but yes
> for now.  Done.

Oops, please ignore my previous comment.  Just made this private.

[ericu, 2011-02-09 23:03:37]

LGTM

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.h (right):

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:27: class FileSystemUsageTracker {
On 2011/02/09 05:22:36, kinuko wrote:
> On 2011/02/09 02:05:22, ericu wrote:
> > What's the lifetime of this object?  Is there guaranteed to be exactly one
per
> > profile?  Does it need to be deleted on any particular thread?  What about
> > requests in progress at destruction time?
> 
> Added comments, as I interpreted your questions as so :)
> For now it's simply owned by SandboxedFileSystemContext.  I may change this
> design a bit if I find it doesn't work.  Any advise is welcome.
> (I'll change the comment accordingly if I do so)

Great--thanks.

http://codereview.chromium.org/6426001/diff/12012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:47: void
UnregisterUsageTask(GetUsageTask* task);
On 2011/02/09 05:22:36, kinuko wrote:
> On 2011/02/09 02:05:22, ericu wrote:
> > I'd probably call these [Un]RegisterGetUsageTask, just for consistency.
> 
> Agreed, except that the method names reflect the future plan - I'm planning to
> have a base class called UsageTask. So I'd like to make them as is for now.

OK.