FileSystem code cleanup 2nd cut - introduce SandboxedFileSystemOperation

FileSystem code cleanup 2nd cut - introduce SandboxedFileSystemOperation

1. Introduced SandboxedFileSystemOperation.
2. Factored out most of the PathManager/QuotaManager related code from
FileSystemDispatcherHost to the SandboxedFileSystemOperation.

BUG=60243
TEST=none

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

[kinuko, 2010-10-25 22:38:27]

The main part of the refactoring.  (The test_shell change will come later)

[ericu, 2010-10-27 00:05:22]

http://codereview.chromium.org/4054003/diff/15001/16003
File chrome/browser/file_system/browser_file_system_context.h (right):

http://codereview.chromium.org/4054003/diff/15001/16003#newcode17
chrome/browser/file_system/browser_file_system_context.h:17: // that shared by
the same profile.  This class is just a thin wrapper of
s/of/around/

http://codereview.chromium.org/4054003/diff/15001/16003#newcode18
chrome/browser/file_system/browser_file_system_context.h:18: //
fileapi::SandboxedFileSystemContext.
Are there going to be any other holders of or classes derived from
SandboxedFileSystemContext?  If not, do we really need two classes?

http://codereview.chromium.org/4054003/diff/15001/16005
File chrome/browser/file_system/file_system_dispatcher_host.h (right):

http://codereview.chromium.org/4054003/diff/15001/16005#newcode102
chrome/browser/file_system/file_system_dispatcher_host.h:102: typedef
IDMap<fileapi::FileSystemOperation, IDMapOwnPointer> OperationsMap;
Will we have any FileSystemOperations stored here that aren't
SandboxedFileSystemOperations?

http://codereview.chromium.org/4054003/diff/15001/16009
File webkit/fileapi/sandboxed_file_system_operation.cc (right):

http://codereview.chromium.org/4054003/diff/15001/16009#newcode5
webkit/fileapi/sandboxed_file_system_operation.cc:5: #include
"webkit/fileapi/sandboxed_file_system_operation.h"
I really like the way these wrapped calls worked out.  Nice cleanup!

http://codereview.chromium.org/4054003/diff/15001/16009#newcode33
webkit/fileapi/sandboxed_file_system_operation.cc:33:
sandbox_context_->path_manager()->GetFileSystemRootPath(
sandbox_context_ sounds to me like the context of a sandbox, not the sandboxed
context of a filesystem.  I'd prefer either sandboxed_context_ or just
file_system_context_ or ???

http://codereview.chromium.org/4054003/diff/15001/16010
File webkit/fileapi/sandboxed_file_system_operation.h (right):

http://codereview.chromium.org/4054003/diff/15001/16010#newcode62
webkit/fileapi/sandboxed_file_system_operation.h:62: kOperationOpenFileSystem =
100,
Where is this used?

[dumi, 2010-10-27 00:06:49]

http://codereview.chromium.org/4054003/diff/15001/16006
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/4054003/diff/15001/16006#newcode106
webkit/fileapi/file_system_operation.h:106:
scoped_ptr<FileSystemCallbackDispatcher> dispatcher_;
i personally prefer to make all fields 'private', and add public/protected
getters as needed. it's up to you if you want to change this.

[michaeln, 2010-10-27 00:15:27]

Good to see more of the logic being put into the fileapi library!

Is there a reason to have a split between FileSystemOperation and
SandboxedFileSystemOperation? I wonder of those two classes can (should) be
merged?

[ericu, 2010-10-27 00:19:05]

I actually liked the split; it lets you put functionality in one place and
protection in the other.
Do you think it's too much code for that?

On Tue, Oct 26, 2010 at 5:15 PM, <michaeln@chromium.org> wrote:

> Good to see more of the logic being put into the fileapi library!
>
> Is there a reason to have a split between FileSystemOperation and
> SandboxedFileSystemOperation? I wonder of those two classes can (should) be
> merged?
>
>
> http://codereview.chromium.org/4054003/show
>

[kinuko, 2010-10-27 01:07:45]

I kept FileSystemOperation as a separate class mainly for testing; so that
we can test raw filesystem operations separately.   Merging sounds
reasonable too, though I don't have strong motivation to do that right
now...

On Tue, Oct 26, 2010 at 5:18 PM, Eric Uhrhane <ericu@chromium.org> wrote:

> I actually liked the split; it lets you put functionality in one place and
> protection in the other.
> Do you think it's too much code for that?
>
>
> On Tue, Oct 26, 2010 at 5:15 PM, <michaeln@chromium.org> wrote:
>
>> Good to see more of the logic being put into the fileapi library!
>>
>> Is there a reason to have a split between FileSystemOperation and
>> SandboxedFileSystemOperation? I wonder of those two classes can (should)
>> be
>> merged?
>>
>>
>> http://codereview.chromium.org/4054003/show
>>
>
>

[michaeln, 2010-10-27 19:31:17]

On 2010/10/27 01:07:45, Kinuko Yasuda wrote:
> I kept FileSystemOperation as a separate class mainly for testing; so that
> we can test raw filesystem operations separately.   Merging sounds
> reasonable too, though I don't have strong motivation to do that right
> now...
> 
> On Tue, Oct 26, 2010 at 5:18 PM, Eric Uhrhane <mailto:ericu@chromium.org>
wrote:
> 
> > I actually liked the split; it lets you put functionality in one place and
> > protection in the other.
> > Do you think it's too much code for that?

This change LGTM, was just probing to understand the rationale.

There's a little downside:
* some methods are virtualized
* extra cognitive costs to understand the hierarchy

But also some upside:
* division between 'function' and 'protection'
* more test friendly

[kinuko, 2010-10-27 23:59:29]

http://codereview.chromium.org/4054003/diff/15001/16003
File chrome/browser/file_system/browser_file_system_context.h (right):

http://codereview.chromium.org/4054003/diff/15001/16003#newcode17
chrome/browser/file_system/browser_file_system_context.h:17: // that shared by
the same profile.  This class is just a thin wrapper of
On 2010/10/27 00:05:22, ericu wrote:
> s/of/around/

Done.

http://codereview.chromium.org/4054003/diff/15001/16003#newcode18
chrome/browser/file_system/browser_file_system_context.h:18: //
fileapi::SandboxedFileSystemContext.
On 2010/10/27 00:05:22, ericu wrote:
> Are there going to be any other holders of or classes derived from
> SandboxedFileSystemContext?  If not, do we really need two classes?

No.  test_shell will be going to have a FileSystemContext too but there'll be no
subclassing.

This class is left just to ensure quota methods are called on the right thread
but actually could be removed.  Having 'SandboxedFileSystemContext' instance
everywhere in the chrome code may look a bit awkward though?  Should we rename
it to FileSystemContext (and if we do so merging SandboxedFSOperation into
FSOperation may sound more reasonable)?  Wdyt?

http://codereview.chromium.org/4054003/diff/15001/16005
File chrome/browser/file_system/file_system_dispatcher_host.h (right):

http://codereview.chromium.org/4054003/diff/15001/16005#newcode102
chrome/browser/file_system/file_system_dispatcher_host.h:102: typedef
IDMap<fileapi::FileSystemOperation, IDMapOwnPointer> OperationsMap;
On 2010/10/27 00:05:22, ericu wrote:
> Will we have any FileSystemOperations stored here that aren't
> SandboxedFileSystemOperations?

No; just didn't like the long name.  Changed to SandboxedFileSystemOperation.

http://codereview.chromium.org/4054003/diff/15001/16006
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/4054003/diff/15001/16006#newcode106
webkit/fileapi/file_system_operation.h:106:
scoped_ptr<FileSystemCallbackDispatcher> dispatcher_;
On 2010/10/27 00:06:50, dumi wrote:
> i personally prefer to make all fields 'private', and add public/protected
> getters as needed. it's up to you if you want to change this.

Done.  (Left pending_operation_ as is as it's only for debugging)

http://codereview.chromium.org/4054003/diff/15001/16009
File webkit/fileapi/sandboxed_file_system_operation.cc (right):

http://codereview.chromium.org/4054003/diff/15001/16009#newcode33
webkit/fileapi/sandboxed_file_system_operation.cc:33:
sandbox_context_->path_manager()->GetFileSystemRootPath(
On 2010/10/27 00:05:22, ericu wrote:
> sandbox_context_ sounds to me like the context of a sandbox, not the sandboxed
> context of a filesystem.  I'd prefer either sandboxed_context_ or just
> file_system_context_ or ???

Changed to file_system_context_.

http://codereview.chromium.org/4054003/diff/15001/16010
File webkit/fileapi/sandboxed_file_system_operation.h (right):

http://codereview.chromium.org/4054003/diff/15001/16010#newcode62
webkit/fileapi/sandboxed_file_system_operation.h:62: kOperationOpenFileSystem =
100,
On 2010/10/27 00:05:22, ericu wrote:
> Where is this used?

In OpenFileSystem().  (It was setting kOperationCreateFile in the previous
version due to my blind copy-and-paste.  It's fixed now.)

[ericu, 2010-10-29 02:24:58]

http://codereview.chromium.org/4054003/diff/15001/16003
File chrome/browser/file_system/browser_file_system_context.h (right):

http://codereview.chromium.org/4054003/diff/15001/16003#newcode18
chrome/browser/file_system/browser_file_system_context.h:18: //
fileapi::SandboxedFileSystemContext.
On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> On 2010/10/27 00:05:22, ericu wrote:
> > Are there going to be any other holders of or classes derived from
> > SandboxedFileSystemContext?  If not, do we really need two classes?
> 
> No.  test_shell will be going to have a FileSystemContext too but there'll be
no
> subclassing.
> 
> This class is left just to ensure quota methods are called on the right thread
> but actually could be removed.  Having 'SandboxedFileSystemContext' instance
> everywhere in the chrome code may look a bit awkward though?  Should we rename
> it to FileSystemContext (and if we do so merging SandboxedFSOperation into
> FSOperation may sound more reasonable)?  Wdyt?

If test_shell's FileSystemContext won't be derived from, or hold on to an
instance of, this FileSystemContext, then I don't think we need two classes
here.  I like the idea of merging them and calling the resulting class
FileSystemContext.

I think SandboxedFSOperation has more reason to be separate; I kind of like the
split.  However, if you feel like merging those, I'm fine with that too.

http://codereview.chromium.org/4054003/diff/15001/16010
File webkit/fileapi/sandboxed_file_system_operation.h (right):

http://codereview.chromium.org/4054003/diff/15001/16010#newcode62
webkit/fileapi/sandboxed_file_system_operation.h:62: kOperationOpenFileSystem =
100,
On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> On 2010/10/27 00:05:22, ericu wrote:
> > Where is this used?
> 
> In OpenFileSystem().  (It was setting kOperationCreateFile in the previous
> version due to my blind copy-and-paste.  It's fixed now.)

We assert on the others here and there.  Where could we put an assert that would
have failed with this typo?  Perhaps in DidGetRootPath?

[kinuko, 2010-11-02 07:53:24]

Thanks, please find my response inline.

http://codereview.chromium.org/4054003/diff/15001/16003
File chrome/browser/file_system/browser_file_system_context.h (right):

http://codereview.chromium.org/4054003/diff/15001/16003#newcode18
chrome/browser/file_system/browser_file_system_context.h:18: //
fileapi::SandboxedFileSystemContext.
On 2010/10/29 02:24:59, ericu wrote:
> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> > On 2010/10/27 00:05:22, ericu wrote:
> > > Are there going to be any other holders of or classes derived from
> > > SandboxedFileSystemContext?  If not, do we really need two classes?
> > 
> > No.  test_shell will be going to have a FileSystemContext too but there'll
be
> no
> > subclassing.
> > 
> > This class is left just to ensure quota methods are called on the right
thread
> > but actually could be removed.  Having 'SandboxedFileSystemContext' instance
> > everywhere in the chrome code may look a bit awkward though?  Should we
rename
> > it to FileSystemContext (and if we do so merging SandboxedFSOperation into
> > FSOperation may sound more reasonable)?  Wdyt?
> 
> If test_shell's FileSystemContext won't be derived from, or hold on to an
> instance of, this FileSystemContext, then I don't think we need two classes
> here.

On the second thought, this browser class also ensures that it gets deleted on
the right thread.  If we remove this class we lose all the checks/validations
for thread correctness.  Can we just keep these classes as is? After all we may
have more browser specific code later.

http://codereview.chromium.org/4054003/diff/15001/16010
File webkit/fileapi/sandboxed_file_system_operation.h (right):

http://codereview.chromium.org/4054003/diff/15001/16010#newcode62
webkit/fileapi/sandboxed_file_system_operation.h:62: kOperationOpenFileSystem =
100,
On 2010/10/29 02:24:59, ericu wrote:
> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> > On 2010/10/27 00:05:22, ericu wrote:
> > > Where is this used?
> > 
> > In OpenFileSystem().  (It was setting kOperationCreateFile in the previous
> > version due to my blind copy-and-paste.  It's fixed now.)
> 
> We assert on the others here and there.  Where could we put an assert that
would
> have failed with this typo?  Perhaps in DidGetRootPath?

This variable is only for assertions in DEBUG build.  IMHO if we have no
assertions that would have failed by this typo in the current code paths/tests I
don't think we need another one.

[ericu, 2010-11-05 23:30:49]

On Tue, Nov 2, 2010 at 12:53 AM,  <kinuko@chromium.org> wrote:
> Thanks, please find my response inline.
>
>
> http://codereview.chromium.org/4054003/diff/15001/16003
> File chrome/browser/file_system/browser_file_system_context.h (right):
>
> http://codereview.chromium.org/4054003/diff/15001/16003#newcode18
> chrome/browser/file_system/browser_file_system_context.h:18: //
> fileapi::SandboxedFileSystemContext.
> On 2010/10/29 02:24:59, ericu wrote:
>>
>> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
>> > On 2010/10/27 00:05:22, ericu wrote:
>> > > Are there going to be any other holders of or classes derived from
>> > > SandboxedFileSystemContext?  If not, do we really need two
>
> classes?
>>
>> >
>> > No.  test_shell will be going to have a FileSystemContext too but
>
> there'll be
>>
>> no
>> > subclassing.
>> >
>> > This class is left just to ensure quota methods are called on the
>
> right thread
>>
>> > but actually could be removed.  Having 'SandboxedFileSystemContext'
>
> instance
>>
>> > everywhere in the chrome code may look a bit awkward though?  Should
>
> we rename
>>
>> > it to FileSystemContext (and if we do so merging
>
> SandboxedFSOperation into
>>
>> > FSOperation may sound more reasonable)?  Wdyt?
>
>> If test_shell's FileSystemContext won't be derived from, or hold on to
>
> an
>>
>> instance of, this FileSystemContext, then I don't think we need two
>
> classes
>>
>> here.
>
> On the second thought, this browser class also ensures that it gets
> deleted on the right thread.  If we remove this class we lose all the
> checks/validations for thread correctness.  Can we just keep these
> classes as is? After all we may have more browser specific code later.

If the whole purpose of the extra class is just those two DCHECKs, can
we just move them into the parent class?

If the parent class has no use outside the browser, then it's OK to
add all the browser-specific code there.

> http://codereview.chromium.org/4054003/diff/15001/16010
> File webkit/fileapi/sandboxed_file_system_operation.h (right):
>
> http://codereview.chromium.org/4054003/diff/15001/16010#newcode62
> webkit/fileapi/sandboxed_file_system_operation.h:62:
> kOperationOpenFileSystem = 100,
> On 2010/10/29 02:24:59, ericu wrote:
>>
>> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
>> > On 2010/10/27 00:05:22, ericu wrote:
>> > > Where is this used?
>> >
>> > In OpenFileSystem().  (It was setting kOperationCreateFile in the
>
> previous
>>
>> > version due to my blind copy-and-paste.  It's fixed now.)
>
>> We assert on the others here and there.  Where could we put an assert
>
> that would
>>
>> have failed with this typo?  Perhaps in DidGetRootPath?
>
> This variable is only for assertions in DEBUG build.  IMHO if we have no
> assertions that would have failed by this typo in the current code
> paths/tests I don't think we need another one.
>
> http://codereview.chromium.org/4054003/show
>

[kinuko, 2010-11-06 01:22:50]

On Fri, Nov 5, 2010 at 4:30 PM, Eric Uhrhane <ericu@chromium.org> wrote:

> On Tue, Nov 2, 2010 at 12:53 AM,  <kinuko@chromium.org> wrote:
> > Thanks, please find my response inline.
> >
> >
> > http://codereview.chromium.org/4054003/diff/15001/16003
> > File chrome/browser/file_system/browser_file_system_context.h (right):
> >
> > http://codereview.chromium.org/4054003/diff/15001/16003#newcode18
> > chrome/browser/file_system/browser_file_system_context.h:18: //
> > fileapi::SandboxedFileSystemContext.
> > On 2010/10/29 02:24:59, ericu wrote:
> >>
> >> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> >> > On 2010/10/27 00:05:22, ericu wrote:
> >> > > Are there going to be any other holders of or classes derived from
> >> > > SandboxedFileSystemContext?  If not, do we really need two
> >
> > classes?
> >>
> >> >
> >> > No.  test_shell will be going to have a FileSystemContext too but
> >
> > there'll be
> >>
> >> no
> >> > subclassing.
> >> >
> >> > This class is left just to ensure quota methods are called on the
> >
> > right thread
> >>
> >> > but actually could be removed.  Having 'SandboxedFileSystemContext'
> >
> > instance
> >>
> >> > everywhere in the chrome code may look a bit awkward though?  Should
> >
> > we rename
> >>
> >> > it to FileSystemContext (and if we do so merging
> >
> > SandboxedFSOperation into
> >>
> >> > FSOperation may sound more reasonable)?  Wdyt?
> >
> >> If test_shell's FileSystemContext won't be derived from, or hold on to
> >
> > an
> >>
> >> instance of, this FileSystemContext, then I don't think we need two
> >
> > classes
> >>
> >> here.
> >
> > On the second thought, this browser class also ensures that it gets
> > deleted on the right thread.  If we remove this class we lose all the
> > checks/validations for thread correctness.  Can we just keep these
> > classes as is? After all we may have more browser specific code later.
>
> If the whole purpose of the extra class is just those two DCHECKs, can
> we just move them into the parent class?
>

DCHECKs and DeleteOnIOThread.
DCHECKs check against BrowserThread.   We can have generic thread class that
hides details like appcache thread does, but I'm not feeling the strong
necessity of it at least for now.

Getting rid of DCHECKs and putting all those responsibilities (calling
methods and destroying it on the right thread) is another option but usually
I love to have these checks when it comes to threading.

If the parent class has no use outside the browser, then it's OK to
> add all the browser-specific code there.
>
> > http://codereview.chromium.org/4054003/diff/15001/16010
> > File webkit/fileapi/sandboxed_file_system_operation.h (right):
> >
> > http://codereview.chromium.org/4054003/diff/15001/16010#newcode62
> > webkit/fileapi/sandboxed_file_system_operation.h:62:
> > kOperationOpenFileSystem = 100,
> > On 2010/10/29 02:24:59, ericu wrote:
> >>
> >> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> >> > On 2010/10/27 00:05:22, ericu wrote:
> >> > > Where is this used?
> >> >
> >> > In OpenFileSystem().  (It was setting kOperationCreateFile in the
> >
> > previous
> >>
> >> > version due to my blind copy-and-paste.  It's fixed now.)
> >
> >> We assert on the others here and there.  Where could we put an assert
> >
> > that would
> >>
> >> have failed with this typo?  Perhaps in DidGetRootPath?
> >
> > This variable is only for assertions in DEBUG build.  IMHO if we have no
> > assertions that would have failed by this typo in the current code
> > paths/tests I don't think we need another one.
> >
> > http://codereview.chromium.org/4054003/show
> >
>

[ericu, 2010-11-09 00:01:30]

On 2010/11/06 01:22:50, Kinuko Yasuda wrote:
> On Fri, Nov 5, 2010 at 4:30 PM, Eric Uhrhane <mailto:ericu@chromium.org>
wrote:
> 
> > On Tue, Nov 2, 2010 at 12:53 AM,  <mailto:kinuko@chromium.org> wrote:
> > > Thanks, please find my response inline.
> > >
> > >
> > > http://codereview.chromium.org/4054003/diff/15001/16003
> > > File chrome/browser/file_system/browser_file_system_context.h (right):
> > >
> > > http://codereview.chromium.org/4054003/diff/15001/16003#newcode18
> > > chrome/browser/file_system/browser_file_system_context.h:18: //
> > > fileapi::SandboxedFileSystemContext.
> > > On 2010/10/29 02:24:59, ericu wrote:
> > >>
> > >> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> > >> > On 2010/10/27 00:05:22, ericu wrote:
> > >> > > Are there going to be any other holders of or classes derived from
> > >> > > SandboxedFileSystemContext?  If not, do we really need two
> > >
> > > classes?
> > >>
> > >> >
> > >> > No.  test_shell will be going to have a FileSystemContext too but
> > >
> > > there'll be
> > >>
> > >> no
> > >> > subclassing.
> > >> >
> > >> > This class is left just to ensure quota methods are called on the
> > >
> > > right thread
> > >>
> > >> > but actually could be removed.  Having 'SandboxedFileSystemContext'
> > >
> > > instance
> > >>
> > >> > everywhere in the chrome code may look a bit awkward though?  Should
> > >
> > > we rename
> > >>
> > >> > it to FileSystemContext (and if we do so merging
> > >
> > > SandboxedFSOperation into
> > >>
> > >> > FSOperation may sound more reasonable)?  Wdyt?
> > >
> > >> If test_shell's FileSystemContext won't be derived from, or hold on to
> > >
> > > an
> > >>
> > >> instance of, this FileSystemContext, then I don't think we need two
> > >
> > > classes
> > >>
> > >> here.
> > >
> > > On the second thought, this browser class also ensures that it gets
> > > deleted on the right thread.  If we remove this class we lose all the
> > > checks/validations for thread correctness.  Can we just keep these
> > > classes as is? After all we may have more browser specific code later.
> >
> > If the whole purpose of the extra class is just those two DCHECKs, can
> > we just move them into the parent class?
> >
> 
> DCHECKs and DeleteOnIOThread.
> DCHECKs check against BrowserThread.   We can have generic thread class that
> hides details like appcache thread does, but I'm not feeling the strong
> necessity of it at least for now.
> 
> Getting rid of DCHECKs and putting all those responsibilities (calling
> methods and destroying it on the right thread) is another option but usually
> I love to have these checks when it comes to threading.

I like the checks too, but do we need a whole separate class for them?  Why not
just get rid of the parent class and roll it all into BrowserFileSystemContext?

> If the parent class has no use outside the browser, then it's OK to
> > add all the browser-specific code there.
> >
> > > http://codereview.chromium.org/4054003/diff/15001/16010
> > > File webkit/fileapi/sandboxed_file_system_operation.h (right):
> > >
> > > http://codereview.chromium.org/4054003/diff/15001/16010#newcode62
> > > webkit/fileapi/sandboxed_file_system_operation.h:62:
> > > kOperationOpenFileSystem = 100,
> > > On 2010/10/29 02:24:59, ericu wrote:
> > >>
> > >> On 2010/10/27 23:59:29, Kinuko Yasuda wrote:
> > >> > On 2010/10/27 00:05:22, ericu wrote:
> > >> > > Where is this used?
> > >> >
> > >> > In OpenFileSystem().  (It was setting kOperationCreateFile in the
> > >
> > > previous
> > >>
> > >> > version due to my blind copy-and-paste.  It's fixed now.)
> > >
> > >> We assert on the others here and there.  Where could we put an assert
> > >
> > > that would
> > >>
> > >> have failed with this typo?  Perhaps in DidGetRootPath?
> > >
> > > This variable is only for assertions in DEBUG build.  IMHO if we have no
> > > assertions that would have failed by this typo in the current code
> > > paths/tests I don't think we need another one.
> > >
> > > http://codereview.chromium.org/4054003/show
> > >
> >
>

[kinuko, 2010-11-09 00:27:14]

On 2010/11/09 00:01:30, ericu wrote:
> > > > On the second thought, this browser class also ensures that it gets
> > > > deleted on the right thread.  If we remove this class we lose all the
> > > > checks/validations for thread correctness.  Can we just keep these
> > > > classes as is? After all we may have more browser specific code later.
> > >
> > > If the whole purpose of the extra class is just those two DCHECKs, can
> > > we just move them into the parent class?
> > >
> > 
> > DCHECKs and DeleteOnIOThread.
> > DCHECKs check against BrowserThread.   We can have generic thread class that
> > hides details like appcache thread does, but I'm not feeling the strong
> > necessity of it at least for now.
> > 
> > Getting rid of DCHECKs and putting all those responsibilities (calling
> > methods and destroying it on the right thread) is another option but usually
> > I love to have these checks when it comes to threading.
> 
> I like the checks too, but do we need a whole separate class for them?  Why
not
> just get rid of the parent class and roll it all into
BrowserFileSystemContext?

fileapi::SandboxedFileSystemOperation depends on the context class and we need
some base class that is available outside the chrome directory.   I'm also going
to extend simple_file_system (for test_shell/DRT) to use the new
SandboxedFileSystemOperation and it'll require a context instantance, which
again needs to be outside the chrome directory.

[ericu, 2010-11-09 00:29:14]

On Mon, Nov 8, 2010 at 4:27 PM,  <kinuko@chromium.org> wrote:
> On 2010/11/09 00:01:30, ericu wrote:
>>
>> > > > On the second thought, this browser class also ensures that it gets
>> > > > deleted on the right thread.  If we remove this class we lose all
>> > > > the
>> > > > checks/validations for thread correctness.  Can we just keep these
>> > > > classes as is? After all we may have more browser specific code
>> > > > later.
>> > >
>> > > If the whole purpose of the extra class is just those two DCHECKs, can
>> > > we just move them into the parent class?
>> > >
>> >
>> > DCHECKs and DeleteOnIOThread.
>> > DCHECKs check against BrowserThread.   We can have generic thread class
>> > that
>> > hides details like appcache thread does, but I'm not feeling the strong
>> > necessity of it at least for now.
>> >
>> > Getting rid of DCHECKs and putting all those responsibilities (calling
>> > methods and destroying it on the right thread) is another option but
>> > usually
>> > I love to have these checks when it comes to threading.
>
>> I like the checks too, but do we need a whole separate class for them?
>>  Why
>
> not
>>
>> just get rid of the parent class and roll it all into
>
> BrowserFileSystemContext?
>
> fileapi::SandboxedFileSystemOperation depends on the context class and we
> need
> some base class that is available outside the chrome directory.   I'm also
> going
> to extend simple_file_system (for test_shell/DRT) to use the new
> SandboxedFileSystemOperation and it'll require a context instantance, which
> again needs to be outside the chrome directory.

Ah, OK, nevermind, then.

> http://codereview.chromium.org/4054003/show
>

[ericu, 2010-11-09 00:30:14]

LGTM