Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7433006:
Pepper quota support (Closed)
|
Created:
9 years, 5 months ago by kinuko Modified:
9 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, kinuko+watch, jam, ddorwin Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPepper quota support
Probably this could be better implemented using push notification model, but in this patch I made this very straightforwardly (with possibly minimal change). **NOTE: This design may not be really safe as quota accounting is done in renderer**
* Added QuotaFileIO class (in quota_file_io.{cc,h}) which performs pre- and post- write quota related operations.
* For Write/SetLength the QuotaFileIO instance first performs QueryFileInfo and QueryAvailableSpace to get the file size and current available space. If the results look good it dispatches actual Write/SetLength operations. After the operation has succeeded it then notifies the browser that it has made a storage modify operation.
* For WillWrite/WillSetLength this change assumes they are always followed by actual write/setlength operations and the operations will succeed.
Based on another cleanup patch:
http://codereview.chromium.org/7438001/
BUG=86556
TEST=manually tested
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94892
Patch Set 1 #
Total comments: 32
Patch Set 2 : updated #
Total comments: 15
Patch Set 3 : WillUpdate/DidUpdate model #Patch Set 4 : '' #
Total comments: 17
Patch Set 5 : Separated the quota code into quota_file_io #Patch Set 6 : fixed tests #
Total comments: 21
Patch Set 7 : '' #Patch Set 8 : fix #
Total comments: 10
Patch Set 9 : '' #
Total comments: 8
Patch Set 10 : fixed nits #Patch Set 11 : '' #Patch Set 12 : '' #Messages
Total messages: 30 (0 generated)
This is still rough, but I would like your early feedbacks. Thanks!
On 2011/07/19 12:12:36, kinuko wrote: > This is still rough, but I would like your early feedbacks. Thanks! Well on the second thought if we want to securely enforce/update the quota we might need to move more code (namely the main write operation part) from renderer to browser...? Wdyt?
I took a look at ppb_file_io_impl.{h,cc}, and here are the first round of
comments.
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i...
webkit/plugins/ppapi/ppb_file_io_impl.cc:200: int64 expected_growth = offset +
bytes_to_write - file_size_;
It is possible that this method gets called for multiple times before we get
replies via WriteCallback / DidCheckQuotaForWrite.
In that case, file_size_ is not correct, right?
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i...
webkit/plugins/ppapi/ppb_file_io_impl.cc:478: file_size_ =
pending_setlength_length_;
Even if it failed, we still set file_size_ to pending_setlength_length_?
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i...
webkit/plugins/ppapi/ppb_file_io_impl.cc:482: void
PPB_FileIO_Impl::DidCheckQuotaForWrite(bool success) {
This method and DidCheckQuotaForSetLength ignore |success| entirely. Is that
intended?
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i...
File webkit/plugins/ppapi/ppb_file_io_impl.h (right):
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i...
webkit/plugins/ppapi/ppb_file_io_impl.h:154: std::queue<PendingWrite>
pending_writes_;
|pending_writes_| and |pending_write_callbacks_| look confusing. It is better to
have more comments.
To me, |pending_write_callbacks_| can be removed. (We probably only need put the
offset information in |callbacks_|, right?)
http://codereview.chromium.org/7433006/diff/1/content/renderer/pepper_plugin_... File content/renderer/pepper_plugin_delegate_impl.h (right): http://codereview.chromium.org/7433006/diff/1/content/renderer/pepper_plugin_... content/renderer/pepper_plugin_delegate_impl.h:204: It's not clear why you added this. http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util_proxy.h (right): http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util_proxy.h:39: int64 /* file_size */ Can you document what will happen on failure? Like if the error code is not PLATFORM_FILE_OK, the size will be -1. http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util_proxy.h:52: // Creates or opens a file with the given flags. This also returns the This comment initially confused me since this function doesn't return the size. I think probably it's not necessary to mention the size, since the way you did the callback is pretty obvious what types of data you get. http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_oper... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.cc:43: } // anonymous namespace You don't need "anonymous" here, just "// namespace" http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.cc:739: if (rv == base::PLATFORM_FILE_OK) { This file doesn't usually use {} for single-line conditionals, so I wouldn't add them here.
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:200: int64 expected_growth = offset + bytes_to_write - file_size_; On 2011/07/19 17:41:55, yzshen1 wrote: > In that case, file_size_ is not correct, right? I have these concerns too, keeping track of filesize looks fragile. This is a usecase where having a cached 'space_available' value being updated by the backend would be really helpful. Stopping the write to ask the browser and them resuming the write is bumpy. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:231: DCHECK_EQ(-1, pending_setlength_length_); What guarantees are there that there's only one SetLength() pending at a time... just checking. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:477: RunAndRemoveFirstPendingCallback(PlatformFileErrorToPepperError(error_code)); Now that some of these operations call out the the browser process instead of just marshaling to the local file thread (via FileUtilProxy)... is it safe to assume that the FirstPendingCallback is the callback associated with whatever operation is being completed. Seems like that assumption depends on all operations being serialized and completing in the same order. True when everything is handled via a single hop to and return from the local file thread... but now that some operations go thru a different async code path that is not serialized with the FileUtilProxy operations, is this assumption busted. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.h (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:126: int64 file_size); Seems that this class mostly uses int64_t in place of int64 (not sure why), but for consistency this probably should too. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:154: std::queue<PendingWrite> pending_writes_; On 2011/07/19 17:41:55, yzshen1 wrote: > |pending_writes_| and |pending_write_callbacks_| look confusing Maybe... pending_quota_checks_ pending_writes_
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:477: RunAndRemoveFirstPendingCallback(PlatformFileErrorToPepperError(error_code)); What is more, the order of multiple write operations may be changed. (We only allow one pending SetLength at a time, so SetLength should be fine.) On 2011/07/19 22:01:45, michaeln wrote: > Now that some of these operations call out the the browser process instead of > just marshaling to the local file thread (via FileUtilProxy)... is it safe to > assume that the FirstPendingCallback is the callback associated with whatever > operation is being completed. > > Seems like that assumption depends on all operations being serialized and > completing in the same order. True when everything is handled via a single hop > to and return from the local file thread... but now that some operations go thru > a different async code path that is not serialized with the FileUtilProxy > operations, is this assumption busted.
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.h (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:34: class PPB_FileIO_Impl : public Resource, I'm kinda wondering if for the TEMP and PERSISTENT file_system_types_, this entire interface should be implemented in terms of our existing file reading and writing IPCs and ultimately handled in the browser process. Has that approach been considered? http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:64: virtual int32_t GetOSFileDescriptor() OVERRIDE; How is this used i wonder... will do some grepping? http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:68: virtual int32_t WillSetLength(int64_t length, What are WillWrite() and WillSetLength() about... deadcode?
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.h (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:68: virtual int32_t WillSetLength(int64_t length, The proxy gets the OS file descriptor to duplicate it into the other process. This is a trusted API only so can't be called by normal plugins. The intention of WillWrite and WillSetLength was that the proxy would use this to keep the quota management system appraised of file operations it's performing in the plugin process so we don't have to proxy every single file I/O operation.
Thanks for reviewing. I updated the code. http://codereview.chromium.org/7433006/diff/1/content/renderer/pepper_plugin_... File content/renderer/pepper_plugin_delegate_impl.h (right): http://codereview.chromium.org/7433006/diff/1/content/renderer/pepper_plugin_... content/renderer/pepper_plugin_delegate_impl.h:204: On 2011/07/19 20:29:31, brettw wrote: > It's not clear why you added this. Removed. http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_file... File webkit/fileapi/file_system_file_util_proxy.h (right): http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util_proxy.h:39: int64 /* file_size */ On 2011/07/19 20:29:31, brettw wrote: > Can you document what will happen on failure? Like if the error code is not > PLATFORM_FILE_OK, the size will be -1. Done. http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_file... webkit/fileapi/file_system_file_util_proxy.h:52: // Creates or opens a file with the given flags. This also returns the On 2011/07/19 20:29:31, brettw wrote: > This comment initially confused me since this function doesn't return the size. > I think probably it's not necessary to mention the size, since the way you did > the callback is pretty obvious what types of data you get. Removed. http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_oper... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.cc:43: } // anonymous namespace On 2011/07/19 20:29:31, brettw wrote: > You don't need "anonymous" here, just "// namespace" Done. http://codereview.chromium.org/7433006/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.cc:739: if (rv == base::PLATFORM_FILE_OK) { On 2011/07/19 20:29:31, brettw wrote: > This file doesn't usually use {} for single-line conditionals, so I wouldn't add > them here. Done. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:200: int64 expected_growth = offset + bytes_to_write - file_size_; On 2011/07/19 22:01:45, michaeln wrote: > On 2011/07/19 17:41:55, yzshen1 wrote: > > In that case, file_size_ is not correct, right? > > I have these concerns too, keeping track of filesize looks fragile. Putting aside the fact that the current design is not really great, I think this would be ok (if my assumptions are correct). By writes the file_size_ can only be extended but not be truncated, and every write comes with a proper offset-- therefore parallel writes won't screw up the file_size_. > This is a usecase where having a cached 'space_available' value being updated by > the backend would be really helpful. Stopping the write to ask the browser and > them resuming the write is bumpy. I updated the patch to do some caching. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:231: DCHECK_EQ(-1, pending_setlength_length_); On 2011/07/19 22:01:45, michaeln wrote: > What guarantees are there that there's only one SetLength() pending at a time... > just checking. Seems like EXCLUSIVE operations cannot run in parallel, if I understand correctly. (Otherwise I'll change this with queue) http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:477: RunAndRemoveFirstPendingCallback(PlatformFileErrorToPepperError(error_code)); On 2011/07/19 23:14:14, yzshen1 wrote: > What is more, the order of multiple write operations may be changed. > (We only allow one pending SetLength at a time, so SetLength should be fine.) > > On 2011/07/19 22:01:45, michaeln wrote: > > Now that some of these operations call out the the browser process instead of > > just marshaling to the local file thread (via FileUtilProxy)... is it safe to > > assume that the FirstPendingCallback is the callback associated with whatever > > operation is being completed. > > > > Seems like that assumption depends on all operations being serialized and > > completing in the same order. True when everything is handled via a single hop > > to and return from the local file thread... but now that some operations go > thru > > a different async code path that is not serialized with the FileUtilProxy > > operations, is this assumption busted. > I splitted the callbacks queue into two, one for regular calls and the other for the ones waiting a quota query callback. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:478: file_size_ = pending_setlength_length_; On 2011/07/19 17:41:55, yzshen1 wrote: > Even if it failed, we still set file_size_ to pending_setlength_length_? Good catch... fixed. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.h (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:34: class PPB_FileIO_Impl : public Resource, On 2011/07/19 23:54:58, michaeln wrote: > I'm kinda wondering if for the TEMP and PERSISTENT file_system_types_, this > entire interface should be implemented in terms of our existing file reading and > writing IPCs and ultimately handled in the browser process. > > Has that approach been considered? I suspected we may want to do that. Not sure if we need to re-implement the entire interface but at least as for the setlength/write part. (Btw which code do you mean by 'existing file reading IPC'?) http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:126: int64 file_size); On 2011/07/19 22:01:45, michaeln wrote: > Seems that this class mostly uses int64_t in place of int64 (not sure why), but > for consistency this probably should too. Done. http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.h:154: std::queue<PendingWrite> pending_writes_; On 2011/07/19 22:01:45, michaeln wrote: > On 2011/07/19 17:41:55, yzshen1 wrote: > > |pending_writes_| and |pending_write_callbacks_| look confusing > > Maybe... > pending_quota_checks_ > pending_writes_ Done.
> (Btw which code do you mean by 'existing file reading IPC'?) WebCore::FileReader uses WebCore::FileReaderLoader (an async reader) which currently uses the blob URL loading machinery. WebCore::FileReaderLoader uses WebCore::ThreadableLoader, which invokes WebKit::WebURLLoader, which invokes chrome's ResourceLoaderBridge which sends/receives IPCs.
http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... webkit/plugins/ppapi/ppb_file_io_impl.cc:231: DCHECK_EQ(-1, pending_setlength_length_); Yes, that is correct. On 2011/07/20 13:39:35, kinuko wrote: > On 2011/07/19 22:01:45, michaeln wrote: > > What guarantees are there that there's only one SetLength() pending at a > time... > > just checking. > > Seems like EXCLUSIVE operations cannot run in parallel, if I understand > correctly. (Otherwise I'll change this with queue) http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:219: } The problem is that write operations may be executed in wrong order. Consider the following example: Current file length: 5 Write_1: offset: 3; bytes_to_write: 10 Write_2: offset: 0; bytes_to_write: 5 Write_1 requires QueryAvailableSpace, while Write_2 doesn't. If Write_1 and Write_2 comes in quick succession, it is possible that Write_2 is executed before Write_1, which will result in unexpected file contents.
http://codereview.chromium.org/7433006/diff/15001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7433006/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.cc:846: is_write_operation_ = true; Seems like an odd place to be setting this flag, also i don't see where this flag is used? http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/plugin... File webkit/plugins/ppapi/plugin_delegate.h (right): http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/plugin... webkit/plugins/ppapi/plugin_delegate.h:300: int64_t /* file_size */>::Type AsyncOpenFileCallback; How is this size used? There's no guarantee that it's accurate assuming that others may be writing/truncating it. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/plugin... webkit/plugins/ppapi/plugin_delegate.h:334: virtual bool QueryAvailableSpace( Does the return value mean anything? I don't see the callsites checking it, maybe there shouldn't be a return value. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:5: #include "webkit/plugins/ppapi/ppb_file_io_impl.h" On 2011/07/20 04:08:06, brettw wrote: > http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... > File webkit/plugins/ppapi/ppb_file_io_impl.h (right): > > http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... > webkit/plugins/ppapi/ppb_file_io_impl.h:68: virtual int32_t > WillSetLength(int64_t length, > The proxy gets the OS file descriptor to duplicate it into the other process. > This is a trusted API only so can't be called by normal plugins. > > The intention of WillWrite and WillSetLength was that the proxy would use this > to keep the quota management system appraised of file operations it's performing > in the plugin process so we don't have to proxy every single file I/O operation. Are any plugins using the file descriptor base APIs yet? On 2011/07/20 17:54:52, michaeln wrote: > > (Btw which code do you mean by 'existing file reading IPC'?) > > WebCore::FileReader uses WebCore::FileReaderLoader (an async reader) which > currently uses the blob URL loading machinery. WebCore::FileReaderLoader uses > WebCore::ThreadableLoader, which invokes WebKit::WebURLLoader, which invokes > chrome's ResourceLoaderBridge which sends/receives IPCs. Looks like the pepper glueware has a URLLoader capability defined in url_loader.h and implemented in ppb_url_loader_impl.h which invokes WebURLLoader. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:294: // Request quota. Given the shape of the trusted APIs (there's a WillDoSomething() but no DidDoSomething()), should this method (and the one below) check space available and also notify storage modified. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:496: pending_setlength_length_ = -1; If the caller's callback has invoked FileIO.SetLength(), i think this line will squash the value provided to that call. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:539: return; What about other pending checks in the queue? Is it ok to return here or should the others be processed too so the FileIO doesn't hang.
http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/plugin... File webkit/plugins/ppapi/plugin_delegate.h (right): http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/plugin... webkit/plugins/ppapi/plugin_delegate.h:300: int64_t /* file_size */>::Type AsyncOpenFileCallback; On 2011/07/20 21:28:33, michaeln wrote: > How is this size used? There's no guarantee that it's accurate assuming that > others may be writing/truncating it. No, but I don't think we can strictly guarantee accurate counting in the current model. For WillWrite/WillSetLength it's almost impossible to perform GetFileSize and Write/Truncate in a single task, and for untrusted writes by API we still cannot avoid a stale size situation unless we flush after each Write call, since we keep file descriptors opened for FileIO. Anyway I dropped this size parameter from the OpenFile callback and modified the code to perform GetFileInfo and Write as closely as possible. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/plugin... webkit/plugins/ppapi/plugin_delegate.h:334: virtual bool QueryAvailableSpace( On 2011/07/20 21:28:33, michaeln wrote: > Does the return value mean anything? I don't see the callsites checking it, > maybe there shouldn't be a return value. Done. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:5: #include "webkit/plugins/ppapi/ppb_file_io_impl.h" On 2011/07/20 21:28:33, michaeln wrote: > On 2011/07/20 04:08:06, brettw wrote: > > > http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... > > File webkit/plugins/ppapi/ppb_file_io_impl.h (right): > > > > > http://codereview.chromium.org/7433006/diff/1/webkit/plugins/ppapi/ppb_file_i... > > webkit/plugins/ppapi/ppb_file_io_impl.h:68: virtual int32_t > > WillSetLength(int64_t length, > > The proxy gets the OS file descriptor to duplicate it into the other process. > > This is a trusted API only so can't be called by normal plugins. > > > > The intention of WillWrite and WillSetLength was that the proxy would use this > > to keep the quota management system appraised of file operations it's > performing > > in the plugin process so we don't have to proxy every single file I/O > operation. > > Are any plugins using the file descriptor base APIs yet? > > > On 2011/07/20 17:54:52, michaeln wrote: > > > (Btw which code do you mean by 'existing file reading IPC'?) > > > > WebCore::FileReader uses WebCore::FileReaderLoader (an async reader) which > > currently uses the blob URL loading machinery. WebCore::FileReaderLoader uses > > WebCore::ThreadableLoader, which invokes WebKit::WebURLLoader, which invokes > > chrome's ResourceLoaderBridge which sends/receives IPCs. I guessed you meant this but also wished there might be an easier path :) > Looks like the pepper glueware has a URLLoader capability defined in > url_loader.h and implemented in ppb_url_loader_impl.h which invokes > WebURLLoader. Sounds good-- but could we make it a TODO while we're targeting M14? Also even if we move the major code to the browser don't we still need to do some tricky stuff for trusted apps? http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:219: RegisterCallback(&pending_quota_check_callbacks_, On 2011/07/20 20:50:36, yzshen1 wrote: > The problem is that write operations may be executed in wrong order. > Consider the following example: > Current file length: 5 > Write_1: offset: 3; bytes_to_write: 10 > Write_2: offset: 0; bytes_to_write: 5 > > Write_1 requires QueryAvailableSpace, while Write_2 doesn't. > If Write_1 and Write_2 comes in quick succession, it is possible that Write_2 is > executed before Write_1, which will result in unexpected file contents. > I changed the code to either always or never queue the operations. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:294: // Request quota. On 2011/07/20 21:28:33, michaeln wrote: > Given the shape of the trusted APIs (there's a WillDoSomething() but no > DidDoSomething()), should this method (and the one below) check space available > and also notify storage modified. Hmm the successive write or truncate may fail, so this model may not work really well. I made a tentative change that performs something like that. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:496: pending_setlength_length_ = -1; On 2011/07/20 21:28:33, michaeln wrote: > If the caller's callback has invoked FileIO.SetLength(), i think this line will > squash the value provided to that call. Good catch, fixed. http://codereview.chromium.org/7433006/diff/15001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:539: return; On 2011/07/20 21:28:33, michaeln wrote: > What about other pending checks in the queue? Is it ok to return here or should > the others be processed too so the FileIO doesn't hang. Done.
Here are some more comments for ppb_file_io_impl.{h,cc}.
One issue is that this CL cannot accurately control quota usage. I am not
familiar with how the quota system is designed, so I don't know whether this is
okay.
If we want to make sure we never exceed the quota, the correct way is to reserve
space instead of querying for available space:
- the PPB_FileIO_Impl object keeps track of file_size_ and reserved_space_. (And
if we assume that the file can only be opened with write permission by one
PPB_FileIO_Impl at a time, then we don't need to query its size from the system
over and over.)
- we don't start Write / SetLength if target size > file_size_ +
reserved_space_, instead, we send request to ask for more space first.
- we could return unused space later, when the object goes away/partial
write/set length to a smaller size.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:51:
PPB_FileIO_Impl::PendingOperation::PendingOperation()
Please use the same order as they are declared in the .h file.
This method should come after CallbackEntry::*.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:52: : type(UNKNOWN), offset(0),
buffer(NULL), bytes_to_write(0) {}
You miss |length|.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:72: cached_available_space_(0),
You miss quite a few members. Please initialize all those which don't have a
constructor.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:375: if (callbacks->empty())
pending_op_ can be set incorrectly.
Consider the following example: there are two writes in the callbacks_ queue,
while the third in pending_quota_check_callbacks_. And now we call
RunAndRemoveFirstPendingCallback() with the pending_quota_check_callbacks_
queue. pending_op_ will be set to NONE.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:531: pending_quota_checks_.pop();
Maybe you could push at the end of the method, so that you don't need to
remember to pop on every error handling path.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:614:
WriteCallback(base::PLATFORM_FILE_OK, op.bytes_to_write);
You shouldn't call WriteCallback before moving the callback from
|pending_quota_check_callbacks_| into |callbacks_|.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:626:
SetLengthCallback(base::PLATFORM_FILE_OK);
You shouldn't call SetLengthCallback before moving the callback from
|pending_quota_check_callbacks_| into |callbacks_|.
Please do tests to ensure that the CL works properly.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_io_impl.h (right):
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.h:96: struct PendingOperation {
It seems we have two kinds of "pending" now.
- called base::FileUtilProxy::* but haven't got replies.
(That is what pending means in OperationType.)
- started quota check but haven't called base::FileUtilProxy::*.
(That is what pending means here.)
Please think up better names and/or add comments to clarify.
Otherwise, OperationType and PendingOperation/PendingOperation::Type seem very
confusing.
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.h:165: // Valid only while there're
pending write requests.
I think SetLength/Will* also use this, right?
(PS: As I mentioned above, we need to clarify what pending means here.)
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.h:173: std::queue<CallbackEntry>
pending_quota_check_callbacks_;
I would suggest that you don't keep another CallbackEntry queue. Instead, you
keep a scoped_refptr<TrackedCompletionCallback> in PendingOperation.
When DidQueryFileSizeAndQuota is called, you move this callback into callbacks_
(or run it directly on error).
In that way, you don't need two CallbackEntry queues, which is confusing.
And you need to be careful about setting |pending_op_| correctly. (The current
CL does it incorrectly. Please see my comment in CommonCallValidation.)
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.h:176: std::queue<int64_t>
pending_write_offset_;
I think this should be part of CallbackEntry, right? The original code doesn't
keep a separate queue for |read_buffer|.
If you concern about putting too many members (which are only used by one kind
of operations) into CallbackEntry, we could probably find another way to do it.
But at least we should be consistent.
On 2011/07/21 22:23:34, yzshen1 wrote:
> Here are some more comments for ppb_file_io_impl.{h,cc}.
>
> One issue is that this CL cannot accurately control quota usage. I am not
> familiar with how the quota system is designed, so I don't know whether this
is
> okay.
Yes, without reservation we could exceed the quota (esp while there are multiple
concurrent write operations), but for now it's how it works in the current
design.
Actually reservation was one of my first design choice, but after some lengthy
internal discussion in storage team we decided to go with simpler relaxed design
at least for the initial revisions.
> If we want to make sure we never exceed the quota, the correct way is to
reserve
> space instead of querying for available space:
> - the PPB_FileIO_Impl object keeps track of file_size_ and reserved_space_.
(And
> if we assume that the file can only be opened with write permission by one
> PPB_FileIO_Impl at a time, then we don't need to query its size from the
system
> over and over.)
> - we don't start Write / SetLength if target size > file_size_ +
> reserved_space_, instead, we send request to ask for more space first.
> - we could return unused space later, when the object goes away/partial
> write/set length to a smaller size.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:51:
> PPB_FileIO_Impl::PendingOperation::PendingOperation()
> Please use the same order as they are declared in the .h file.
>
> This method should come after CallbackEntry::*.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:52: : type(UNKNOWN), offset(0),
> buffer(NULL), bytes_to_write(0) {}
> You miss |length|.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:72: cached_available_space_(0),
> You miss quite a few members. Please initialize all those which don't have a
> constructor.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:375: if (callbacks->empty())
> pending_op_ can be set incorrectly.
> Consider the following example: there are two writes in the callbacks_ queue,
> while the third in pending_quota_check_callbacks_. And now we call
> RunAndRemoveFirstPendingCallback() with the pending_quota_check_callbacks_
> queue. pending_op_ will be set to NONE.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:531: pending_quota_checks_.pop();
> Maybe you could push at the end of the method, so that you don't need to
> remember to pop on every error handling path.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:614:
> WriteCallback(base::PLATFORM_FILE_OK, op.bytes_to_write);
> You shouldn't call WriteCallback before moving the callback from
> |pending_quota_check_callbacks_| into |callbacks_|.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:626:
> SetLengthCallback(base::PLATFORM_FILE_OK);
> You shouldn't call SetLengthCallback before moving the callback from
> |pending_quota_check_callbacks_| into |callbacks_|.
>
> Please do tests to ensure that the CL works properly.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> File webkit/plugins/ppapi/ppb_file_io_impl.h (right):
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:96: struct PendingOperation {
> It seems we have two kinds of "pending" now.
> - called base::FileUtilProxy::* but haven't got replies.
> (That is what pending means in OperationType.)
> - started quota check but haven't called base::FileUtilProxy::*.
> (That is what pending means here.)
>
> Please think up better names and/or add comments to clarify.
>
> Otherwise, OperationType and PendingOperation/PendingOperation::Type seem very
> confusing.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:165: // Valid only while there're
> pending write requests.
> I think SetLength/Will* also use this, right?
>
> (PS: As I mentioned above, we need to clarify what pending means here.)
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:173: std::queue<CallbackEntry>
> pending_quota_check_callbacks_;
> I would suggest that you don't keep another CallbackEntry queue. Instead, you
> keep a scoped_refptr<TrackedCompletionCallback> in PendingOperation.
> When DidQueryFileSizeAndQuota is called, you move this callback into
callbacks_
> (or run it directly on error).
>
> In that way, you don't need two CallbackEntry queues, which is confusing.
>
> And you need to be careful about setting |pending_op_| correctly. (The current
> CL does it incorrectly. Please see my comment in CommonCallValidation.)
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:176: std::queue<int64_t>
> pending_write_offset_;
> I think this should be part of CallbackEntry, right? The original code doesn't
> keep a separate queue for |read_buffer|.
>
> If you concern about putting too many members (which are only used by one kind
> of operations) into CallbackEntry, we could probably find another way to do
it.
> But at least we should be consistent.
On 2011/07/21 22:23:34, yzshen1 wrote:
> Here are some more comments for ppb_file_io_impl.{h,cc}.
>
> One issue is that this CL cannot accurately control quota usage. I am not
> familiar with how the quota system is designed, so I don't know whether this
is
> okay.
Yes, without reservation we could exceed the quota (esp while there are multiple
concurrent write operations), but for now it's how it works in the current
design.
Actually reservation was one of my first design choice, but after some lengthy
internal discussion in storage team we decided to go with simpler relaxed design
at least for the initial revisions.
> If we want to make sure we never exceed the quota, the correct way is to
reserve
> space instead of querying for available space:
> - the PPB_FileIO_Impl object keeps track of file_size_ and reserved_space_.
(And
> if we assume that the file can only be opened with write permission by one
> PPB_FileIO_Impl at a time, then we don't need to query its size from the
system
> over and over.)
> - we don't start Write / SetLength if target size > file_size_ +
> reserved_space_, instead, we send request to ask for more space first.
> - we could return unused space later, when the object goes away/partial
> write/set length to a smaller size.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:51:
> PPB_FileIO_Impl::PendingOperation::PendingOperation()
> Please use the same order as they are declared in the .h file.
>
> This method should come after CallbackEntry::*.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:52: : type(UNKNOWN), offset(0),
> buffer(NULL), bytes_to_write(0) {}
> You miss |length|.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:72: cached_available_space_(0),
> You miss quite a few members. Please initialize all those which don't have a
> constructor.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:375: if (callbacks->empty())
> pending_op_ can be set incorrectly.
> Consider the following example: there are two writes in the callbacks_ queue,
> while the third in pending_quota_check_callbacks_. And now we call
> RunAndRemoveFirstPendingCallback() with the pending_quota_check_callbacks_
> queue. pending_op_ will be set to NONE.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:531: pending_quota_checks_.pop();
> Maybe you could push at the end of the method, so that you don't need to
> remember to pop on every error handling path.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:614:
> WriteCallback(base::PLATFORM_FILE_OK, op.bytes_to_write);
> You shouldn't call WriteCallback before moving the callback from
> |pending_quota_check_callbacks_| into |callbacks_|.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.cc:626:
> SetLengthCallback(base::PLATFORM_FILE_OK);
> You shouldn't call SetLengthCallback before moving the callback from
> |pending_quota_check_callbacks_| into |callbacks_|.
>
> Please do tests to ensure that the CL works properly.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> File webkit/plugins/ppapi/ppb_file_io_impl.h (right):
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:96: struct PendingOperation {
> It seems we have two kinds of "pending" now.
> - called base::FileUtilProxy::* but haven't got replies.
> (That is what pending means in OperationType.)
> - started quota check but haven't called base::FileUtilProxy::*.
> (That is what pending means here.)
>
> Please think up better names and/or add comments to clarify.
>
> Otherwise, OperationType and PendingOperation/PendingOperation::Type seem very
> confusing.
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:165: // Valid only while there're
> pending write requests.
> I think SetLength/Will* also use this, right?
>
> (PS: As I mentioned above, we need to clarify what pending means here.)
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:173: std::queue<CallbackEntry>
> pending_quota_check_callbacks_;
> I would suggest that you don't keep another CallbackEntry queue. Instead, you
> keep a scoped_refptr<TrackedCompletionCallback> in PendingOperation.
> When DidQueryFileSizeAndQuota is called, you move this callback into
callbacks_
> (or run it directly on error).
>
> In that way, you don't need two CallbackEntry queues, which is confusing.
>
> And you need to be careful about setting |pending_op_| correctly. (The current
> CL does it incorrectly. Please see my comment in CommonCallValidation.)
>
>
http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi...
> webkit/plugins/ppapi/ppb_file_io_impl.h:176: std::queue<int64_t>
> pending_write_offset_;
> I think this should be part of CallbackEntry, right? The original code doesn't
> keep a separate queue for |read_buffer|.
>
> If you concern about putting too many members (which are only used by one kind
> of operations) into CallbackEntry, we could probably find another way to do
it.
> But at least we should be consistent.
I only looked briefly at this patch, so sorry if my suggestion is late or doesn't make sense... It seems like we have a lot of delicate logic in ppb_file_io_impl.cc. This stuff is security sensitive (in some respect) so I'm extra worried about it. I wonder if it makes sense to separate out the quota logic into an extra class with a clear simple API, and have this be a member of the FileIO_Impl class. Then we can have unit tests for the quota management and different cases/sequences of events. As-is the FileIO_Impl is kind of hard to have low-level tests for since it depends on lots of other parts of the system. What do you think? Is there a better or easier way to make sure this important code is well-tested?
On 2011/07/22 18:30:25, brettw wrote:
> I only looked briefly at this patch, so sorry if my suggestion is late or
> doesn't make sense...
>
> It seems like we have a lot of delicate logic in ppb_file_io_impl.cc. This
stuff
> is security sensitive (in some respect) so I'm extra worried about it.
>
> I wonder if it makes sense to separate out the quota logic into an extra class
> with a clear simple API, and have this be a member of the FileIO_Impl class.
>
> Then we can have unit tests for the quota management and different
> cases/sequences of events. As-is the FileIO_Impl is kind of hard to have
> low-level tests for since it depends on lots of other parts of the system.
It totally makes sense. Writing such a code without a test is scary.
I separated out the quota-related code into a separate class (in
quota_file_io.{cc,h}) and added unit_tests for it. The entire changelist has
grown a bit but the changes in FileIO_Impl has gotten really small. In my
local env the tests look ok (valgrind looks ok too). Could you PTAL? Thanks,
> One issue is that this CL cannot accurately control quota usage. I am not > familiar with how the quota system is designed, so I don't know whether this is > okay. Right, without reservation we could exceed the quota (esp while there are multiple concurrent write operations), but for now it's how it works (and how it's supposed to work) in the current design. Back then we had discussed whether we should adopt reservation/lease or some similar concept internally, but after a lengthy discussion we chose to implement a simpler one at least for the initial version. http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:531: pending_quota_checks_.pop(); On 2011/07/21 22:23:34, yzshen1 wrote: > Maybe you could push at the end of the method, so that you don't need to > remember to pop on every error handling path. Done (in quota_file_io.cc) http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:626: SetLengthCallback(base::PLATFORM_FILE_OK); On 2011/07/21 22:23:34, yzshen1 wrote: > You shouldn't call SetLengthCallback before moving the callback from > |pending_quota_check_callbacks_| into |callbacks_|. > > Please do tests to ensure that the CL works properly. Added tests that call WillWrite/WillSetLength in ppapi/tests/test_file_io.cc and verified it works (on my local env). As for quota related operations I've also added new unittest as quota_file_io_unittest. http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... File webkit/plugins/ppapi/ppb_file_io_impl.h (right): http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.h:96: struct PendingOperation { On 2011/07/21 22:23:34, yzshen1 wrote: > It seems we have two kinds of "pending" now. > - called base::FileUtilProxy::* but haven't got replies. > (That is what pending means in OperationType.) > - started quota check but haven't called base::FileUtilProxy::*. > (That is what pending means here.) > > Please think up better names and/or add comments to clarify. > > Otherwise, OperationType and PendingOperation/PendingOperation::Type seem very > confusing. I've moved all the quota-related 'pending' bookkeeping info to the new class QuotaFileIO, therefore in FileIOImpl we only have single type of 'pending' operations as ever. (QuotaFileIO keeps its own 'pending' operations but it has the almost same meaning as the PPB_FileIOImpl's 'pending', i.e. operations that are being dispatched or waiting for quota checks are referred as 'pending'.) http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.h:165: // Valid only while there're pending write requests. On 2011/07/21 22:23:34, yzshen1 wrote: > I think SetLength/Will* also use this, right? > > (PS: As I mentioned above, we need to clarify what pending means here.) Fixed the comment (in quota_file_io.h) http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.h:173: std::queue<CallbackEntry> pending_quota_check_callbacks_; On 2011/07/21 22:23:34, yzshen1 wrote: > I would suggest that you don't keep another CallbackEntry queue. Instead, you > keep a scoped_refptr<TrackedCompletionCallback> in PendingOperation. > When DidQueryFileSizeAndQuota is called, you move this callback into callbacks_ > (or run it directly on error). > > In that way, you don't need two CallbackEntry queues, which is confusing. > > And you need to be careful about setting |pending_op_| correctly. (The current > CL does it incorrectly. Please see my comment in CommonCallValidation.) In the new QuotaFileIO class I organized the code/callbacks like that; now we have single queue of pending operations and each operation keeps/maintains its associated callback(s)/parameters encapsulated in it. http://codereview.chromium.org/7433006/diff/24003/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.h:176: std::queue<int64_t> pending_write_offset_; On 2011/07/21 22:23:34, yzshen1 wrote: > I think this should be part of CallbackEntry, right? The original code doesn't > keep a separate queue for |read_buffer|. > > If you concern about putting too many members (which are only used by one kind > of operations) into CallbackEntry, we could probably find another way to do it. > But at least we should be consistent. Similarly they now belong to each (QuotaFileIO's) pending operation instance.
Thanks, I think this new design is easier to follow! I'll give an LGTM for content/ and I assume Michael and Yuzhu will check all the details of the quota management. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi... File webkit/plugins/ppapi/ppb_file_io_impl.h (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.h:141: scoped_ptr<QuotaFileIO> quota_file_io_; It looks like you use the existence of this pointer to mean that quota is being enforced, and if it doesn't exist, it means there's no quota. We should be sure to have a clear comment documenting this policy here. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:244: return false; Can you add a comment here that the op_ptr destructor will handle issuing the callback with an error? I was initially confused about this.
LGTM, the separate class is soooo much nicer and easier to read! http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:67: buffer_.reset(new char[bytes_to_write]); Do we need the copy the bytes? If the caller is required to keep the ptr valid until the callback is invoked, is this copy really needed? I see that PPB_FileIO_Impl does not may a copy.
Reviewed ppb_file_io_impl.{h,cc} and quota_file_io.{h,cc}.
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:250: int32_t rv =
CommonCallValidation(true, OPERATION_WRITE, callback);
Using OPERATION_WRITE means it can be queued up in |callbacks_| along with
PPB_FileIO_Impl::Write.
That requires all WillWrite / Write operations to be completed in the same order
as they are pushed into |callbacks_|. Otherwise, we will call the wrong
callbacks!
However, after reading the implementation of quota_file_io, it seems that cannot
be guaranteed.
One possible, simple solution is to make WillWrite OPERATION_EXCLUSIVE. I don't
think we explicitly say that WillWrite can be queued up in the API definition.
What do you think?
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:262: RegisterCallback(OPERATION_WRITE,
callback, NULL);
Please see comments on line 250.
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_...
File webkit/plugins/ppapi/quota_file_io.cc (right):
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_...
webkit/plugins/ppapi/quota_file_io.cc:72: virtual ~WriteOperation() OVERRIDE {
I think it is not OVERRIDE, is it?
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_...
webkit/plugins/ppapi/quota_file_io.cc:78: if
(quota_io_->CheckIfExceedsQuota(offset_ + bytes_to_write_)) {
In addition to the problem that I mentioned in another file about mixing
WillWrite and Write, it seems we have the same problem with a set of writes:
Consider two write ops: write_1 and write_2.
The callbacks from the user of PPB_FileIO_Impl are queued up in |callbacks_|: <
write_1_callback, write_2_callback >.
write_1 passes the check on line 78, so it goes to line 88.
However, write_2 fails to pass the check and goes to DidFail.
In this case, it is possible that write_2 is completed first, and calls the
first callback in |callbacks_|, that is, write_1_callback.
The key is, PPB_FileIO_Impl expects that the order of write completion is
exactly the same as the order it calls QuotaFileIO::Write. You need to either
change PPB_FileIO_Impl to not rely on that assumption or fix QuotaFileIO.
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_...
webkit/plugins/ppapi/quota_file_io.cc:129: virtual ~SetLengthOperation()
OVERRIDE {
I think it is not OVERRIDE, is it?
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_...
File webkit/plugins/ppapi/quota_file_io.h (right):
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_...
webkit/plugins/ppapi/quota_file_io.h:34: virtual ~QuotaFileIO();
virtual is not necessary.
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:78: if (quota_io_->CheckIfExceedsQuota(offset_ + bytes_to_write_)) { On 2011/07/26 19:26:44, yzshen1 wrote: > In addition to the problem that I mentioned in another file about mixing > WillWrite and Write, it seems we have the same problem with a set of writes: > > Consider two write ops: write_1 and write_2. > The callbacks from the user of PPB_FileIO_Impl are queued up in |callbacks_|: < > write_1_callback, write_2_callback >. > > write_1 passes the check on line 78, so it goes to line 88. > However, write_2 fails to pass the check and goes to DidFail. > > In this case, it is possible that write_2 is completed first, and calls the > first callback in |callbacks_|, that is, write_1_callback. > > The key is, PPB_FileIO_Impl expects that the order of write completion is > exactly the same as the order it calls QuotaFileIO::Write. You need to either > change PPB_FileIO_Impl to not rely on that assumption or fix QuotaFileIO. Doh... right... my vote would be to make QuotaFileIO invoke the callbacks in order so PPB_FileIO remains the same. This looks good besides the order in which the callbacks are invoked. This class could defer calling the callbacks until the 'batch' of pending ops initiated in DidQueryForQuotaCheck all completed, and the call them in order.
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi... File webkit/plugins/ppapi/ppb_file_io_impl.cc (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:250: int32_t rv = CommonCallValidation(true, OPERATION_WRITE, callback); On 2011/07/26 19:26:44, yzshen1 wrote: > Using OPERATION_WRITE means it can be queued up in |callbacks_| along with > PPB_FileIO_Impl::Write. > That requires all WillWrite / Write operations to be completed in the same order > as they are pushed into |callbacks_|. Otherwise, we will call the wrong > callbacks! > > However, after reading the implementation of quota_file_io, it seems that cannot > be guaranteed. > > One possible, simple solution is to make WillWrite OPERATION_EXCLUSIVE. I don't > think we explicitly say that WillWrite can be queued up in the API definition. > What do you think? Guessing how the API is supposed to be used, making this OPERATION_EXCLUSIVE sounds right to me. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.cc:262: RegisterCallback(OPERATION_WRITE, callback, NULL); On 2011/07/26 19:26:44, yzshen1 wrote: > Please see comments on line 250. Done. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi... File webkit/plugins/ppapi/ppb_file_io_impl.h (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/ppb_fi... webkit/plugins/ppapi/ppb_file_io_impl.h:141: scoped_ptr<QuotaFileIO> quota_file_io_; On 2011/07/26 16:40:14, brettw wrote: > It looks like you use the existence of this pointer to mean that quota is being > enforced, and if it doesn't exist, it means there's no quota. We should be sure > to have a clear comment documenting this policy here. Done. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:67: buffer_.reset(new char[bytes_to_write]); On 2011/07/26 19:25:08, michaeln wrote: > Do we need the copy the bytes? If the caller is required to keep the ptr valid > until the callback is invoked, is this copy really needed? I see that > PPB_FileIO_Impl does not may a copy. Right. I wanted to keep the pointers to make my tests run, but if that's the spec I should rather change the test code... http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:72: virtual ~WriteOperation() OVERRIDE { On 2011/07/26 19:26:44, yzshen1 wrote: > I think it is not OVERRIDE, is it? Done. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:78: if (quota_io_->CheckIfExceedsQuota(offset_ + bytes_to_write_)) { On 2011/07/26 19:58:07, michaeln wrote: > On 2011/07/26 19:26:44, yzshen1 wrote: > > In addition to the problem that I mentioned in another file about mixing > > WillWrite and Write, it seems we have the same problem with a set of writes: > > > > Consider two write ops: write_1 and write_2. > > The callbacks from the user of PPB_FileIO_Impl are queued up in |callbacks_|: > < > > write_1_callback, write_2_callback >. > > > > write_1 passes the check on line 78, so it goes to line 88. > > However, write_2 fails to pass the check and goes to DidFail. > > > > In this case, it is possible that write_2 is completed first, and calls the > > first callback in |callbacks_|, that is, write_1_callback. > > > > The key is, PPB_FileIO_Impl expects that the order of write completion is > > exactly the same as the order it calls QuotaFileIO::Write. You need to either > > change PPB_FileIO_Impl to not rely on that assumption or fix QuotaFileIO. > > Doh... right... my vote would be to make QuotaFileIO invoke the callbacks in > order so PPB_FileIO remains the same. This looks good besides the order in which > the callbacks are invoked. This class could defer calling the callbacks until > the 'batch' of pending ops initiated in DidQueryForQuotaCheck all completed, and > the call them in order. Done. Thanks for your careful review. Changed the code to have another queue for pending callbacks. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:129: virtual ~SetLengthOperation() OVERRIDE { On 2011/07/26 19:26:44, yzshen1 wrote: > I think it is not OVERRIDE, is it? Done. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:244: return false; On 2011/07/26 16:40:14, brettw wrote: > Can you add a comment here that the op_ptr destructor will handle issuing the > callback with an error? I was initially confused about this. Changed to explicitly call DidFail. http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.h (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:34: virtual ~QuotaFileIO(); On 2011/07/26 19:26:44, yzshen1 wrote: > virtual is not necessary. Done.
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:67: buffer_.reset(new char[bytes_to_write]); The reason why PPB_FileIO_Impl doesn't need to make a copy is that FileUtilProxy::Write makes a copy. I am not sure whether it is okay to assume that the buffer will stay valid until the callback is invoked. The API definition doesn't mention it. I agree that it looks bad to copy the buffer (especially later FileUtilProxy::Write will do it one more time), but please double check that the assumption (buffer stays valid until callback returns) is consistent with the overall API convention. On 2011/07/27 09:33:14, kinuko wrote: > On 2011/07/26 19:25:08, michaeln wrote: > > Do we need the copy the bytes? If the caller is required to keep the ptr valid > > until the callback is invoked, is this copy really needed? I see that > > PPB_FileIO_Impl does not may a copy. > > Right. I wanted to keep the pointers to make my tests run, but if that's the > spec I should rather change the test code... http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:310: did_notify_will_update_ = false; This seems bad to me: we only fire callbacks when we don't have any pending operations. Have you considered the following approaches? 1) change PPB_FileIO_Impl so that it won't expect the callbacks from QuotaFileIO::Write are in order. (In that case, it cannot easily RunAndRemoveFirstPendingCallback anymore.) Or 2) never take any shortcut when QuotaFileIO processes writes. For example, even if we fails the quota check, we takes a round trip to the file thread and back, so that we make sure the callbacks to PPB_FileIO_Impl are kept in order. In my personal opinion, option (1) sounds better. What do you think? (I am fine if you would like to make the improvement in a separate change later. In that case, make a TODO item.) http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.h (right): http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:36: bool Write(int64_t offset, 1) Please comment that if returns false the callback won't be run. 2) It seems the all pending callbacks won't be run when this object is deleted, right? Please also comment that. 3) If you want to keep the promise that all WriteCallbacks will be run in the same order as calls to QuotaFileIO::Write, please also comment it. http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:71: std::deque<PendingOperationBase*> pending_operations_; All you use are *queue* operations. Why to change it to deque? http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:74: std::deque<PendingOperationBase*> pending_callbacks_; It seems to me that you don't need two queues: you always keep them in |pending_operations_|, and only when inflight_operations_ == 0, you run all callbacks in order and remove all entries in that queue. Am I missing anything?
http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/35001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:67: buffer_.reset(new char[bytes_to_write]); On 2011/07/27 18:26:07, yzshen1 wrote: > The reason why PPB_FileIO_Impl doesn't need to make a copy is that > FileUtilProxy::Write makes a copy. > > I am not sure whether it is okay to assume that the buffer will stay valid until > the callback is invoked. The API definition doesn't mention it. > > I agree that it looks bad to copy the buffer (especially later > FileUtilProxy::Write will do it one more time), but please double check that the > assumption (buffer stays valid until callback returns) is consistent with the > overall API convention. > > On 2011/07/27 09:33:14, kinuko wrote: > > On 2011/07/26 19:25:08, michaeln wrote: > > > Do we need the copy the bytes? If the caller is required to keep the ptr > valid > > > until the callback is invoked, is this copy really needed? I see that > > > PPB_FileIO_Impl does not may a copy. Ok, let me revert this change for now and keep a copy of the buffer until/unless we can be sure not having copy is ok. (I wasn't able to find out the particular notion about that in the api header document) I put a TODO comment too. > > Right. I wanted to keep the pointers to make my tests run, but if that's the > > spec I should rather change the test code... > http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:310: did_notify_will_update_ = false; On 2011/07/27 18:26:08, yzshen1 wrote: > This seems bad to me: we only fire callbacks when we don't have any pending > operations. > Have you considered the following approaches? > 1) change PPB_FileIO_Impl so that it won't expect the callbacks from > QuotaFileIO::Write are in order. (In that case, it cannot easily > RunAndRemoveFirstPendingCallback anymore.) Or > 2) never take any shortcut when QuotaFileIO processes writes. For example, even > if we fails the quota check, we takes a round trip to the file thread and back, > so that we make sure the callbacks to PPB_FileIO_Impl are kept in order. > > In my personal opinion, option (1) sounds better. What do you think? > > (I am fine if you would like to make the improvement in a separate change later. > In that case, make a TODO item.) Yes 1) sounds better to me too and I had wondered if I should do that, but I didn't want to make bigger changes in file_io_impl.cc in this patch. I revised DidWrite to try dispatching callbacks in order whenever possible. Hope it looks better now. http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.h (right): http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:36: bool Write(int64_t offset, On 2011/07/27 18:26:08, yzshen1 wrote: > 1) Please comment that if returns false the callback won't be run. > 2) It seems the all pending callbacks won't be run when this object is deleted, > right? Please also comment that. > 3) If you want to keep the promise that all WriteCallbacks will be run in the > same order as calls to QuotaFileIO::Write, please also comment it. Done. http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:71: std::deque<PendingOperationBase*> pending_operations_; On 2011/07/27 18:26:08, yzshen1 wrote: > All you use are *queue* operations. Why to change it to deque? I wanted to use methods like size(), begin() and end() which are not supported by std:queue. Also in the new code I use iterator which queue doesn't support either. http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:74: std::deque<PendingOperationBase*> pending_callbacks_; On 2011/07/27 18:26:08, yzshen1 wrote: > It seems to me that you don't need two queues: > you always keep them in |pending_operations_|, and only when > inflight_operations_ == 0, you run all callbacks in order and remove all entries > in that queue. > > Am I missing anything? That looks true. Removed.
LGTM for ppb_file_io_impl.* and quota_file_io.* Thanks! http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:310: if (--inflight_operations_ == 0) { It looks better now! Thanks. On 2011/07/28 14:11:00, kinuko wrote: > On 2011/07/27 18:26:08, yzshen1 wrote: > > This seems bad to me: we only fire callbacks when we don't have any pending > > operations. > > Have you considered the following approaches? > > 1) change PPB_FileIO_Impl so that it won't expect the callbacks from > > QuotaFileIO::Write are in order. (In that case, it cannot easily > > RunAndRemoveFirstPendingCallback anymore.) Or > > 2) never take any shortcut when QuotaFileIO processes writes. For example, > even > > if we fails the quota check, we takes a round trip to the file thread and > back, > > so that we make sure the callbacks to PPB_FileIO_Impl are kept in order. > > > > In my personal opinion, option (1) sounds better. What do you think? > > > > (I am fine if you would like to make the improvement in a separate change > later. > > In that case, make a TODO item.) > > Yes 1) sounds better to me too and I had wondered if I should do that, but I > didn't want to make bigger changes in file_io_impl.cc in this patch. > > I revised DidWrite to try dispatching callbacks in order whenever possible. > Hope it looks better now. http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.h (right): http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:71: std::deque<PendingOperationBase*> pending_operations_; Okay. :) On 2011/07/28 14:11:00, kinuko wrote: > On 2011/07/27 18:26:08, yzshen1 wrote: > > All you use are *queue* operations. Why to change it to deque? > > I wanted to use methods like size(), begin() and end() which are not supported > by std:queue. Also in the new code I use iterator which queue doesn't support > either. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:37: const char* buffer, nit: remove 'and'? http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:42: int32_t bytes_to_write, nit: Please be more clear about "in order" in the comments. - the same order as Write being called. - do we support multiple inflight SetLength/WillWrite/WillSetLength? (It seems WillWrite is okay, but .*SetLength are not. Right?) http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io_unittest.cc (right): http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io_unittest.cc:8: #include "base/task.h" Please arrange includes alphabetically. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io_unittest.cc:10: #include "base/memory/scoped_callback_factory.h" You have redundant includes.
LGTM for ppb_file_io_impl.* and quota_file_io.* Thanks! http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.cc (right): http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.cc:310: if (--inflight_operations_ == 0) { It looks better now! Thanks. On 2011/07/28 14:11:00, kinuko wrote: > On 2011/07/27 18:26:08, yzshen1 wrote: > > This seems bad to me: we only fire callbacks when we don't have any pending > > operations. > > Have you considered the following approaches? > > 1) change PPB_FileIO_Impl so that it won't expect the callbacks from > > QuotaFileIO::Write are in order. (In that case, it cannot easily > > RunAndRemoveFirstPendingCallback anymore.) Or > > 2) never take any shortcut when QuotaFileIO processes writes. For example, > even > > if we fails the quota check, we takes a round trip to the file thread and > back, > > so that we make sure the callbacks to PPB_FileIO_Impl are kept in order. > > > > In my personal opinion, option (1) sounds better. What do you think? > > > > (I am fine if you would like to make the improvement in a separate change > later. > > In that case, make a TODO item.) > > Yes 1) sounds better to me too and I had wondered if I should do that, but I > didn't want to make bigger changes in file_io_impl.cc in this patch. > > I revised DidWrite to try dispatching callbacks in order whenever possible. > Hope it looks better now. http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.h (right): http://codereview.chromium.org/7433006/diff/39042/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:71: std::deque<PendingOperationBase*> pending_operations_; Okay. :) On 2011/07/28 14:11:00, kinuko wrote: > On 2011/07/27 18:26:08, yzshen1 wrote: > > All you use are *queue* operations. Why to change it to deque? > > I wanted to use methods like size(), begin() and end() which are not supported > by std:queue. Also in the new code I use iterator which queue doesn't support > either. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:37: const char* buffer, nit: remove 'and'? http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:42: int32_t bytes_to_write, nit: Please be more clear about "in order" in the comments. - the same order as Write being called. - do we support multiple inflight SetLength/WillWrite/WillSetLength? (It seems WillWrite is okay, but .*SetLength are not. Right?) http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io_unittest.cc (right): http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io_unittest.cc:8: #include "base/task.h" Please arrange includes alphabetically. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io_unittest.cc:10: #include "base/memory/scoped_callback_factory.h" You have redundant includes.
if it lgt shen, then it lgtm
Thanks for reviewing! All fixed, I'm going to submit. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io.h (right): http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:37: // Returns true and when the operation is successfully dispatched. On 2011/07/28 17:40:52, yzshen1 wrote: > nit: remove 'and'? Done. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io.h:42: // |callback| are always dispatched in order. On 2011/07/28 17:40:52, yzshen1 wrote: > nit: Please be more clear about "in order" in the comments. > - the same order as Write being called. Done. > - do we support multiple inflight SetLength/WillWrite/WillSetLength? (It seems > WillWrite is okay, but .*SetLength are not. Right?) Right. I added the comment too. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... File webkit/plugins/ppapi/quota_file_io_unittest.cc (right): http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io_unittest.cc:8: #include "base/task.h" On 2011/07/28 17:40:52, yzshen1 wrote: > Please arrange includes alphabetically. Done. http://codereview.chromium.org/7433006/diff/50001/webkit/plugins/ppapi/quota_... webkit/plugins/ppapi/quota_file_io_unittest.cc:10: #include "base/memory/scoped_callback_factory.h" On 2011/07/28 17:40:52, yzshen1 wrote: > You have redundant includes. Done. |
