Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414123002:
[BlobAsync] Renderer support for blob file writing. (Closed)
|
Created:
5 years, 2 months ago by dmurph Modified:
4 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@blob-hookup Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[BlobAsync] Renderer support for blob file writing.
This implements the logic where the renderer is writing blob pieces
directly to disk, instead of sending the data to the browser.browser
R=kinuko, michaeln
BUG=375297
Committed: https://crrev.com/c27bc134ef590aa1435f9ccd6bcb35219c9b3cf3
Cr-Commit-Position: refs/heads/master@{#394985}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : test #Patch Set 4 : Rebase #Patch Set 5 : Better tests #Patch Set 6 : Added error test #Patch Set 7 : rebase #
Total comments: 16
Patch Set 8 : Added test & simplified IPC callback #Patch Set 9 : Added test & simplified IPC callback #Patch Set 10 : Added test & simplified IPC callback[ #
Total comments: 31
Patch Set 11 : Less callbacks, IPC::Sender propogation, and cleanup #Patch Set 12 : linker fix #
Total comments: 41
Patch Set 13 : comments #Patch Set 14 : comments, build fix #
Total comments: 24
Patch Set 15 : comments #
Total comments: 12
Patch Set 16 : comments #
Total comments: 4
Patch Set 17 : update histograms #
Total comments: 1
Patch Set 18 : rebase #Patch Set 19 : rebase #Patch Set 20 : rebase again, missed refactor #Patch Set 21 : and one more rebase error :/ #Messages
Total messages: 41 (10 generated)
Description was changed from ========== [Blob] Blob-to-disk implementation on Renderer side. This implements the logic where the renderer is writing blob pieces directly to disk, instead of sending the data to the browser.browser R=kinuko, michaeln BUG=375297 ========== to ========== [BlobAsync] Renderer support for blob file writing. This implements the logic where the renderer is writing blob pieces directly to disk, instead of sending the data to the browser.browser R=kinuko, michaeln BUG=375297 ==========
Hey Michael & Kinuko, Here is the renderer side of the blob-to-disk transportation. Let me know what you think, the weirdest part is where I pass in a bunch of async callbacks and task runners to the transport controller, which might need some cleanup. Daniel
Some initial comments, not read the tests yet. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:174: if (!cont) What's 'cont'? Continue? It's not too obvious why we need to return with EARLY_ABORT when 'cont' is false (and it's not documented either) https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:97: // The visitor is guaranteed to be called before this method returns. Now this comment doesn't give enough info about what this method does. nit: visitor -> |visitor| nit: re-add 'Returns:' We also need a comment about when the visitor method returns true or false. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:116: // * ReadStatus::DONE if the memory has been successfully read. For some of these results we probably don't need to repeat the almost same comment. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_message_filter.cc (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.cc:22: : IPC::MessageFilter(), nit: this line is probably not necessary https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.cc:63: base::Bind(&BlobMessageFilter::SendCancel, this, uuid)); BlobTransportController already knows the concept of IPC and knows how to send messages, and this seems to be the only callsite of OnMemoryRequest... I feel we could possibly just let the TransportController/callee to directly call Send method and it might be simpler? https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.cc:81: sender_->Send(new BlobStorageMsg_CancelBuildingBlob(uuid, code)); Common pattern is that we have Send method that checks sender_ and sends or deletes the given message depending on if it's null or not https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:392: base::Owned(file_requests.release()), file_handles, I think base::Passed(&file_requests) would be more understandable in this case? https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:78: // This responds to the request using the sender. We should add a comment about async_xxxx_callback's
https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:174: if (!cont) On 2016/04/15 at 15:02:31, kinuko wrote: > What's 'cont'? Continue? It's not too obvious why we need to return with EARLY_ABORT when 'cont' is false (and it's not documented either) Added comment, and comment on method signature. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:97: // The visitor is guaranteed to be called before this method returns. On 2016/04/15 at 15:02:32, kinuko wrote: > Now this comment doesn't give enough info about what this method does. > > nit: visitor -> |visitor| > > nit: re-add 'Returns:' > > We also need a comment about when the visitor method returns true or false. Done. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:116: // * ReadStatus::DONE if the memory has been successfully read. On 2016/04/15 at 15:02:32, kinuko wrote: > For some of these results we probably don't need to repeat the almost same comment. Done. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_message_filter.cc (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.cc:22: : IPC::MessageFilter(), On 2016/04/15 at 15:02:32, kinuko wrote: > nit: this line is probably not necessary Done. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.cc:63: base::Bind(&BlobMessageFilter::SendCancel, this, uuid)); On 2016/04/15 at 15:02:32, kinuko wrote: > BlobTransportController already knows the concept of IPC and knows how to send messages, and this seems to be the only callsite of OnMemoryRequest... I feel we could possibly just let the TransportController/callee to directly call Send method and it might be simpler? So you're saying we just give them a Send method callback? https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.cc:81: sender_->Send(new BlobStorageMsg_CancelBuildingBlob(uuid, code)); On 2016/04/15 at 15:02:32, kinuko wrote: > Common pattern is that we have Send method that checks sender_ and sends or deletes the given message depending on if it's null or not Done. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:392: base::Owned(file_requests.release()), file_handles, On 2016/04/15 at 15:02:32, kinuko wrote: > I think base::Passed(&file_requests) would be more understandable in this case? done. https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:78: // This responds to the request using the sender. On 2016/04/15 at 15:02:32, kinuko wrote: > We should add a comment about async_xxxx_callback's Done.
A first pass at it... > the weirdest part is where I pass in a bunch of async callbacks and > task runners to the transport controller, which might need some cleanup yup, i have some comments about that https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:172: bool cont = visitor.Run(memory_read, nit: continue instead of cont, then you probably don't need a comment https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:189: consolidated_size, base::Bind(&WriteMemory, memory_out)); i see the need track total_read for this use case, but don't see the need for the parameter to be baked into the callback in public api size_t total_copied= 0; VisitMem(..., base::Bind(&CopyMemory, destination, &total_copied)); https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:45: EARLY_ABORT, maybe call this CANCELLED_BY_VISITOR to make it clear it has to do with the visitor returning false https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:97: // This method calls the |visibor| callback with the given memory item, spelling |visitor| https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:114: base::Callback<bool(size_t /* total_memory_read */, initially, i didn't understand the total_memory_read param. i don't think we need this param since the callback can maintain state about progress if its needed (if the sink was a file stream or a vector, we'd just append any data received thru the callback) https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:135: BlobItemBytesResponse(request.request_number)})); is BlobItemBytesResponse.time_file_modified not needed? maybe it's used elsewhere? https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:409: error_callback, base::Passed(&io_runner))); Passing in a reply task runner seems odd. I'd expect the caller to expect to be called back on the same thread, and pass in a callback that proxies to the other thread if necessary so this utility is ignorant of that complexity. I think this is a good place to use PostTaskAndReply where the reply is delivered to a method of this class which then sends the results via ipc. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:83: scoped_refptr<base::TaskRunner> io_runner, this method can only be called on the io thread so this arg shouldnt be needed, i think you can use from with the method base::ThreadTaskRunnerHandle::Get() https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:85: const IPCSender& ipc_sender); we generally use the IPC::Sender interface for this https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:149: scoped_refptr<base::TaskRunner> io_runner, io_runner may not needed here for the same reason https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:152: const CancelCallback& error_callback); ditto IPC::Sender Since IPC::Sender would be a rawptr type, we'd need to be careful to not use a dangling pointer. We generally do that with binding methods to objects via a weakptr, and invalidating the weakptrs at an appropiate time BlobTransportController::GetResponses(...) { // do stuff StartSomeAsyncStep(base::Bind(BlobTransportController::OnGotResponses), weak_factory_.GetWeakPtr(), ...); } BlobTransportController::OnGotResponses(...) { // do stuff and produce sender->Send(msg) } // The filter would have to call this method // upon nulling out its sender_ data member. BlobTransportController::CancelAll() { blob_storage_.clear(); weak_factory_.InvalidateWeakPtrs(); } https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller_unittest.cc (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:513: std::vector<DataElement> message_descriptions; maybe move this down to just above where its first used
https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:152: const CancelCallback& error_callback); Btw-- why don't we use ThreadSafeSender here and else?
I only get an IPC::Sender* Is there something I'm missing? On Thu, Apr 21, 2016, 00:54 <kinuko@chromium.org> wrote: > > > https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... > File content/child/blob_storage/blob_transport_controller.h (right): > > > https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... > content/child/blob_storage/blob_transport_controller.h:152: const > CancelCallback& error_callback); > Btw-- why don't we use ThreadSafeSender here and else? > > https://codereview.chromium.org/1414123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/21 07:57:02, dmurph wrote: > I only get an IPC::Sender* > Is there something I'm missing? I could be the one who's missing something... we pass ThreadSafeSender in InitiateBlobTransfer, so I thought we store there and could use it here too. But we probably wanted to make sure we send things explicitly on IO thread..?
Oh I see. I was using the sender from the blob message filter, which is different. That's where the memory reqhest method is called On Thu, Apr 21, 2016, 01:04 <kinuko@chromium.org> wrote: > On 2016/04/21 07:57:02, dmurph wrote: > > I only get an IPC::Sender* > > Is there something I'm missing? > > I could be the one who's missing something... we pass ThreadSafeSender in > InitiateBlobTransfer, so I thought we store there and could use it here > too. But > we probably wanted to make sure we send things explicitly on IO thread..? > > https://codereview.chromium.org/1414123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh I see. I was using the sender from the blob message filter, which is different. That's where the memory reqhest method is called On Thu, Apr 21, 2016, 01:04 <kinuko@chromium.org> wrote: > On 2016/04/21 07:57:02, dmurph wrote: > > I only get an IPC::Sender* > > Is there something I'm missing? > > I could be the one who's missing something... we pass ThreadSafeSender in > InitiateBlobTransfer, so I thought we store there and could use it here > too. But > we probably wanted to make sure we send things explicitly on IO thread..? > > https://codereview.chromium.org/1414123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
a few more comments https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:194: // sender->Send(new BlobStorageMsg_CancelBuildingBlob(uuid, is this needed? https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:211: ipc_sender.Run(base::WrapUnique( As coded, the GetResponses helper is responsible for sending the async messages and its caller is responsible for sending the immediate messages? Seems like an unnecessary split of responsibility. As a simplification, I think you should consider hoisting all of GetResponses in to this method, and getting rid of the helper. OnMemoryRequest produces some number of ipc messages in response, perhaps asyncly. I think that lends itself well to testing, call it, examine the ipcsink for messages expected immediately, RunTillIdle() and examine expected async messages. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:77: // to save files then the call will happen asynchronously. maybe also mention that a single call to OnMemoryRequest can result in multiple response messages being sent
I chatted with Michael a bit and ended up doing this approach: * I keep hold of the IPC::Sender pointer, with the invariant that it must stay valid until the client calls CancelAllBlobTransfers(). * We accomplish the invalidation by using a weak pointer factor for reporting-back-to-controller callbacks from the file tasks. * I simplified the WriteToDisk callback by making it return the results, so I can use PostTaskAndReplyWithResult. It uses a pair, but the callbacks are much cleaner. Let me know what you think! Daniel https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:172: bool cont = visitor.Run(memory_read, On 2016/04/21 at 01:58:41, michaeln wrote: > nit: continue instead of cont, then you probably don't need a comment That is a keyword. How about "continu" https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:189: consolidated_size, base::Bind(&WriteMemory, memory_out)); On 2016/04/21 at 01:58:40, michaeln wrote: > i see the need track total_read for this use case, but don't see the need for the parameter to be baked into the callback in public api > > size_t total_copied= 0; > VisitMem(..., base::Bind(&CopyMemory, destination, &total_copied)); I modelled this off of something... I've seen this before. People will always have to use it if they're visiting, because there's no guarentee that the visitor will be called only once. I'd rather leave it this way because people have to use it anyways. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:45: EARLY_ABORT, On 2016/04/21 at 01:58:41, michaeln wrote: > maybe call this CANCELLED_BY_VISITOR to make it clear it has to do with the visitor returning false Done. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:97: // This method calls the |visibor| callback with the given memory item, On 2016/04/21 at 01:58:41, michaeln wrote: > spelling |visitor| Done. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:114: base::Callback<bool(size_t /* total_memory_read */, On 2016/04/21 at 01:58:41, michaeln wrote: > initially, i didn't understand the total_memory_read param. i don't think we need this param since the callback can maintain state about progress if its needed (if the sink was a file stream or a vector, we'd just append any data received thru the callback) I end up using it in the places I use this method. Usually we're putting it into a large memory block (either file or shared memory segment), so I'd rather keep it in here. We keep track of it anyways in the loops, so I think it's cleaner to do this instead of passing a stack pointer or something to the callback. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:135: BlobItemBytesResponse(request.request_number)})); On 2016/04/21 at 01:58:41, michaeln wrote: > is BlobItemBytesResponse.time_file_modified not needed? maybe it's used elsewhere? Oh right, I need to add that. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:194: // sender->Send(new BlobStorageMsg_CancelBuildingBlob(uuid, On 2016/04/21 at 18:51:43, michaeln wrote: > is this needed? Probably not. Removed. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:211: ipc_sender.Run(base::WrapUnique( On 2016/04/21 at 18:51:43, michaeln wrote: > As coded, the GetResponses helper is responsible for sending the async messages and its caller is responsible for sending the immediate messages? Seems like an unnecessary split of responsibility. As a simplification, I think you should consider hoisting all of GetResponses in to this method, and getting rid of the helper. > > OnMemoryRequest produces some number of ipc messages in response, perhaps asyncly. I think that lends itself well to testing, call it, examine the ipcsink for messages expected immediately, RunTillIdle() and examine expected async messages. sgtm. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:211: ipc_sender.Run(base::WrapUnique( On 2016/04/21 at 18:51:43, michaeln wrote: > As coded, the GetResponses helper is responsible for sending the async messages and its caller is responsible for sending the immediate messages? Seems like an unnecessary split of responsibility. As a simplification, I think you should consider hoisting all of GetResponses in to this method, and getting rid of the helper. > > OnMemoryRequest produces some number of ipc messages in response, perhaps asyncly. I think that lends itself well to testing, call it, examine the ipcsink for messages expected immediately, RunTillIdle() and examine expected async messages. Ok, done, check it out. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:409: error_callback, base::Passed(&io_runner))); On 2016/04/21 at 01:58:41, michaeln wrote: > Passing in a reply task runner seems odd. I'd expect the caller to expect to be > called back on the same thread, and pass in a callback that proxies to the other > thread if necessary so this utility is ignorant of that complexity. > > I think this is a good place to use PostTaskAndReply where the reply is delivered to a method of this class which then sends the results via ipc. Done. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:83: scoped_refptr<base::TaskRunner> io_runner, On 2016/04/21 at 01:58:41, michaeln wrote: > this method can only be called on the io thread so this arg shouldnt be needed, i think you can use from with the method base::ThreadTaskRunnerHandle::Get() Done. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:85: const IPCSender& ipc_sender); On 2016/04/21 at 01:58:41, michaeln wrote: > we generally use the IPC::Sender interface for this Done. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:149: scoped_refptr<base::TaskRunner> io_runner, On 2016/04/21 at 01:58:41, michaeln wrote: > io_runner may not needed here for the same reason Done. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:152: const CancelCallback& error_callback); On 2016/04/21 at 07:54:34, kinuko wrote: > Btw-- why don't we use ThreadSafeSender here and else? I don't have a thread safe version, I just have the IPC::Sender* from the message filter. I switched to using the raw pointer, but then letting the client cancel running requests to invalidate the callbacks that use it. That was after a discussion w/ Michael. https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller_unittest.cc (right): https://codereview.chromium.org/1414123002/diff/180001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:513: std::vector<DataElement> message_descriptions; On 2016/04/21 at 01:58:41, michaeln wrote: > maybe move this down to just above where its first used Done.
https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:172: bool continu = visitor.Run( nit: continu -> continue ? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_message_filter.h (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.h:42: BlobMessageFilter(scoped_refptr<base::TaskRunner> io_runner, io_runner no longer used? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:376: IPCBlobCreationCancelCode>& result) { We might get called here after ReleaseBlobConsolidation is called for the uuid? Should we check blob_storage_.find(uuid)? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:397: if (blob_storage_.erase(uuid)) { Now that consolidation is ref-counted we might have running tasks on file runner holding the consolidation (or the ref might be held somewhere else after the code grows), is this check still safe? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller_unittest.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:124: files_opened_.push_back(path); nit: it'd be probably easier to create scoped temp dir by ScopedTempDir::CreateUniqueTempDir() and create all temp files there, then all files will be gone when scope's out https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:182: } Maybe also check we have no pending tasks on any task runners at the end of each test? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:441: TEST_F(BlobTransportControllerTest, TestMemoryResponsesPublicMethod) { nit: for this one we have 'Test' prefix while for TestPublicMethods we dropped 'Test'... what's rationale here?
Thanks! The only question was about the Release w/ the consolidation now being refcounted. If it gets destructed on the file thread is that bad? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:172: bool continu = visitor.Run( On 2016/04/25 at 14:10:30, kinuko wrote: > nit: continu -> continue ? It's a keyword :( https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_message_filter.h (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_message_filter.h:42: BlobMessageFilter(scoped_refptr<base::TaskRunner> io_runner, On 2016/04/25 at 14:10:30, kinuko wrote: > io_runner no longer used? Nice catch, thanks https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:376: IPCBlobCreationCancelCode>& result) { On 2016/04/25 at 14:10:30, kinuko wrote: > We might get called here after ReleaseBlobConsolidation is called for the uuid? Should we check blob_storage_.find(uuid)? I don't think this would be called in without it being in the map (due to the weak pointer use), but I'll add it just in case. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:397: if (blob_storage_.erase(uuid)) { On 2016/04/25 at 14:10:30, kinuko wrote: > Now that consolidation is ref-counted we might have running tasks on file runner holding the consolidation (or the ref might be held somewhere else after the code grows), is this check still safe? What do you think would happen here? The consolidation would be deconstructed on the wrong thread? Is that a bad thing? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller_unittest.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:124: files_opened_.push_back(path); On 2016/04/25 at 14:10:30, kinuko wrote: > nit: it'd be probably easier to create scoped temp dir by ScopedTempDir::CreateUniqueTempDir() and create all temp files there, then all files will be gone when scope's out We have issues on windows where directories with files can't be deleted (or maybe if any of them are open?). I forget, but it's easier to avoid if we just do it this way. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:182: } On 2016/04/25 at 14:10:30, kinuko wrote: > Maybe also check we have no pending tasks on any task runners at the end of each test? Good idea, done. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:441: TEST_F(BlobTransportControllerTest, TestMemoryResponsesPublicMethod) { On 2016/04/25 at 14:10:30, kinuko wrote: > nit: for this one we have 'Test' prefix while for TestPublicMethods we dropped 'Test'... what's rationale here? Done.
https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:129: LOG(ERROR) << "Invalid consolidated size " << consolidated_size is this worth logging? search content/child for other LOG(ERROR) and you'll notice how very little there is https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:172: bool continu = visitor.Run( On 2016/04/25 14:10:30, kinuko wrote: > nit: continu -> continue ? i said the same thing, but continue already means something in c/c++ :) https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:187: CHECK(memory_out); check isn't needed becuase VisitMemory/WriteMemory will crash in an obvious way https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:102: // return and we return EARLY_ABORT. |visitor| is guaranteed to be called replace EARLY_ABORT with CANCELLED_BY_VISITOR here and below https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:109: // * ReadStatus::DONE if the memory has been successfully visited. nit: these codes are pretty self explanatory, not sure the comments are needed https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:54: using CancelCallback = these callback tyhpes are no longer needed since this class sends ipc messages directly https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:77: size_t total_read, i noticed that total_read isn't use here since we just append to the file https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:88: << " (dmurph@google.com)"; i'd vote to not bake usernames into logging do we even want this logging? logging is of practical value to such a small fraction of chrome users, if you're interested in monitoring the rate of this error occurring, we have better mechanisms to gather information like that https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:108: LOG(ERROR) << "Error seeking " << request.handle_offset << " in file, got " ditto logging question https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:128: WriteDiskRequests( wicked line wrapping https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:271: << "Invalid handle index."; style nit: the verbosity of logging at each dcheck might hurt readability more than it helps debug-ability. for debug-ability we get file/lineNumbers out of dcheck https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:308: base::Bind(&DecChildProcessRefCount)); refcounting bug? shouldn't there be one ref for each consolidation so we need blob_storage_.size() calls to decref here? https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:376: IPCBlobCreationCancelCode>& result) { On 2016/04/25 14:10:30, kinuko wrote: > We might get called here after ReleaseBlobConsolidation is called for the uuid? > Should we check blob_storage_.find(uuid)? +1 https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:113: base::Callback<void(storage::IPCBlobCreationCancelCode /* reason */)>; these callback tyhpes are no longer needed since this class sends ipc messages directly https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:147: base::WeakPtrFactory<BlobTransportController> ptr_factory_; nit: this is typically named something like weak_factory_ or weak_ptr_factory_
Thanks! https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:187: CHECK(memory_out); On 2016/04/26 at 22:50:08, michaeln wrote: > check isn't needed becuase VisitMemory/WriteMemory will crash in an obvious way ok https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:102: // return and we return EARLY_ABORT. |visitor| is guaranteed to be called On 2016/04/26 at 22:50:08, michaeln wrote: > replace EARLY_ABORT with CANCELLED_BY_VISITOR here and below done. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:109: // * ReadStatus::DONE if the memory has been successfully visited. On 2016/04/26 at 22:50:08, michaeln wrote: > nit: these codes are pretty self explanatory, not sure the comments are needed I'll keep em https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:54: using CancelCallback = On 2016/04/26 at 22:50:08, michaeln wrote: > these callback tyhpes are no longer needed since this class sends ipc messages directly Done. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:77: size_t total_read, On 2016/04/26 at 22:50:08, michaeln wrote: > i noticed that total_read isn't use here since we just append to the file you're right, removed. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:88: << " (dmurph@google.com)"; On 2016/04/26 at 22:50:08, michaeln wrote: > i'd vote to not bake usernames into logging > > do we even want this logging? > > logging is of practical value to such a small fraction of chrome users, if you're interested in monitoring the rate of this error occurring, we have better mechanisms to gather information like that Done. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:108: LOG(ERROR) << "Error seeking " << request.handle_offset << " in file, got " On 2016/04/26 at 22:50:08, michaeln wrote: > ditto logging question Done. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:128: WriteDiskRequests( On 2016/04/26 at 22:50:08, michaeln wrote: > wicked line wrapping We actually don't have a style guide for this (it's not an argument, but part of the method signiture). clang-format does this, which I think sort of makes sense, because it's the method name and not argument, where one would indent the arguments. I'd like to keep as is. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:308: base::Bind(&DecChildProcessRefCount)); On 2016/04/26 at 22:50:08, michaeln wrote: > refcounting bug? shouldn't there be one ref for each consolidation so we need blob_storage_.size() calls to decref here? yep, I fixed this. nice catch! https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:113: base::Callback<void(storage::IPCBlobCreationCancelCode /* reason */)>; On 2016/04/26 at 22:50:08, michaeln wrote: > these callback tyhpes are no longer needed since this class sends ipc messages directly Done. https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:147: base::WeakPtrFactory<BlobTransportController> ptr_factory_; On 2016/04/26 at 22:50:08, michaeln wrote: > nit: this is typically named something like weak_factory_ or weak_ptr_factory_ done.
mostly nits, but i wonder about the handling of actual_written https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:376: IPCBlobCreationCancelCode>& result) { On 2016/04/26 22:33:28, dmurph wrote: > On 2016/04/25 at 14:10:30, kinuko wrote: > > We might get called here after ReleaseBlobConsolidation is called for the > uuid? Should we check blob_storage_.find(uuid)? > > I don't think this would be called in without it being in the map (due to the > weak pointer use), but I'll add it just in case. +1 adding it Just looking at this class, it does look possible after receipt of an OnCancel msg to release a single blob, that codepath doesn't invalidate the weakptrs. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:119: base::Callback<bool(const char* /* memory */, size_t /* memory_size */)> ditto VisitorFunction https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:171: // If the visitor doesn't want to continue then we return early. return CANCELLED_BY_VISITOR makes it pretty clear what's happening, you could nix the comment https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:114: base::Callback<bool(const char* /* memory */, size_t /* memory_size */)> maybe spring for a VisitorFunction using/typedef https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:91: written += writing_size; should this be written += actual_written? and what if actual_written == 0 but written < size, should we return false in that case? https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:118: // We need to release the file so we don't automatically close the file. i'm curious, when do the file handles get closed? https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:245: // memory nit: line wraps are arbitrary https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:136: // our map, we call ChildProcess::ReleaseProcess to release our previous comments describing aggregated process refcounting are stale the fact that the class fiddles with the childprocess refcount when in the act of transporting a blob could be a class level comment instead of a comment on each method that may affect it https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller_unittest.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:325: const std::string KRefBlobUUID = "refuuid"; K --> k https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:338: consolidation->AddDataItem(CreateData("Hello3")); maybe factor the constants and test consolidation setup out into class members and a helper method, i think it's replicated 3 times https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:345: // Request for all data in shared memory should the comment say 'files'? https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:443: TEST_F(BlobTransportControllerTest, MemoryResponsesPublicMethod) { Maybe this test and the Responses test could be merged? Now that the public method is more of a one-stop-shop this test seems like it may be redundant.
dmurph@chromium.org changed reviewers: + mpearson@chromium.org
Thanks for the review! Hey Mark, can you take a look at the histograms change? Thanks, Daniel https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:119: base::Callback<bool(const char* /* memory */, size_t /* memory_size */)> On 2016/04/28 at 00:16:56, michaeln wrote: > ditto VisitorFunction Done. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:171: // If the visitor doesn't want to continue then we return early. On 2016/04/28 at 00:16:56, michaeln wrote: > return CANCELLED_BY_VISITOR makes it pretty clear what's happening, you could nix the comment ok. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:114: base::Callback<bool(const char* /* memory */, size_t /* memory_size */)> On 2016/04/28 at 00:16:56, michaeln wrote: > maybe spring for a VisitorFunction using/typedef Done. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:91: written += writing_size; On 2016/04/28 at 00:16:56, michaeln wrote: > should this be written += actual_written? > > and what if actual_written == 0 but written < size, should we return false in that case? how would that happen? We wouldn't be calling file->WriteAtCurrentPos, as we would be out of the while loop. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:118: // We need to release the file so we don't automatically close the file. On 2016/04/28 at 00:16:57, michaeln wrote: > i'm curious, when do the file handles get closed? Good point. I changed this to accept a file, and I manage the lifetime in a vector in the other callback. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:245: // memory On 2016/04/28 at 00:16:56, michaeln wrote: > nit: line wraps are arbitrary Done. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.h (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.h:136: // our map, we call ChildProcess::ReleaseProcess to release our previous On 2016/04/28 at 00:16:57, michaeln wrote: > comments describing aggregated process refcounting are stale > > the fact that the class fiddles with the childprocess refcount when in the act of transporting a blob could be a class level comment instead of a comment on each method that may affect it Done. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller_unittest.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:325: const std::string KRefBlobUUID = "refuuid"; On 2016/04/28 at 00:16:57, michaeln wrote: > K --> k Done. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:338: consolidation->AddDataItem(CreateData("Hello3")); On 2016/04/28 at 00:16:57, michaeln wrote: > maybe factor the constants and test consolidation setup out into class members and a helper method, i think it's replicated 3 times Done. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:345: // Request for all data in shared memory On 2016/04/28 at 00:16:57, michaeln wrote: > should the comment say 'files'? Done. https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller_unittest.cc:443: TEST_F(BlobTransportControllerTest, MemoryResponsesPublicMethod) { On 2016/04/28 at 00:16:57, michaeln wrote: > Maybe this test and the Responses test could be merged? Now that the public method is more of a one-stop-shop this test seems like it may be redundant. Done.
https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:88: UMA_HISTOGRAM_BOOLEAN("Storage.Blob.RendererFileWriteFailed", true); Consider changing this histogram so true and false are both used (e.g., representing the return values for this function). Otherwise, how will you use the single count of fails, i.e., what will you normalize it by? ditto below (though the situation is more complicated below because you have two distinct failure cases)
https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:91: written += writing_size; On 2016/05/09 19:55:47, dmurph wrote: > On 2016/04/28 at 00:16:56, michaeln wrote: > > should this be written += actual_written? > > > > and what if actual_written == 0 but written < size, should we return false in > that case? > > how would that happen? We wouldn't be calling file->WriteAtCurrentPos, as we > would be out of the while loop. right, let me be more clear What if (actual_written > 0) && (actual_written < writing_size). The code assumes they must be equal for a successful write, but it looks that method can return less than writing_size in some cases.
lgtm https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/260001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:91: written += writing_size; On 2016/05/09 22:24:52, michaeln wrote: > On 2016/05/09 19:55:47, dmurph wrote: > > On 2016/04/28 at 00:16:56, michaeln wrote: > > > should this be written += actual_written? > > > > > > and what if actual_written == 0 but written < size, should we return false > in > > that case? > > > > how would that happen? We wouldn't be calling file->WriteAtCurrentPos, as we > > would be out of the while loop. > > right, let me be more clear > > What if (actual_written > 0) && (actual_written < writing_size). The code > assumes they must be equal for a successful write, but it looks that method can > return less than writing_size in some cases. nm, i see you changed it to += actual_written :)
lgtm https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:72: base::Callback<bool(const char* /* memory */, size_t /* memory_size */)>; nit: I think you can just write parameter names without /* */ https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:76: ChildProcess::current()->ReleaseProcess(); nit: maybe just call DecChildProcessRefCount() instead to make sure we're consistent https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:84: size_t writing_size = std::min(max_int, size - written); nit: base::saturated_cast<int>(size - written) ? https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:103: static_cast<uint64_t>(std::numeric_limits<int64_t>::max())); nit: checked_cast() https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:134: for (const IPC::PlatformFileForTransit& file_handle : file_handles) { nit: it feels 'const auto& file_handle' is fine here
Thanks for the reviews! https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:72: base::Callback<bool(const char* /* memory */, size_t /* memory_size */)>; On 2016/05/10 at 13:30:50, kinuko wrote: > nit: I think you can just write parameter names without /* */ Done. https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:76: ChildProcess::current()->ReleaseProcess(); On 2016/05/10 at 13:30:50, kinuko wrote: > nit: maybe just call DecChildProcessRefCount() instead to make sure we're consistent Done. https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:84: size_t writing_size = std::min(max_int, size - written); On 2016/05/10 at 13:30:50, kinuko wrote: > nit: base::saturated_cast<int>(size - written) ? awesome, good find. Didn't know about that. https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:88: UMA_HISTOGRAM_BOOLEAN("Storage.Blob.RendererFileWriteFailed", true); On 2016/05/09 at 22:00:50, Mark P wrote: > Consider changing this histogram so true and false are both used (e.g., representing the return values for this function). Otherwise, how will you use the single count of fails, i.e., what will you normalize it by? > > ditto below (though the situation is more complicated below because you have two distinct failure cases) Good idea. Thanks. https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:103: static_cast<uint64_t>(std::numeric_limits<int64_t>::max())); On 2016/05/10 at 13:30:50, kinuko wrote: > nit: checked_cast() thanks https://codereview.chromium.org/1414123002/diff/280001/content/child/blob_sto... content/child/blob_storage/blob_transport_controller.cc:134: for (const IPC::PlatformFileForTransit& file_handle : file_handles) { On 2016/05/10 at 13:30:50, kinuko wrote: > nit: it feels 'const auto& file_handle' is fine here Done.
https://codereview.chromium.org/1414123002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1414123002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52453: + Recorded when seeking a file to write a part of a blob failed in the I think you're missing a word in this sentence. I cannot figure out what it means. When revising, please make sure the sentence clearly implies what true and false means. Perhaps (after reading the description below) I think you mean "whether" instead of "when". But I still don't know what "seeking a file to write a part of ..." means. Did you mean: "Recorded whether seeking within a file (in order to find the place to write a part of a blob) failed in the renderer."? https://codereview.chromium.org/1414123002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52461: + Recorded when writing a part of a blob to a file failed in the renderer. This description should clearly state what true and false means, not just when when the histogram is emitted. Did you mean "whether" instead of "when"?
Thanks! https://codereview.chromium.org/1414123002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1414123002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52453: + Recorded when seeking a file to write a part of a blob failed in the On 2016/05/16 at 23:25:56, Mark P wrote: > I think you're missing a word in this sentence. I cannot figure out what it means. > When revising, please make sure the sentence clearly implies what true and false means. > Perhaps (after reading the description below) I think you mean "whether" instead of "when". But I still don't know what "seeking a file to write a part of ..." means. Did you mean: > "Recorded whether seeking within a file (in order to find the place to write a part of a blob) failed in the renderer."? Yeah, that sounds a lot better. Thanks! https://codereview.chromium.org/1414123002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52461: + Recorded when writing a part of a blob to a file failed in the renderer. On 2016/05/16 at 23:25:56, Mark P wrote: > This description should clearly state what true and false means, not just when when the histogram is emitted. > Did you mean "whether" instead of "when"? Yeah. Done.
histograms.xml lgtm https://codereview.chromium.org/1414123002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1414123002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52976: + Recorded whether seeking within a file (in order to write a part of a blob) optional nit: I think this might sound better without the word "Recorded" at the beginning. ditto below.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, michaeln@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1414123002/#ps360001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414123002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414123002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, michaeln@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1414123002/#ps380001 (title: "rebase again, missed refactor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414123002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414123002/380001
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, michaeln@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1414123002/#ps400001 (title: "and one more rebase error :/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414123002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414123002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [BlobAsync] Renderer support for blob file writing. This implements the logic where the renderer is writing blob pieces directly to disk, instead of sending the data to the browser.browser R=kinuko, michaeln BUG=375297 ========== to ========== [BlobAsync] Renderer support for blob file writing. This implements the logic where the renderer is writing blob pieces directly to disk, instead of sending the data to the browser.browser R=kinuko, michaeln BUG=375297 Committed: https://crrev.com/c27bc134ef590aa1435f9ccd6bcb35219c9b3cf3 Cr-Commit-Position: refs/heads/master@{#394985} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/c27bc134ef590aa1435f9ccd6bcb35219c9b3cf3 Cr-Commit-Position: refs/heads/master@{#394985} |
