Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6604020:
Introduce FileSystemFileUtil and -Proxy to decorate base::file_util in webkit/fileapi. (Closed)
|
Created:
9 years, 9 months ago by Dai Mikurube (google.com) Modified:
9 years, 7 months ago CC:
chromium-reviews, kinuko+watch, darin-cc_chromium.org, brettw-cc_chromium.org, Dai Mikurube (NOT FULLTIME) Visibility:
Public. |
DescriptionIntroduce FileSystemFileUtil and -Proxy to decorate base::file_util in webkit/fileapi.
BUG=74841
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76875
Patch Set 1 #
Total comments: 18
Patch Set 2 : Reflected comments. #
Total comments: 15
Patch Set 3 : Fixed. #Patch Set 4 : Passing FileSystemOperationContext by value. #
Total comments: 1
Patch Set 5 : Modified webkit_fileapi.gypi. #
Total comments: 1
Patch Set 6 : Made file_system_operation_context_ a simple variable (replaced from not scoped_ptr.) #
Messages
Total messages: 29 (0 generated)
Picked out fileapi::FileSystemFileUtil -related parts from http://codereview.chromium.org/6286038/ and http://codereview.chromium.org/6471019/ . (i.e. factored out Obfuscation- and Quota-) It has no unit tests yet.
Are those unit tests you mentioned close enough to ready that you can include them in this CL? This code's not supposed to change anything at all, but it's largeley untested from my end. Thanks for putting this together. http://codereview.chromium.org/6604020/diff/1/base/format_macros.h File base/format_macros.h (right): http://codereview.chromium.org/6604020/diff/1/base/format_macros.h#newcode51 base/format_macros.h:51: #define PRIxS "zx" This was only needed for the obfuscation code; you can drop this file from the CL. I think we *should* have these macros, they're just not relevant to this change. Feel free to add them in a separate CL if you want, but don't bother on my account. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... File webkit/fileapi/file_system_context.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... webkit/fileapi/file_system_context.cc:9: #include "webkit/fileapi/file_system_file_util.h" Are these needed? http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... File webkit/fileapi/file_system_context.h (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... webkit/fileapi/file_system_context.h:22: class FileSystemFileUtil; FileSystemFileUtil isn't needed here yet. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util.cc:16: // or not we obfuscate before calling in [if needed]. This comment is somewhat obsolete. We will need to virtualize all these calls. We should do that by making this method a non-virtual member of FileSystemFileUtil, but changing all of its file_util calls to be FileSystemFileUtil calls. That way when we override them for obfuscation or quota, it'll just work. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util.cc:134: std::vector<base::FileUtilProxy::Entry>* entries) { In this method I mixed FileUtilProxy::Entry and FileSystemFileUtilProxy::Entry [which are typedeffed to be the same thing]. That's why both are #included at the top of thise file. I think we can just use the FileUtilProxy version here, and drop the other #include. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util_proxy.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util_proxy.cc:54: return fileapi::FileSystemFileUtil::GetInstance(); Rather than access the singleton getter here, I think it would be cleaner if the FileSystemOperationContext held a FileSystemFileUtil, which we can just give it at its construction time. For now you can just call GetInstance there; later on we'll want to allow the extension code to pass in its own choice, or optionally to create a DB-based FSFU once that code is in. As long as we're using singletons or other stateless objects, there's no issue with getting them on the IO thread and using them on the File thread. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_oper... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.h:34: class FileSystemFileUtilProxy; I don't think we need the FileSystemFileUtilProxy declaration. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_writer_dele... File webkit/fileapi/file_writer_delegate.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_writer_dele... webkit/fileapi/file_writer_delegate.cc:166: } // namespace fileapi You can remove this file from the CL.
Thanks for your review. :) Yes, it changes nothing, but untested. Unit tests are far from ready... I'll upload tests later with reflecting your comments.
http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util_proxy.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util_proxy.cc:54: return fileapi::FileSystemFileUtil::GetInstance(); On 2011/03/03 01:05:11, ericu wrote: > Rather than access the singleton getter here, I think it would be cleaner if the > FileSystemOperationContext held a FileSystemFileUtil, which we can just give it > at its construction time. For now you can just call GetInstance there; later on > we'll want to allow the extension code to pass in its own choice, or optionally > to create a DB-based FSFU once that code is in. > > As long as we're using singletons or other stateless objects, there's no issue > with getting them on the IO thread and using them on the File thread. Incidentally, this explicitly only works for operations within a single filesystem. At some point we're going to have to support operations that copy or move from one filesystem to another, and we'll have to do that some other way. One idea that comes to mind is to detect early on that the operation spans filesystems, and have a high-level copy operation, like a shell operation, that does mkdir/read/write/rm as needed to accomplish it. Any operation that involves only a single filesystem can be handled much more simply and efficiently by that filesystem's FSFU code. We could potentially make a more optimized version of cross-filesystem copy, but that's way down on our list of priorities.
On Thu, Mar 3, 2011 at 10:05 AM, <ericu@chromium.org> wrote: > Are those unit tests you mentioned close enough to ready that you can > include > them in this CL? This code's not supposed to change anything at all, but > it's > largeley untested from my end. > Note that we have file_system_operation_unittest and LayoutTests with simple_file_system. I really wonder if we need more tests than that at this level, especially we don't change any functionalities. Thanks for putting this together. > > > http://codereview.chromium.org/6604020/diff/1/base/format_macros.h > File base/format_macros.h (right): > > > http://codereview.chromium.org/6604020/diff/1/base/format_macros.h#newcode51 > base/format_macros.h:51: #define PRIxS "zx" > This was only needed for the obfuscation code; you can drop this file > from the CL. > > I think we *should* have these macros, they're just not relevant to this > change. Feel free to add them in a separate CL if you want, but don't > bother on my account. > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... > File webkit/fileapi/file_system_context.cc (right): > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... > webkit/fileapi/file_system_context.cc:9: #include > > "webkit/fileapi/file_system_file_util.h" > Are these needed? > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... > File webkit/fileapi/file_system_context.h (right): > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... > webkit/fileapi/file_system_context.h:22: class FileSystemFileUtil; > FileSystemFileUtil isn't needed here yet. > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... > File webkit/fileapi/file_system_file_util.cc (right): > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... > webkit/fileapi/file_system_file_util.cc:16: // or not we obfuscate > before calling in [if needed]. > This comment is somewhat obsolete. We will need to virtualize all these > calls. We should do that by making this method a non-virtual member of > FileSystemFileUtil, but changing all of its file_util calls to be > FileSystemFileUtil calls. That way when we override them for > obfuscation or quota, it'll just work. > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... > webkit/fileapi/file_system_file_util.cc:134: > std::vector<base::FileUtilProxy::Entry>* entries) { > In this method I mixed FileUtilProxy::Entry and > FileSystemFileUtilProxy::Entry [which are typedeffed to be the same > thing]. That's why both are #included at the top of thise file. I > think we can just use the FileUtilProxy version here, and drop the other > #include. > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... > File webkit/fileapi/file_system_file_util_proxy.cc (right): > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... > webkit/fileapi/file_system_file_util_proxy.cc:54: return > fileapi::FileSystemFileUtil::GetInstance(); > Rather than access the singleton getter here, I think it would be > cleaner if the FileSystemOperationContext held a FileSystemFileUtil, > which we can just give it at its construction time. For now you can > just call GetInstance there; later on we'll want to allow the extension > code to pass in its own choice, or optionally to create a DB-based FSFU > once that code is in. > > As long as we're using singletons or other stateless objects, there's no > issue with getting them on the IO thread and using them on the File > thread. > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_oper... > File webkit/fileapi/file_system_operation.h (right): > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_oper... > webkit/fileapi/file_system_operation.h:34: class > FileSystemFileUtilProxy; > I don't think we need the FileSystemFileUtilProxy declaration. > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_writer_dele... > File webkit/fileapi/file_writer_delegate.cc (right): > > > http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_writer_dele... > webkit/fileapi/file_writer_delegate.cc:166: } // namespace fileapi > You can remove this file from the CL. > > > http://codereview.chromium.org/6604020/ >
On Wed, Mar 2, 2011 at 6:41 PM, Kinuko Yasuda <kinuko@chromium.org> wrote: > On Thu, Mar 3, 2011 at 10:05 AM, <ericu@chromium.org> wrote: >> >> Are those unit tests you mentioned close enough to ready that you can >> include >> them in this CL? This code's not supposed to change anything at all, but >> it's >> largeley untested from my end. > > Note that we have file_system_operation_unittest and LayoutTests with > simple_file_system. > I really wonder if we need more tests than that at this level, especially we > don't change any functionalities. The reason I brought it up is that I don't think we have a way of /verifying/ that we haven't changed any of the functionality. I could easily have made a typo or two in generating those files [I did, in fact, but caught at least some of them and the compiler caught others]. I'm not sure how thoroughly we can test them without too much effort, but if Dai's got something close that helps at all, I'm glad about that.
On Wed, Mar 2, 2011 at 6:46 PM, Eric Uhrhane <ericu@chromium.org> wrote: > On Wed, Mar 2, 2011 at 6:41 PM, Kinuko Yasuda <kinuko@chromium.org> wrote: > > On Thu, Mar 3, 2011 at 10:05 AM, <ericu@chromium.org> wrote: > >> > >> Are those unit tests you mentioned close enough to ready that you can > >> include > >> them in this CL? This code's not supposed to change anything at all, > but > >> it's > >> largeley untested from my end. > > > > Note that we have file_system_operation_unittest and LayoutTests with > > simple_file_system. > > I really wonder if we need more tests than that at this level, especially > we > > don't change any functionalities. > > The reason I brought it up is that I don't think we have a way of > /verifying/ that we haven't changed any of the functionality. I could > easily have made a typo or two in generating those files [I did, in > fact, but caught at least some of them and the compiler caught > others]. I'm not sure how thoroughly we can test them without too > much effort, but if Dai's got something close that helps at all, I'm > glad about that. > file_system_operation_unittest was mostly for testing file_util_proxy's methods (as FSO layer is really thin layer as you know), and have a lot of detailed operation level tests. I think it's enough for the purpose but you (Eric and Dai) could take a look at it and see if we need more.
On Wed, Mar 2, 2011 at 7:44 PM, Kinuko Yasuda <kinuko@chromium.org> wrote: > On Wed, Mar 2, 2011 at 6:46 PM, Eric Uhrhane <ericu@chromium.org> wrote: >> >> On Wed, Mar 2, 2011 at 6:41 PM, Kinuko Yasuda <kinuko@chromium.org> wrote: >> > On Thu, Mar 3, 2011 at 10:05 AM, <ericu@chromium.org> wrote: >> >> >> >> Are those unit tests you mentioned close enough to ready that you can >> >> include >> >> them in this CL? This code's not supposed to change anything at all, >> >> but >> >> it's >> >> largeley untested from my end. >> > >> > Note that we have file_system_operation_unittest and LayoutTests with >> > simple_file_system. >> > I really wonder if we need more tests than that at this level, >> > especially we >> > don't change any functionalities. >> >> The reason I brought it up is that I don't think we have a way of >> /verifying/ that we haven't changed any of the functionality. I could >> easily have made a typo or two in generating those files [I did, in >> fact, but caught at least some of them and the compiler caught >> others]. I'm not sure how thoroughly we can test them without too >> much effort, but if Dai's got something close that helps at all, I'm >> glad about that. > > file_system_operation_unittest was mostly for testing file_util_proxy's > methods (as FSO layer is really thin layer as you know), and have a lot of > detailed operation level tests. > I think it's enough for the purpose but you (Eric and Dai) could take a look > at it and see if we need more. You're right--I was misreading the test code. file_system_operation_unittest will be sufficient.
Thank you for the comments. :) Ok, I've reflected the comments and uploaded it without adding tests. http://codereview.chromium.org/6604020/diff/1/base/format_macros.h File base/format_macros.h (right): http://codereview.chromium.org/6604020/diff/1/base/format_macros.h#newcode51 base/format_macros.h:51: #define PRIxS "zx" On 2011/03/03 01:05:11, ericu wrote: > This was only needed for the obfuscation code; you can drop this file from the > CL. > > I think we *should* have these macros, they're just not relevant to this change. > Feel free to add them in a separate CL if you want, but don't bother on my > account. Ok, thanks. I've dropped them from this CL. Let's add them with a change which requires them. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... File webkit/fileapi/file_system_context.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... webkit/fileapi/file_system_context.cc:9: #include "webkit/fileapi/file_system_file_util.h" On 2011/03/03 01:05:11, ericu wrote: > Are these needed? Not required. Removed. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... File webkit/fileapi/file_system_context.h (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_cont... webkit/fileapi/file_system_context.h:22: class FileSystemFileUtil; On 2011/03/03 01:05:11, ericu wrote: > FileSystemFileUtil isn't needed here yet. Done. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util.cc:16: // or not we obfuscate before calling in [if needed]. On 2011/03/03 01:05:11, ericu wrote: > This comment is somewhat obsolete. We will need to virtualize all these calls. > We should do that by making this method a non-virtual member of > FileSystemFileUtil, but changing all of its file_util calls to be > FileSystemFileUtil calls. That way when we override them for obfuscation or > quota, it'll just work. Agreed. I've rewritten the TODO. Could you check whether the comment reflects your opinion? I've not rewritten the code since it takes some time and our first priority is to introduce this FSFileUtil layer. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util.cc:134: std::vector<base::FileUtilProxy::Entry>* entries) { On 2011/03/03 01:05:11, ericu wrote: > In this method I mixed FileUtilProxy::Entry and FileSystemFileUtilProxy::Entry > [which are typedeffed to be the same thing]. That's why both are #included at > the top of thise file. I think we can just use the FileUtilProxy version here, > and drop the other #include. Agreed. Actually, I did the same and found that introducing FSFUProxy::Entry may cause many effects to other code. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util_proxy.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util_proxy.cc:54: return fileapi::FileSystemFileUtil::GetInstance(); On 2011/03/03 02:34:07, ericu wrote: > On 2011/03/03 01:05:11, ericu wrote: > > Rather than access the singleton getter here, I think it would be cleaner if > the > > FileSystemOperationContext held a FileSystemFileUtil, which we can just give > it > > at its construction time. For now you can just call GetInstance there; later > on > > we'll want to allow the extension code to pass in its own choice, or > optionally > > to create a DB-based FSFU once that code is in. > > > > As long as we're using singletons or other stateless objects, there's no issue > > with getting them on the IO thread and using them on the File thread. > > Incidentally, this explicitly only works for operations within a single > filesystem. > > At some point we're going to have to support operations that copy or move from > one filesystem to another, and we'll have to do that some other way. One idea > that comes to mind is to detect early on that the operation spans filesystems, > and have a high-level copy operation, like a shell operation, that does > mkdir/read/write/rm as needed to accomplish it. Any operation that involves > only a single filesystem can be handled much more simply and efficiently by that > filesystem's FSFU code. > > We could potentially make a more optimized version of cross-filesystem copy, but > that's way down on our list of priorities. Agreed. I've changed FSFileUtil::GetInstance() to be called at the FileSystemOperation's constructor. Let's consider operations between two filesystems later. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_oper... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.h:34: class FileSystemFileUtilProxy; On 2011/03/03 01:05:11, ericu wrote: > I don't think we need the FileSystemFileUtilProxy declaration. Done. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_writer_dele... File webkit/fileapi/file_writer_delegate.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_writer_dele... webkit/fileapi/file_writer_delegate.cc:166: } // namespace fileapi On 2011/03/03 01:05:11, ericu wrote: > You can remove this file from the CL. Done.
LGTM other than that one nit. http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util.cc (right): http://codereview.chromium.org/6604020/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util.cc:16: // or not we obfuscate before calling in [if needed]. On 2011/03/03 20:24:52, Dai Mikurube wrote: > On 2011/03/03 01:05:11, ericu wrote: > > This comment is somewhat obsolete. We will need to virtualize all these > calls. > > We should do that by making this method a non-virtual member of > > FileSystemFileUtil, but changing all of its file_util calls to be > > FileSystemFileUtil calls. That way when we override them for obfuscation or > > quota, it'll just work. > > Agreed. I've rewritten the TODO. Could you check whether the comment reflects > your opinion? I've not rewritten the code since it takes some time and our first > priority is to introduce this FSFileUtil layer. Yes, that looks good. Thanks. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util_proxy.cc (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util_proxy.cc:55: return file_system_file_util_; Why store it separately? Why not just return context_->file_system_file_util()?
looking good. I think we'd better have a few more comments in the code. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util.cc (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util.cc:73: nit: unnecessary empty line http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util.h (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util.h:30: class FileSystemFileUtil { Can we add some comment to note that the implementation is largely taken from FileUtilProxy and TODO (you can put my name there too) to clean up FileUtilProxy or factor out common routines. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation.cc:76: bool unused) { The parameter still exists in the caller side. If we want to deprecate the flag can you add a DCHECK(!unused) to make sure no one is calling this with recursive=true and also add a TODO comment to clean up the call sites? (Then later you can make a separate small patch to do the latter) http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation.h:59: bool); I'd like to keep this signature as is until we clean up all the call sites (please see my other comment in file_system_operation.cc). http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_operation_context.h (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation_context.h:17: ~FileSystemOperationContext() { nit: do we need this? http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation_context.h:25: FileSystemFileUtil* file_system_file_util_; Please add a comment to clarify it's not owned (and supposed to be a pointer to singletons).
Thank you. :) http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util.cc (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util.cc:73: On 2011/03/03 21:29:22, kinuko wrote: > nit: unnecessary empty line Done. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util.h (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util.h:30: class FileSystemFileUtil { On 2011/03/03 21:29:22, kinuko wrote: > Can we add some comment to note that the implementation is largely taken from > FileUtilProxy and TODO (you can put my name there too) to clean up FileUtilProxy > or factor out common routines. Done. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util_proxy.cc (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util_proxy.cc:55: return file_system_file_util_; On 2011/03/03 21:27:48, ericu wrote: > Why store it separately? Why not just return context_->file_system_file_util()? It causes Segmentation Fault. I, actually, didn't debug it so precisely with gdb, but calling context_->file_system_file_util() from destructors (e.g. ~RelayCreateOrOpen()) seems suspicious. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation.cc:76: bool unused) { On 2011/03/03 21:29:22, kinuko wrote: > The parameter still exists in the caller side. > If we want to deprecate the flag can you add a DCHECK(!unused) to make sure no > one is calling this with recursive=true and also add a TODO comment to clean up > the call sites? (Then later you can make a separate small patch to do the > latter) Added DCHECK(!unused). Verified it passed in unit tests and some manual operations on the built binary. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation.h:59: bool); On 2011/03/03 21:29:22, kinuko wrote: > I'd like to keep this signature as is until we clean up all the call sites > (please see my other comment in file_system_operation.cc). Done. Replied in file_system_operation.cc. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_operation_context.h (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation_context.h:17: ~FileSystemOperationContext() { On 2011/03/03 21:29:22, kinuko wrote: > nit: do we need this? Done. http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_o... webkit/fileapi/file_system_operation_context.h:25: FileSystemFileUtil* file_system_file_util_; On 2011/03/03 21:29:22, kinuko wrote: > Please add a comment to clarify it's not owned (and supposed to be a pointer to > singletons). Done.
http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util_proxy.cc (right): http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util_proxy.cc:55: return file_system_file_util_; On 2011/03/03 22:19:10, Dai Mikurube wrote: > On 2011/03/03 21:27:48, ericu wrote: > > Why store it separately? Why not just return > context_->file_system_file_util()? > > It causes Segmentation Fault. I, actually, didn't debug it so precisely with > gdb, but calling context_->file_system_file_util() from destructors (e.g. > ~RelayCreateOrOpen()) seems suspicious. Ah, it's because the FileSystemOperationContext can go away before the MessageLoopProxy, if the FileSystemOperation goes away first. In fact, since the callback that the MessageLoopProxy calls will delete the FileSystemOperation, that'll always happen if all goes well. Please add a comment that the FileSystemOperationContext will become invalid when the callback is called, but that the FileSystemFileUtil is a singleton that lives effectively forever.
On 2011/03/03 23:13:22, ericu wrote: > http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... > File webkit/fileapi/file_system_file_util_proxy.cc (right): > > http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... > webkit/fileapi/file_system_file_util_proxy.cc:55: return file_system_file_util_; > On 2011/03/03 22:19:10, Dai Mikurube wrote: > > On 2011/03/03 21:27:48, ericu wrote: > > > Why store it separately? Why not just return > > context_->file_system_file_util()? > > > > It causes Segmentation Fault. I, actually, didn't debug it so precisely with > > gdb, but calling context_->file_system_file_util() from destructors (e.g. > > ~RelayCreateOrOpen()) seems suspicious. > > Ah, it's because the FileSystemOperationContext can go away before the > MessageLoopProxy, if the FileSystemOperation goes away first. In fact, since > the callback that the MessageLoopProxy calls will delete the > FileSystemOperation, that'll always happen if all goes well. > > Please add a comment that the FileSystemOperationContext will become invalid > when the callback is called, but that the FileSystemFileUtil is a singleton that > lives effectively forever. Can we pass the context by value? Passing a pointer across threads makes me feel a bit nervous... The heavier part will be in file_system_context (we'll need to take care of thread-safeness in the class though) and the instance won't have much other than that right?
On Thu, Mar 3, 2011 at 3:17 PM, <kinuko@chromium.org> wrote: > On 2011/03/03 23:13:22, ericu wrote: > > http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... >> >> File webkit/fileapi/file_system_file_util_proxy.cc (right): > > > http://codereview.chromium.org/6604020/diff/6003/webkit/fileapi/file_system_f... >> >> webkit/fileapi/file_system_file_util_proxy.cc:55: return > > file_system_file_util_; >> >> On 2011/03/03 22:19:10, Dai Mikurube wrote: >> > On 2011/03/03 21:27:48, ericu wrote: >> > > Why store it separately? Why not just return >> > context_->file_system_file_util()? >> > >> > It causes Segmentation Fault. I, actually, didn't debug it so precisely >> > with >> > gdb, but calling context_->file_system_file_util() from destructors >> > (e.g. >> > ~RelayCreateOrOpen()) seems suspicious. > >> Ah, it's because the FileSystemOperationContext can go away before the >> MessageLoopProxy, if the FileSystemOperation goes away first. In fact, >> since >> the callback that the MessageLoopProxy calls will delete the >> FileSystemOperation, that'll always happen if all goes well. > >> Please add a comment that the FileSystemOperationContext will become >> invalid >> when the callback is called, but that the FileSystemFileUtil is a >> singleton > > that >> >> lives effectively forever. > > Can we pass the context by value? Passing a pointer across threads makes me > feel a bit nervous... The failure mode would be if we somehow deleted the FileSystemOperation while it still had activity pending on the File thread. Can that happen? If so, then the safer way to do this would be to release the reference as we call FileSystemFileUtilProxy, passing ownership across to the File thread permanently. > The heavier part will be in file_system_context (we'll need to take care of > thread-safeness in the class though) and the instance won't have much other > than > that right? I'm not sure what else will end up in there, but there's going to be some quota stuff, right? > http://codereview.chromium.org/6604020/ >
On 2011/03/03 23:22:55, ericu wrote: > > Can we pass the context by value? Passing a pointer across threads makes me > > feel a bit nervous... > > The failure mode would be if we somehow deleted the > FileSystemOperation while it still had activity pending on the File > thread. Can that happen? I think this could happen. FSO creates callbacks using ScopedCallbackFactory so that they won't be called when the operation goes away, but it does not cancel ongoing FILE thread tasks. > If so, then the safer way to do this would > be to release the reference as we call FileSystemFileUtilProxy, > passing ownership across to the File thread permanently. Yes that's another option I was thinking. Then wondered if the operation in IO thread and FILE thread need to refer/share the same context instance (other than file_system_context). If it is I'd rather make it ThreadSafeRefCounted (or will need to take a closer look to make everything consistent). If it is not we'll be able to pass the ownership or pass-by-value. > > The heavier part will be in file_system_context (we'll need to take care of > > thread-safeness in the class though) and the instance won't have much other > > than > > that right? > > I'm not sure what else will end up in there, but there's going to be > some quota stuff, right? What we'll need (at least for the 1st cut) is only one integer. We can make other context-dependent values available via fs_context->usage_tracker if it is really necessary.
On Thu, Mar 3, 2011 at 3:45 PM, <kinuko@chromium.org> wrote: > On 2011/03/03 23:22:55, ericu wrote: >> >> > Can we pass the context by value? Passing a pointer across threads > > makes me >> >> > feel a bit nervous... > >> The failure mode would be if we somehow deleted the >> FileSystemOperation while it still had activity pending on the File >> thread. Can that happen? > > I think this could happen. FSO creates callbacks using > ScopedCallbackFactory so > that they won't be called when the operation goes away, but it does not > cancel > ongoing FILE thread tasks. > >> If so, then the safer way to do this would >> be to release the reference as we call FileSystemFileUtilProxy, >> passing ownership across to the File thread permanently. > > Yes that's another option I was thinking. Then wondered if the operation in > IO > thread and FILE thread need to refer/share the same context instance (other > than > file_system_context). If it is I'd rather make it ThreadSafeRefCounted (or > will > need to take a closer look to make everything consistent). If it is not > we'll > be able to pass the ownership or pass-by-value. I think your idea of passing it by value is better than going all the way to ThreadSafeRefCounted. It's both simpler and easier to see that it's correct. Let's just go with that, and we can change it later if the class gets big. >> > The heavier part will be in file_system_context (we'll need to take care >> > of >> > thread-safeness in the class though) and the instance won't have much >> > other >> > than >> > that right? > >> I'm not sure what else will end up in there, but there's going to be >> some quota stuff, right? > > What we'll need (at least for the 1st cut) is only one integer. > We can make other context-dependent values available via > fs_context->usage_tracker if it is really necessary. > > > http://codereview.chromium.org/6604020/ >
On Thu, Mar 3, 2011 at 3:45 PM, <kinuko@chromium.org> wrote: > On 2011/03/03 23:22:55, ericu wrote: > >> > Can we pass the context by value? Passing a pointer across threads >> > makes me > >> > feel a bit nervous... >> > > The failure mode would be if we somehow deleted the >> FileSystemOperation while it still had activity pending on the File >> thread. Can that happen? >> > > I think this could happen. FSO creates callbacks using > ScopedCallbackFactory so > that they won't be called when the operation goes away, but it does not > cancel > ongoing FILE thread tasks. Sorry, correction... the operation usually gets deleted by the callbacks so usually this shouldn't happen. > If so, then the safer way to do this would >> be to release the reference as we call FileSystemFileUtilProxy, >> passing ownership across to the File thread permanently. >> > > Yes that's another option I was thinking. Then wondered if the operation > in IO > thread and FILE thread need to refer/share the same context instance (other > than > file_system_context). If it is I'd rather make it ThreadSafeRefCounted (or > will > need to take a closer look to make everything consistent). If it is not > we'll > be able to pass the ownership or pass-by-value. > > > > The heavier part will be in file_system_context (we'll need to take care >> of >> > thread-safeness in the class though) and the instance won't have much >> other >> > than >> > that right? >> > > I'm not sure what else will end up in there, but there's going to be >> some quota stuff, right? >> > > What we'll need (at least for the 1st cut) is only one integer. > We can make other context-dependent values available via > fs_context->usage_tracker if it is really necessary. > > > > http://codereview.chromium.org/6604020/ >
On Thu, Mar 3, 2011 at 3:55 PM, Eric Uhrhane <ericu@chromium.org> wrote: > On Thu, Mar 3, 2011 at 3:45 PM, <kinuko@chromium.org> wrote: > > On 2011/03/03 23:22:55, ericu wrote: > >> > >> > Can we pass the context by value? Passing a pointer across > threads > > > > makes me > >> > >> > feel a bit nervous... > > > >> The failure mode would be if we somehow deleted the > >> FileSystemOperation while it still had activity pending on the File > >> thread. Can that happen? > > > > I think this could happen. FSO creates callbacks using > > ScopedCallbackFactory so > > that they won't be called when the operation goes away, but it does not > > cancel > > ongoing FILE thread tasks. > > > >> If so, then the safer way to do this would > >> be to release the reference as we call FileSystemFileUtilProxy, > >> passing ownership across to the File thread permanently. > > > > Yes that's another option I was thinking. Then wondered if the operation > in > > IO > > thread and FILE thread need to refer/share the same context instance > (other > > than > > file_system_context). If it is I'd rather make it ThreadSafeRefCounted > (or > > will > > need to take a closer look to make everything consistent). If it is not > > we'll > > be able to pass the ownership or pass-by-value. > > I think your idea of passing it by value is better than going all the > way to ThreadSafeRefCounted. > It's both simpler and easier to see that it's correct. > Let's just go with that, and we can change it later if the class gets big. SGTM. >> > The heavier part will be in file_system_context (we'll need to take > care > >> > of > >> > thread-safeness in the class though) and the instance won't have much > >> > other > >> > than > >> > that right? > > > >> I'm not sure what else will end up in there, but there's going to be > >> some quota stuff, right? > > > > What we'll need (at least for the 1st cut) is only one integer. > > We can make other context-dependent values available via > > fs_context->usage_tracker if it is really necessary. > > > > > > http://codereview.chromium.org/6604020/ > > >
Ok, I've changed FileSystemOperationContext to be passed by value to the File thread. Passing-by-value (copying) is done at the constructor of MessageLoopRelay.
The rest LGTM if you can get the trybots happier. http://codereview.chromium.org/6604020/diff/4006/webkit/fileapi/file_system_f... File webkit/fileapi/file_system_file_util_proxy.cc (right): http://codereview.chromium.org/6604020/diff/4006/webkit/fileapi/file_system_f... webkit/fileapi/file_system_file_util_proxy.cc:94: fileapi::FileSystemFileUtilProxy::Close(*context(), That * looks like a bug.
On 2011/03/04 01:05:34, ericu wrote: > The rest LGTM if you can get the trybots happier. > > http://codereview.chromium.org/6604020/diff/4006/webkit/fileapi/file_system_f... > File webkit/fileapi/file_system_file_util_proxy.cc (right): > > http://codereview.chromium.org/6604020/diff/4006/webkit/fileapi/file_system_f... > webkit/fileapi/file_system_file_util_proxy.cc:94: > fileapi::FileSystemFileUtilProxy::Close(*context(), > That * looks like a bug. I guess it's correct since > bool FileSystemFileUtilProxy::Close( > const FileSystemOperationContext& context, ... The trybots error looked in patching because of conflict in webkit_fileapi.gypi.
I missed uploading newly added files. Do it again.
I've uploaded again.
On Thu, Mar 3, 2011 at 5:56 PM, <dmikurube@google.com> wrote: > On 2011/03/04 01:05:34, ericu wrote: >> >> The rest LGTM if you can get the trybots happier. > > > http://codereview.chromium.org/6604020/diff/4006/webkit/fileapi/file_system_f... >> >> File webkit/fileapi/file_system_file_util_proxy.cc (right): > > > http://codereview.chromium.org/6604020/diff/4006/webkit/fileapi/file_system_f... >> >> webkit/fileapi/file_system_file_util_proxy.cc:94: >> fileapi::FileSystemFileUtilProxy::Close(*context(), >> That * looks like a bug. > > I guess it's correct since >> >> bool FileSystemFileUtilProxy::Close( >> const FileSystemOperationContext& context, ... Ah, right. My mistake. LGTM > The trybots error looked in patching because of conflict in > webkit_fileapi.gypi. > > http://codereview.chromium.org/6604020/ >
lgtm http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.h:176: scoped_ptr<FileSystemOperationContext> file_system_operation_context_; Does this need to be scoped_ptr?
On Thu, Mar 3, 2011 at 7:15 PM, <kinuko@chromium.org> wrote: > lgtm > > > http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_operation.h (right): > > http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... > webkit/fileapi/file_system_operation.h:176: > scoped_ptr<FileSystemOperationContext> file_system_operation_context_; > Does this need to be scoped_ptr? Good point. Just a simple variable would work. > http://codereview.chromium.org/6604020/ >
On 2011/03/04 03:25:10, ericu wrote: > On Thu, Mar 3, 2011 at 7:15 PM, <mailto:kinuko@chromium.org> wrote: > > lgtm > > > > > > > http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... > > File webkit/fileapi/file_system_operation.h (right): > > > > > http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... > > webkit/fileapi/file_system_operation.h:176: > > scoped_ptr<FileSystemOperationContext> file_system_operation_context_; > > Does this need to be scoped_ptr? > > Good point. Just a simple variable would work. > > > http://codereview.chromium.org/6604020/ > > Agreed. Made it a simple variable.
On Thu, Mar 3, 2011 at 7:29 PM, <dmikurube@google.com> wrote: > On 2011/03/04 03:25:10, ericu wrote: >> >> On Thu, Mar 3, 2011 at 7:15 PM, <mailto:kinuko@chromium.org> wrote: >> > lgtm >> > >> > >> > > > http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... >> >> > File webkit/fileapi/file_system_operation.h (right): >> > >> > > > http://codereview.chromium.org/6604020/diff/16001/webkit/fileapi/file_system_... >> >> > webkit/fileapi/file_system_operation.h:176: >> > scoped_ptr<FileSystemOperationContext> file_system_operation_context_; >> > Does this need to be scoped_ptr? > >> Good point. Just a simple variable would work. > >> > http://codereview.chromium.org/6604020/ >> > > > Agreed. Made it a simple variable. > > http://codereview.chromium.org/6604020/ > Great! Fire when ready. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
