Change webkit/{fileapi,quota} code to use TaskRunner.

Change webkit/{fileapi,quota} code to use TaskRunner.

BUG=123559
TEST=existing tests

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

[kinuko, 2012-04-26 14:19:45]

Mostly mechanical changes. PTL thanks!

[michaeln, 2012-04-27 01:04:02]

It is mostly mechanical but some subtleties too.

Please think about the type of runner that should be provided to abstract FSO
and FSR classes. I suspect order of tasks could be important to particular impls
in which case i think SequencedTaskRunner is really what we should be using for
those guys.

Also, last i looked, PostTaskAndReply only worked if the current thread has a
message loop on it, so be careful about invoking that on SWP threads which dont
run a message loop. But, I hadn't seen the 'SingleThread' interface variant
until this CL, so i'm not sure what else has changed since last i looked.

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/chromeos/fil...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/chromeos/fil...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:270:
base::MessageLoopProxy* file_proxy,
should this variant be removed now, i don't see it in the .h file?

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
File webkit/fileapi/file_system_context.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_system_context.cc:148: if
(file_task_runner_->BelongsToCurrentThread() &&
Why the change from !IsOnIOThread to IsOnFileThread?

FileSystemContext is a refcounted object which makes it difficult to truely have
a handle on where exactly the last ref will be released. As coded i think this
class is assuming that it's either the IO thread or the FileThread and there and
anything else is an impossibility...

...but is that really a safe assumption given that things outside of the fileapi
may be hanging on to refs (for example gdrive stuff).

What goes wrong if the the test original test is retained?

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
File webkit/fileapi/file_system_context.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_system_context.h:57: base::SingleThreadTaskRunner*
file_task_runner,
If the intent is to eventually put these tasks on the sequenced worker pool,
does the file_task_runner want to be of type SequencedTaskRunner? By using
SingltThreadTaskRunner, I think this constrains us to
MessageLoop/MessageLoopProxy only based impls with no option to use SWP.

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_system_context.h:123: base::TaskRunner* file_task_runner);
Since an operation may involve multiple steps, would it make sense to provide a
SequencedTaskRunner here so the 'op' gets a guarantee of the order of those
steps.

Also SequencedWorkerPoolSequencedTaskRunner's RunsTasksOnCurrentThread method
provides some nice utility similar to IsRunningOnThread semantics that generic
TaskRunner wont necessaritly provide.

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_system_context.h:134: base::TaskRunner* file_task_runner);
ditto

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
File webkit/fileapi/file_system_mount_point_provider.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_system_mount_point_provider.h:95: base::TaskRunner*
task_runner,
SequencedTaskRunner?

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_system_mount_point_provider.h:106: base::TaskRunner*
task_runner,
SequencedTaskRunner?

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
File webkit/fileapi/file_system_quota_client.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_system_quota_client.h:37: base::TaskRunner*
file_task_runner,
i think since the QuotaManager wants operations to complete in order, this class
does need sequencing guarantees.

base::SequencedTaskRunner*

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
File webkit/fileapi/file_writer_delegate.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_writer_delegate.cc:74: scoped_refptr<base::TaskRunner>
origin_task_runner_;
My vote would be to keep this as a MLP for clarity or maybe switch it to
SingleThreadTaskRunner.

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/fileapi/file...
webkit/fileapi/file_writer_delegate.cc:205:
base::MessageLoopProxy::current()->PostTask(
nit: change not really needed (afaict)

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/quota/quota_...
File webkit/quota/quota_manager.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/quota/quota_...
webkit/quota/quota_manager.h:110: base::SequencedTaskRunner* db_thread,
Yup... SingleThread for the first, Sequenced for the second looks spot on here.

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/quota/quota_...
File webkit/quota/quota_task.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/4002/webkit/quota/quota_...
webkit/quota/quota_task.cc:50: original_task_runner_->DeleteSoon(FROM_HERE,
this);
Is it ok to make this change, previously the delete would happen on the current
thread, now not necessarily so? Are all usages of this protected method ok with
that change?

[kinuko, 2012-04-27 10:10:29]

Updated, thanks.

In general I replaced too generic TRs with Sequenced one, changed some of
SingleThreadTR also with SequencedTR (this was wrongly made because I didn't
know the existence of RunsTasksOnCurrentThread).

PostTaskAndReply usage looks ok to me as we almost always runs it on IO thread
(in chrome case).

http://codereview.chromium.org/10197007/diff/4002/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10197007/diff/4002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:270:
base::MessageLoopProxy* file_proxy,
On 2012/04/27 01:04:03, michaeln wrote:
> should this variant be removed now, i don't see it in the .h file?

Due to bad merge... this change contained some rough stuff.  Sorry for abusing
your eyes for detecting them :(

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:148: if
(file_task_runner_->BelongsToCurrentThread() &&
On 2012/04/27 01:04:03, michaeln wrote:
> Why the change from !IsOnIOThread to IsOnFileThread?
> 
> FileSystemContext is a refcounted object which makes it difficult to truely
have
> a handle on where exactly the last ref will be released. As coded i think this
> class is assuming that it's either the IO thread or the FileThread and there
and
> anything else is an impossibility...
> 
> ...but is that really a safe assumption given that things outside of the
fileapi
> may be hanging on to refs (for example gdrive stuff).
> 
> What goes wrong if the the test original test is retained?

Sorry this change was not intended. Fixed.

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:57: base::SingleThreadTaskRunner*
file_task_runner,
On 2012/04/27 01:04:03, michaeln wrote:
> If the intent is to eventually put these tasks on the sequenced worker pool,
> does the file_task_runner want to be of type SequencedTaskRunner? By using
> SingltThreadTaskRunner, I think this constrains us to
> MessageLoop/MessageLoopProxy only based impls with no option to use SWP.

I wanted to do so but couldn't find the nice RunsTasksOnCurrentThread so ended
up using SingleThreadTaskRunner (and it wasn't for regular file operations, for
which we could separately specify SWP).

Fixed.

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:123: base::TaskRunner* file_task_runner);
On 2012/04/27 01:04:03, michaeln wrote:
> Since an operation may involve multiple steps, would it make sense to provide
a
> SequencedTaskRunner here so the 'op' gets a guarantee of the order of those
> steps.
> 
> Also SequencedWorkerPoolSequencedTaskRunner's RunsTasksOnCurrentThread method
> provides some nice utility similar to IsRunningOnThread semantics that generic
> TaskRunner wont necessaritly provide.

Done.  (Removed this parameter in favor of using the runner given at the
constructor time, assuming it must be ok)

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:134: base::TaskRunner* file_task_runner);
On 2012/04/27 01:04:03, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:95: base::TaskRunner*
task_runner,
On 2012/04/27 01:04:03, michaeln wrote:
> SequencedTaskRunner?

Done.

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:106: base::TaskRunner*
task_runner,
On 2012/04/27 01:04:03, michaeln wrote:
> SequencedTaskRunner?

Done.

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_quota_client.h (right):

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_quota_client.h:37: base::TaskRunner*
file_task_runner,
On 2012/04/27 01:04:03, michaeln wrote:
> i think since the QuotaManager wants operations to complete in order, this
class
> does need sequencing guarantees.
> 
> base::SequencedTaskRunner*

Done.

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:74: scoped_refptr<base::TaskRunner>
origin_task_runner_;
On 2012/04/27 01:04:03, michaeln wrote:
> My vote would be to keep this as a MLP for clarity or maybe switch it to
> SingleThreadTaskRunner.

Yup, I reverted this change.

http://codereview.chromium.org/10197007/diff/4002/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:205:
base::MessageLoopProxy::current()->PostTask(
On 2012/04/27 01:04:03, michaeln wrote:
> nit: change not really needed (afaict)

This was another not-indended-fix... reverted.

http://codereview.chromium.org/10197007/diff/4002/webkit/quota/quota_task.cc
File webkit/quota/quota_task.cc (right):

http://codereview.chromium.org/10197007/diff/4002/webkit/quota/quota_task.cc#...
webkit/quota/quota_task.cc:50: original_task_runner_->DeleteSoon(FROM_HERE,
this);
On 2012/04/27 01:04:03, michaeln wrote:
> Is it ok to make this change, previously the delete would happen on the
current
> thread, now not necessarily so? Are all usages of this protected method ok
with
> that change?

Reverted this for compatibility reasons.

[michaeln, 2012-04-27 22:31:07]

this looks pretty good to me, i'd like to hear your thoughts on making a
FSC.file_task_runner() accessor and using that to  eliminate some of the ctor
params on various related classes.

seem like that might make things more clear... and reduce the possibility of
callers passing in 'runners' that put things on different sequences/threads than
expected.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_context.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.cc:189: origin_url, file_system_type,
file_path, file_task_runner_, this);
sgtm to pass in the 'runner' the context was ctor'd with, here and below!

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_context.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.h:22: class SequencedTaskRunner;
nit(picky-nit): alpha ordering

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.h:56: // false if the current task is not
running on the thread that allows
fyi: the semantics/implemetation of RunsTasksOnCurrentThread across the various
impls of TaskRunner is squisher than the old CurrentlyOn() semantics we've been
using against our set of well-known-threads.

The abstract TaskRunner specs this method in a very squishy way...
  // Returns true if the current thread is a thread on which a task
  // may be run, and false if no task will be run on the current
  // thread.
  //
  // It is valid for an implementation to always return true, or in
  // general to use 'true' as a default value.
  virtual bool RunsTasksOnCurrentThread() const = 0;


The SequencedTaskRunner derivatives have impls that are more useful than that.
The impl
backed by a message loop functions just like the old CurrentlyOn() stuff. The
impl backed
by SWP (SequencedWorkerPoolSequencedTaskRunner) is written in terms of
SWP::IsRunningSequenceOnCurrentThread() which is a close approximation of
CurrentlyOn(), but not quite the same thing...

For example, owned/refcounted object held by a Bind closure may be
deleted/finallyReleased immediately after having executed a task on the SWP (on
the same thread as having executed that task). During task execution,
RunsTasksOnCurrentThread() will return true, but during Bind closure cleanup, it
will return false (since  that closure cleanup is outside of execution of the
task).

Just pointing these subtleties out so you know what you getting into, and aren't
totally surprised when things work slightly differently when you start passing
in an instance of SequencedWorkerPoolSequencedTaskRunner to the
FileSystemContext ctor as the file_task_runner.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.h:67: // This method can be called on any
thread.
Is this this comment correct? DCHECKs in the method bodies suggest otherwise?

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_dir_url_request_job.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_dir_url_request_job.cc:46:
file_thread_proxy_(file_thread_proxy),
Maybe we can delete the file_thread_proxy_ data member entirely now that the
'op' automatically picks up the 'runner' the context was ctor'd with.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_file_reader.cc:40: : task_runner_(task_runner),
ditto, maybe we can pick this 'runner' up from the context and remove the data
member here

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_file_reader.cc:92: new LocalFileReader(task_runner_,
maybe we could use a context_->file_task_runner() accessor here?

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_mount_point_provider.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_mount_point_provider.h:95: base::SequencedTaskRunner*
task_runner,
if 'context' had an accessor for file_task_runner(), maybe the param could be
removed from this interface (ditto below)

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_operation.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_operation.h:133:
FileSystemOperation(base::SequencedTaskRunner* task_runner,
similarly, maybe the 'runner' could be picked up from the context

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_quota_client.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_quota_client.h:37: base::SequencedTaskRunner*
file_task_runner,
ditto, maybe can be picked up thru the 'context'

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/quota/quota...
File webkit/quota/quota_task.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/quota/quota...
webkit/quota/quota_task.h:86: base::TaskRunner* target_task_runner() const {
aside: i'm glad to see the scoped_refptr<> by value for simple getters and input
params going by the wayside!

[ericu, 2012-05-01 00:30:35]

I don't really have anything to add to Michael's review; he knows this stuff
better than I do.  But I do like his idea of grabbing the taskrunner from the
context for safety and compactness.

http://codereview.chromium.org/10197007/diff/30037/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/10197007/diff/30037/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.h:67: // This method can be called on any
thread.
On 2012/04/27 22:31:08, michaeln wrote:
> Is this this comment correct? DCHECKs in the method bodies suggest otherwise?

It used to go with IsStorageUnlimited, and should have been deleted when that
went away.

http://codereview.chromium.org/10197007/diff/30037/webkit/quota/quota_task.cc
File webkit/quota/quota_task.cc (left):

http://codereview.chromium.org/10197007/diff/30037/webkit/quota/quota_task.cc...
webkit/quota/quota_task.cc:71:
DCHECK(target_message_loop_->BelongsToCurrentThread());
Did this DCHECK go away on purpose, or was it just that you hadn't yet found
RunsTasksOnCurrentThread?

[kinuko, 2012-05-04 19:05:35]

Thanks, updated the patch.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_context.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.cc:189: origin_url, file_system_type,
file_path, file_task_runner_, this);
On 2012/04/27 22:31:08, michaeln wrote:
> sgtm to pass in the 'runner' the context was ctor'd with, here and below!

Done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_context.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.h:22: class SequencedTaskRunner;
On 2012/04/27 22:31:08, michaeln wrote:
> nit(picky-nit): alpha ordering

Done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.h:56: // false if the current task is not
running on the thread that allows
On 2012/04/27 22:31:08, michaeln wrote:
> fyi: the semantics/implemetation of RunsTasksOnCurrentThread across the
various
> impls of TaskRunner is squisher than the old CurrentlyOn() semantics we've
been
> using against our set of well-known-threads.
> 
> The abstract TaskRunner specs this method in a very squishy way...
>   // Returns true if the current thread is a thread on which a task
>   // may be run, and false if no task will be run on the current
>   // thread.
>   //
>   // It is valid for an implementation to always return true, or in
>   // general to use 'true' as a default value.
>   virtual bool RunsTasksOnCurrentThread() const = 0;

Yes, I had read this comment and how they are implemented in SWP / STP backed by
MessageLoopProxy since I wasn't comfortable replacing them without looking into
the actual code.

> The SequencedTaskRunner derivatives have impls that are more useful than that.
> The impl
> backed by a message loop functions just like the old CurrentlyOn() stuff. The
> impl backed
> by SWP (SequencedWorkerPoolSequencedTaskRunner) is written in terms of
> SWP::IsRunningSequenceOnCurrentThread() which is a close approximation of
> CurrentlyOn(), but not quite the same thing...
> 
> For example, owned/refcounted object held by a Bind closure may be
> deleted/finallyReleased immediately after having executed a task on the SWP
(on
> the same thread as having executed that task). During task execution,
> RunsTasksOnCurrentThread() will return true, but during Bind closure cleanup,
it
> will return false (since  that closure cleanup is outside of execution of the
> task).
>
> Just pointing these subtleties out so you know what you getting into, and
aren't
> totally surprised when things work slightly differently when you start passing
> in an instance of SequencedWorkerPoolSequencedTaskRunner to the
> FileSystemContext ctor as the file_task_runner.

Thanks, the actual example helps me understand.  I'm expecting
SWP::RunsCurrentTasksOnCurrentThread() could behave differently from what 
BelongsToCurrentThread() used to, but such subtleties may still surprise me :)

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_context.h:67: // This method can be called on any
thread.
On 2012/05/01 00:30:35, ericu wrote:
> On 2012/04/27 22:31:08, michaeln wrote:
> > Is this this comment correct? DCHECKs in the method bodies suggest
otherwise?
> 
> It used to go with IsStorageUnlimited, and should have been deleted when that
> went away.

Done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_dir_url_request_job.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_dir_url_request_job.cc:46:
file_thread_proxy_(file_thread_proxy),
On 2012/04/27 22:31:08, michaeln wrote:
> Maybe we can delete the file_thread_proxy_ data member entirely now that the
> 'op' automatically picks up the 'runner' the context was ctor'd with.

Done.  That's true.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_file_reader.cc:40: : task_runner_(task_runner),
On 2012/04/27 22:31:08, michaeln wrote:
> ditto, maybe we can pick this 'runner' up from the context and remove the data
> member here

Done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_file_reader.cc:92: new LocalFileReader(task_runner_,
On 2012/04/27 22:31:08, michaeln wrote:
> maybe we could use a context_->file_task_runner() accessor here?

SGTM, done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_mount_point_provider.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_mount_point_provider.h:95: base::SequencedTaskRunner*
task_runner,
On 2012/04/27 22:31:08, michaeln wrote:
> if 'context' had an accessor for file_task_runner(), maybe the param could be
> removed from this interface (ditto below)

Done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_operation.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_operation.h:133:
FileSystemOperation(base::SequencedTaskRunner* task_runner,
On 2012/04/27 22:31:08, michaeln wrote:
> similarly, maybe the 'runner' could be picked up from the context

Done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
File webkit/fileapi/file_system_quota_client.h (right):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/fileapi/fil...
webkit/fileapi/file_system_quota_client.h:37: base::SequencedTaskRunner*
file_task_runner,
On 2012/04/27 22:31:08, michaeln wrote:
> ditto, maybe can be picked up thru the 'context'

Done.

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/quota/quota...
File webkit/quota/quota_task.cc (left):

https://chromiumcodereview.appspot.com/10197007/diff/30037/webkit/quota/quota...
webkit/quota/quota_task.cc:71:
DCHECK(target_message_loop_->BelongsToCurrentThread());
On 2012/05/01 00:30:35, ericu wrote:
> Did this DCHECK go away on purpose, or was it just that you hadn't yet found
> RunsTasksOnCurrentThread?

Good catch; the latter.  Restored.

[michaeln, 2012-05-04 19:44:46]

LGTM!