Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1098853003:
[BlobAsync] Patch 4: Browser Classes & Logic. (Closed)
|
Created:
5 years, 8 months ago by dmurph Modified:
5 years ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jsbell, kinuko+fileapi, nhiroki, tzik, wkorman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[BlobAsync] Patch 4: Browser Classes & Logic.
This patch encompasses all browser classes and logic for blob async transportation. This is a no-op.
Patches:
1: https://codereview.chromium.org/1287303002 (committed)
2: https://codereview.chromium.org/1288373002 (committed)
3: https://codereview.chromium.org/1292523002 (committed)
4: https://codereview.chromium.org/1098853003
Hookup: https://codereview.chromium.org/1234813004
See https://bit.ly/BlobStorageRefactor
BUG=375297
Committed: https://crrev.com/233c81baddf00838335bb712a3f8cd67fd327e71
Cr-Commit-Position: refs/heads/master@{#362802}
Patch Set 1 #Patch Set 2 : Removed constexpr for android #Patch Set 3 : Fix for non-C++11 builds #Patch Set 4 : Non C++11 fixes, added nodisk logic and test #
Total comments: 38
Patch Set 5 : Comments #Patch Set 6 : Build fix, tested #Patch Set 7 : Fix for template ambiguity #
Total comments: 16
Patch Set 8 : Comments #Patch Set 9 : formatting #Patch Set 10 : Update! #Patch Set 11 : [BlobAsync] Patch 3: Browser Classes & Logic #Patch Set 12 : [BlobAsync] Patch 4: Browser Classes & Logic #Patch Set 13 : rebase #Patch Set 14 : moving stuff around #
Total comments: 84
Patch Set 15 : Comments & documentation #
Total comments: 1
Patch Set 16 : comments, and simplification #
Total comments: 8
Patch Set 17 : comments, and removed file handles #Patch Set 18 : Removed temp file, and fixed tests #
Total comments: 31
Patch Set 19 : comments #Patch Set 20 : comments #
Total comments: 3
Patch Set 21 : Comments #
Total comments: 33
Patch Set 22 : comments #Patch Set 23 : comments #
Total comments: 38
Patch Set 24 : comments #Patch Set 25 : maybe fixed windows #Patch Set 26 : maybe fixed windows #
Total comments: 5
Patch Set 27 : windows & asan fix #
Total comments: 8
Patch Set 28 : comments & test fix #Patch Set 29 : consolidated handle sizes #
Total comments: 6
Patch Set 30 : Removed size parameterization #Patch Set 31 : fixed comments #Patch Set 32 : fix #Patch Set 33 : Removed Bad Test #Patch Set 34 : Missed duplicate invalid test #Dependent Patchsets: Messages
Total messages: 69 (14 generated)
dmurph@chromium.org changed reviewers: + kinuko@chromium.org, michaeln@chromium.org
Hello Kinuko (and eventually Michael), Michael is currently on vacation and I was wondering if you could take a look at this patch. Basically, I'm adding all of the logic for asynchronously transporting and storing memory from the renderer to the browser for blob storage. These classes aren't hooked up to anything yet, but I wanted to write the tests and get the logic in place first before continuing with the protocol changes. Basic algorithm, X > Y: * If the size of the blob is > X, then consolidate all memory in files and keep it in files (referenced the files with an offset & size for storage) * If the size of the blob is > Y, then consolidate all memory in shared memory. When it gets the browser, unconsolidate the parts that have 'file' or 'blob' items in between, and store the memory as chunks with max size = Z * Otherwise, just send the data over IPC. Let me know what you think, Daniel http://i.imgur.com/02bbP2r.gif
(Sorry for slow response, it's also vacation season in Japan and it's taking a bit for me to digest the CL & design. I'll take a look later this week)
On 2015/04/28 at 14:35:29, kinuko wrote: > (Sorry for slow response, it's also vacation season in Japan and it's taking a bit for me to digest the CL & design. I'll take a look later this week) No worries! I can also give you a quick rundown of the whole project over VC, which would probably be easier. Want me to throw something on your calendar?
Yeah that might help. Would you try booking my calendar? (May 4-6 are holidays in Japan and I'll be ooo) On Thu, Apr 30, 2015 at 9:20 AM, <dmurph@chromium.org> wrote: > On 2015/04/28 at 14:35:29, kinuko wrote: > >> (Sorry for slow response, it's also vacation season in Japan and it's >> taking a >> > bit for me to digest the CL & design. I'll take a look later this week) > > No worries! I can also give you a quick rundown of the whole project over > VC, > which would probably be easier. Want me to throw something on your > calendar? > > https://codereview.chromium.org/1098853003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Whoops, I forgot to add this in our VC, here is the general algorithm we use now for deciding the transport strategy: total_blob_size = all bytes in the blob if total_blob_size > kMaxBlobSize (say 400MB) // Request all data in files // (Segment all of the existing data into // file blocks, of <= kMaxFileSize) else if total_blob_size > kMaxIPCSize (this is 150KB) // Request all data in shared memory // (Segment all of the existing data into // shared memory blocks, of <= kMaxSharedMemorySize) else // Request all data to be sent over IPC
I haven't read through all the code yet, but let me send out first round of review comments. (Many of them are minor style related ones) https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:23: std::vector<BlobTransportStrategy::MemoryItemRequest>* requests) nit: one parameter on each line (here and below) https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:32: } nit: I'd keep segment_size within the loop (reads 0.2 sec faster) while (memory_left > 0) { SizeType segment_size = std::min(...); segment_sizes->push_back(segment_size); memory_left -= segment_size; } https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:32: } What is this segment_sizes for? (Not used?) In the current code it doesn't look it makes much sense to have this same code in ctors of *Strategy. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:35: void operator()(DataElement::Type type, size_t element_index, nit: in chromium code I've hardly seen operator() usage (I have in blink), the most common way to express closure-like type is to have a method Run(). Not sure if it's explicitly discouraged though (while operator overloading is discouraged) https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:43: request.browser_item_index = storage_element_index; is storage_element_index == element_index always true for this one? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:49: request.message.size = size; nit: why the order of indices/sizes different in function params and in BlobItemBytesRequest? (i.e. Having 'size' between two <index, offset> pairs or after the paris) https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:51: request.message.handle_offset = segment_offset; nit: Intentionally not using BlobItemBytesRequest::CreateFileRequest(...) here? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:90: storage_element_index += 2; I think I'm lost here; why do we add 2? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:124: /* static */ void BlobTransportStrategy::ForEachWithSegment( nit: same method order in .h and in .cc https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:169: error_ = BlobTransportStrategy::ERROR_NONE; Could Initialize() be called multiple times? Is there a reason we don't do this in ctor? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:172: size_t memory_items = 0; Not used after line 177? If it's for the # of memory items let's name it num_memory_items or memory_items_count or sth like that. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:188: if (total_memory_size > max_blob_in_memory_size) { max_blob_in_memory_size == kMaxBlobSize (e.g. 400MB) in your comment? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:212: DCHECK_LE(total_memory_size, std::numeric_limits<size_t>::max()); If blob_item_infos comes over IPC this cannot be guaranteed, DCHECK_LE might not be enough https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:51: const std::vector<DataElement> blob_item_infos); nit: split the lines to have one parameter for each line nit: const ref for std::vector<DataElement> Can you add brief documentation for each parameter? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:57: } Are these intentionally made non-const references? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:75: // SizeType /*size*/) would be nice to have a rough explanation about 'element' and 'segment'. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:80: static void ForEachWithSegment(const std::vector<DataElement> items, nit: const ref https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:80: static void ForEachWithSegment(const std::vector<DataElement> items, This could be private? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:88: // can be size_t, but same as file handles so we can use the same methods. ? https://codereview.chromium.org/1098853003/diff/60001/storage/common/fileapi/... File storage/common/fileapi/blob_item_bytes_request.h (right): https://codereview.chromium.org/1098853003/diff/60001/storage/common/fileapi/... storage/common/fileapi/blob_item_bytes_request.h:31: static constexpr uint64_t kInvalidSize = kuint64max; Can't these be just static const if we use constants?
Updated! Thanks for the comments, hopefully things are clearer. http://i.imgur.com/o1PVJM3.gif https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:23: std::vector<BlobTransportStrategy::MemoryItemRequest>* requests) On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > nit: one parameter on each line (here and below) Done. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:32: } On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > nit: I'd keep segment_size within the loop (reads 0.2 sec faster) > > while (memory_left > 0) { > SizeType segment_size = std::min(...); > segment_sizes->push_back(segment_size); > memory_left -= segment_size; > } Done. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:35: void operator()(DataElement::Type type, size_t element_index, On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > nit: in chromium code I've hardly seen operator() usage (I have in blink), the most common way to express closure-like type is to have a method Run(). Not sure if it's explicitly discouraged though (while operator overloading is discouraged) I could turn these into lambdas as well. All the for_each std library methods (and other algorithm stuff) takes functors with the () operator overloaded, so I was following this pattern. But I can change it to whatever. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:43: request.browser_item_index = storage_element_index; On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > is storage_element_index == element_index always true for this one? No, as the element index can stay the same for multiple callbacks (where we need to split the item into chunks). https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:49: request.message.size = size; On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > nit: why the order of indices/sizes different in function params and in BlobItemBytesRequest? (i.e. Having 'size' between two <index, offset> pairs or after the paris) No reason, which way do you prefer? https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:51: request.message.handle_offset = segment_offset; On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > nit: Intentionally not using BlobItemBytesRequest::CreateFileRequest(...) here? Yes, it's a little more readable, as you can read the name of the property that's being set. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:90: storage_element_index += 2; On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > I think I'm lost here; why do we add 2? Added comment. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:124: /* static */ void BlobTransportStrategy::ForEachWithSegment( On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > nit: same method order in .h and in .cc Done. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:169: error_ = BlobTransportStrategy::ERROR_NONE; On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > Could Initialize() be called multiple times? Is there a reason we don't do this in ctor? Yes it can, it's reusable. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:172: size_t memory_items = 0; On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > Not used after line 177? > > If it's for the # of memory items let's name it num_memory_items or memory_items_count or sth like that. Removed. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:188: if (total_memory_size > max_blob_in_memory_size) { On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > max_blob_in_memory_size == kMaxBlobSize (e.g. 400MB) in your comment? Yes, here we know that we need to save to disk. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.cc:212: DCHECK_LE(total_memory_size, std::numeric_limits<size_t>::max()); On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > If blob_item_infos comes over IPC this cannot be guaranteed, DCHECK_LE might not be enough Good catch, changed to CHECK. This should still never hit though, as we're checking for maximums above. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:51: const std::vector<DataElement> blob_item_infos); On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > nit: split the lines to have one parameter for each line > > nit: const ref for std::vector<DataElement> > > Can you add brief documentation for each parameter? Done. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:57: } On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > Are these intentionally made non-const references? Whoops, done. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:75: // SizeType /*size*/) On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > would be nice to have a rough explanation about 'element' and 'segment'. Done. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:80: static void ForEachWithSegment(const std::vector<DataElement> items, On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > This could be private? Done, done. https://codereview.chromium.org/1098853003/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_transport_strategy.h:88: // can be size_t, but same as file handles so we can use the same methods. On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > ? Whoops, forgot to remove this. https://codereview.chromium.org/1098853003/diff/60001/storage/common/fileapi/... File storage/common/fileapi/blob_item_bytes_request.h (right): https://codereview.chromium.org/1098853003/diff/60001/storage/common/fileapi/... storage/common/fileapi/blob_item_bytes_request.h:31: static constexpr uint64_t kInvalidSize = kuint64max; On 2015/04/30 at 15:04:21, kinuko (ooo til May 6) wrote: > Can't these be just static const if we use constants? Yeah, android and windows are having a really hard time with them. Constexpr is technically adds more constraints and makes it easier for the compiler.
+cc wkorman
Some more comments https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... File content/browser/fileapi/blob_transport_strategy_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... content/browser/fileapi/blob_transport_strategy_unittest.cc:25: void AddBlobItem() { AddBlobInfo() of AddBlobItemInfo() to be consistent? https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... content/browser/fileapi/blob_transport_strategy_unittest.cc:31: std::vector<DataElement> infos_; I don't feel having this as a member variable really helpful? Just plainly working on std::vector<Element> in the test code would be probably more straightforward. For example AddMemoryInfo(305); strategy.Initialize(...., infos_); Doesn't really tell readers about relationship between the 1st and 2nd lines, while std::vector<DataElement> infos; AddMemoryInfo(305, &infos); strategy.Initialize(...., infos); could give a slightly better idea what's going on. https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... content/browser/fileapi/blob_transport_strategy_unittest.cc:58: strategy.Initialize(BlobTransportStrategy::MODE_DISK, 100, 200, 400, 300, nit: could we explicitly note about some important parameters that determine the behavior in a comment, e.g. max_file_size == 400 and max_blob_in_memory_size == 300 (< 1000)? (Here and below) https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:133: } nit: no { } required for single-line body in chromium https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:151: BlobTransportStrategy::ForEachWithSegment(blob_item_infos, nit: we probably don't need BlobTransportStorategy:: prefix here? (and below) https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:191: requests_.push_back(request); So in the current design the decision is basically made based on the total memory bytes in the given items and all requests will have the single, same strategy. Is that right? https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:239: } I have a feeling that this is a bit verbose and could be probably done together in ForEachWithSegment() loop, is that the case? https://codereview.chromium.org/1098853003/diff/120001/storage/common/fileapi... File storage/common/fileapi/blob_item_bytes_request.h (right): https://codereview.chromium.org/1098853003/diff/120001/storage/common/fileapi... storage/common/fileapi/blob_item_bytes_request.h:21: enum IPCBlobCreationCancelCode { Not yet used?
PTAL. Next CL is the IPC stuff to hook everything up. https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... File content/browser/fileapi/blob_transport_strategy_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... content/browser/fileapi/blob_transport_strategy_unittest.cc:25: void AddBlobItem() { On 2015/05/25 at 08:32:02, kinuko wrote: > AddBlobInfo() of AddBlobItemInfo() to be consistent? Done. https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... content/browser/fileapi/blob_transport_strategy_unittest.cc:31: std::vector<DataElement> infos_; On 2015/05/25 at 08:32:02, kinuko wrote: > I don't feel having this as a member variable really helpful? Just plainly working on std::vector<Element> in the test code would be probably more straightforward. For example > > AddMemoryInfo(305); > strategy.Initialize(...., infos_); > > Doesn't really tell readers about relationship between the 1st and 2nd lines, while > > std::vector<DataElement> infos; > AddMemoryInfo(305, &infos); > strategy.Initialize(...., infos); > > could give a slightly better idea what's going on. Done. https://codereview.chromium.org/1098853003/diff/120001/content/browser/fileap... content/browser/fileapi/blob_transport_strategy_unittest.cc:58: strategy.Initialize(BlobTransportStrategy::MODE_DISK, 100, 200, 400, 300, On 2015/05/25 at 08:32:02, kinuko wrote: > nit: could we explicitly note about some important parameters that determine the behavior in a comment, e.g. max_file_size == 400 and max_blob_in_memory_size == 300 (< 1000)? (Here and below) Done. Added variables names and better descriptions. https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:133: } On 2015/05/25 at 08:32:02, kinuko wrote: > nit: no { } required for single-line body in chromium Done. https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:151: BlobTransportStrategy::ForEachWithSegment(blob_item_infos, On 2015/05/25 at 08:32:02, kinuko wrote: > nit: we probably don't need BlobTransportStorategy:: prefix here? (and below) Done. https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:191: requests_.push_back(request); On 2015/05/25 at 08:32:03, kinuko wrote: > So in the current design the decision is basically made based on the total memory bytes in the given items and all requests will have the single, same strategy. Is that right? Yes. https://codereview.chromium.org/1098853003/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_transport_strategy.cc:239: } On 2015/05/25 at 08:32:03, kinuko wrote: > I have a feeling that this is a bit verbose and could be probably done together in ForEachWithSegment() loop, is that the case? I'm scared that it will make that function too complicated, as it is already a little difficult to understand it's function. I can add more comments in the .h file to clarify what this does, as it's pretty simple. https://codereview.chromium.org/1098853003/diff/120001/storage/common/fileapi... File storage/common/fileapi/blob_item_bytes_request.h (right): https://codereview.chromium.org/1098853003/diff/120001/storage/common/fileapi... storage/common/fileapi/blob_item_bytes_request.h:21: enum IPCBlobCreationCancelCode { On 2015/05/25 at 08:32:03, kinuko wrote: > Not yet used? No not yet, this is for the IPC messages that will come in the next patch.
Alright guys. This patch grew a lot. This now contains all of the classes needed to start doing async blob transportation, without hooking any of them up. The patch here: https://codereview.chromium.org/1234813004 actually does the hookup. I understand this is pretty huge. I tried to test everything very thoroughly. Let me know if I should do anything else. Daniel
Hi guys, I reduced the patch a lot with the previous split up, there are now 4 patches total, and then 1 final patch here: https://codereview.chromium.org/1234813004 Let me know if I can change anything else. Daniel
Description was changed from ========== This patch encompasses all browser classes and logic for blob async transportation. This is a no-op. Patches: 1: https://codereview.chromium.org/1287303002 2: https://codereview.chromium.org/1288373002 3: https://codereview.chromium.org/1292523002 4: https://codereview.chromium.org/1098853003 Hookup: https://codereview.chromium.org/1234813004 See https://bit.ly/BlobStorageRefactor BUG=375297 ========== to ========== [BlobAsync] Patch 4: Browser Classes & Logic. This patch encompasses all browser classes and logic for blob async transportation. This is a no-op. Patches: 1: https://codereview.chromium.org/1287303002 2: https://codereview.chromium.org/1288373002 3: https://codereview.chromium.org/1292523002 4: https://codereview.chromium.org/1098853003 Hookup: https://codereview.chromium.org/1234813004 See https://bit.ly/BlobStorageRefactor BUG=375297 ==========
Gentle ping on this, this is the next patch :)
Description was changed from ========== [BlobAsync] Patch 4: Browser Classes & Logic. This patch encompasses all browser classes and logic for blob async transportation. This is a no-op. Patches: 1: https://codereview.chromium.org/1287303002 2: https://codereview.chromium.org/1288373002 3: https://codereview.chromium.org/1292523002 4: https://codereview.chromium.org/1098853003 Hookup: https://codereview.chromium.org/1234813004 See https://bit.ly/BlobStorageRefactor BUG=375297 ========== to ========== [BlobAsync] Patch 4: Browser Classes & Logic. This patch encompasses all browser classes and logic for blob async transportation. This is a no-op. Patches: 1: https://codereview.chromium.org/1287303002 (committed) 2: https://codereview.chromium.org/1288373002 (committed) 3: https://codereview.chromium.org/1292523002 (committed) 4: https://codereview.chromium.org/1098853003 Hookup: https://codereview.chromium.org/1234813004 See https://bit.ly/BlobStorageRefactor BUG=375297 ==========
Haven't looked all files yet, but sending comments anyways https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:45: done.Run(&builder); Calling this synchronously while the method name tells 'Async' makes me feel a bit nervous, could this be fired asyncly or I feel at least this should be clearly documented. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:61: memory_available, uuid, descriptions); Can you add a comment for '0'? (disk_space_left?) https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:64: LOG(ERROR) << "Error initializing transport strategy: " DVLOG ? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:162: BlobAsyncBuilderHost::BlobBuildingState::BlobBuildingState() nit: could these inner-class definitions be placed at the top of this file so that class definitions don't interleave? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:175: const auto& requests = state->transport_strategy.requests(); nit: prefer using non-auto class here https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:249: if (state_it == async_blob_map_.end()) { I feel this and the similar line in DoneAndCleanup should be simply DCHECK, could this happen? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:255: delete state; could use ScopedPtrMap or assign state_it->second to scoped_ptr on line 252, let's avoid manual deletion https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:8: #include <storage/browser/blob/blob_async_transport_strategy.h> Looking a bit unusual? Use "" instead of <> and have empty line per our usual style. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:56: const std::vector<BlobItemBytesResponse> responses); const ref https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:61: size_t blob_building_count() { return async_blob_map_.size(); } nit: could be a const method https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:64: void SetMemoryConstants(size_t max_ipc_memory_size, nit: add 'ForTesting' for test-only methods (so that presubmit works) https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:96: base::Callback<void(BlobDataBuilder*)> done; nit: would be nice if these are in the same order as that of params for BuildBlobAsync (assuming they're corresponding to each other) https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:5: #include <storage/browser/blob/blob_async_transport_strategy.h> nit: use "" instead of <> and have empty line per our usual style
Thanks! Done. I notice that I can't use the IPC::PlatformFile include, so I'll be changing that on my next blob work day (tomorrow) to be a ScopedPtrVector with file objects. Then in content/ we'll transform them to the IPC file. (we don't do anything with those files yet, that's for the disk hookup patches) https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:45: done.Run(&builder); On 2015/11/17 at 14:51:41, kinuko wrote: > Calling this synchronously while the method name tells 'Async' makes me feel a bit nervous, could this be fired asyncly or I feel at least this should be clearly documented. Renamed, and clarified behavior in comment. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:61: memory_available, uuid, descriptions); On 2015/11/17 at 14:51:41, kinuko wrote: > Can you add a comment for '0'? (disk_space_left?) Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:64: LOG(ERROR) << "Error initializing transport strategy: " On 2015/11/17 at 14:51:41, kinuko wrote: > DVLOG ? Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:162: BlobAsyncBuilderHost::BlobBuildingState::BlobBuildingState() On 2015/11/17 at 14:51:41, kinuko wrote: > nit: could these inner-class definitions be placed at the top of this file so that class definitions don't interleave? Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:175: const auto& requests = state->transport_strategy.requests(); On 2015/11/17 at 14:51:41, kinuko wrote: > nit: prefer using non-auto class here Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:249: if (state_it == async_blob_map_.end()) { On 2015/11/17 at 14:51:41, kinuko wrote: > I feel this and the similar line in DoneAndCleanup should be simply DCHECK, could this happen? Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:255: delete state; On 2015/11/17 at 14:51:41, kinuko wrote: > could use ScopedPtrMap or assign state_it->second to scoped_ptr on line 252, let's avoid manual deletion Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:8: #include <storage/browser/blob/blob_async_transport_strategy.h> On 2015/11/17 at 14:51:41, kinuko wrote: > Looking a bit unusual? > > Use "" instead of <> and have empty line per our usual style. Whoops, eclipse artifact. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:56: const std::vector<BlobItemBytesResponse> responses); On 2015/11/17 at 14:51:42, kinuko wrote: > const ref Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:61: size_t blob_building_count() { return async_blob_map_.size(); } On 2015/11/17 at 14:51:41, kinuko wrote: > nit: could be a const method Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:64: void SetMemoryConstants(size_t max_ipc_memory_size, On 2015/11/17 at 14:51:41, kinuko wrote: > nit: add 'ForTesting' for test-only methods (so that presubmit works) Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:96: base::Callback<void(BlobDataBuilder*)> done; On 2015/11/17 at 14:51:41, kinuko wrote: > nit: would be nice if these are in the same order as that of params for BuildBlobAsync (assuming they're corresponding to each other) Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:5: #include <storage/browser/blob/blob_async_transport_strategy.h> On 2015/11/17 at 14:51:42, kinuko wrote: > nit: use "" instead of <> and have empty line per our usual style Done. Eclipse artifact.
Finally some comments. What this does looks pretty good. I haven't looked at the async builder class or tests closely yet. I thought this was tough code to read for some reason? I have suggestions for how to describe what this code is doing. The concept/terms used in the comments didn't help me too much. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:56: state->cancel = cancel; naming nit: consider appending _callback to request_memory, done, and cancel (they look bool'ish at a glance) https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:64: LOG(ERROR) << "Error initializing transport strategy: " is the logging needed in a release build, here and below? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:80: if (state_it == async_blob_map_.end()) { is this indicative of a bad ipc message or could this legitimately happen? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:89: LOG(ERROR) << "Invalid request number " << response.request_number; we might want to treat this as a badipc? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:95: LOG(ERROR) << "Already received response for that request."; and this one too, when would this be expected? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:101: if (response.inline_data.size() < request.message.size) { ditto https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:112: if (state->num_shared_memory_requests == 0) { ditto https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:188: if (state->next_request == num_requests) { move this test/early exit up above the local vectors https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:249: if (state_it == async_blob_map_.end()) { looks like this could be dcheck'able instead, i think all callers of this method have an early exit for this condition prior to calling this method https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:261: if (state_it == async_blob_map_.end()) { ditto https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:8: #include <storage/browser/blob/blob_async_transport_strategy.h> should this include be listed below? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:31: // This class holds all blobs that are currently being built asynchronously, and Is the intent to have one-per child process or one-per blob storage context? Just curious. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:14: const char* kTemporaryFileName = "TODO(dmurph) Replace with real filename"; what about this? make the TODO a comment and the filename something else. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:155: A comment like the one you made in the review log would help in here... if total_blob_size > kMaxBlobSize (say 400MB) // Request all data in files // (Segment all of the existing data into // file blocks, of <= kMaxFileSize) else if total_blob_size > kMaxIPCSize (this is 150KB) // Request all data in shared memory // (Segment all of the existing data into // shared memory blocks, of <= kMaxSharedMemorySize) else // Request all data to be sent over IPC https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:157: BlobAsyncTransportStrategy::Mode mode, Is MODE_NO_DISK functionally equivalent to MODE_DISK with disk_space_left and max_file_size zero? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:201: if (total_bytes_size_ > memory_left) { It looks like it is provided |memory_left| is less than |max_blob_in_memory_size|. It might be nice to not have the two modes if possible. Do you think disk_space_left = 0 can be used to distinguish between the two? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:242: This was the note i made at some point to help me understand what what was going on. A comment like this might help. Splits each |element| into one or more |segments| of a max_size, invokes the strategy to determine the request to make for each |segment| produced. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:245: const std::vector<DataElement>& items, it'd be good to use consistent local names for the vector<DataElement> and vector<DataRequest> in this file. i'd vote for |elements| and |requests| and the corresponding singular. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:257: visitor->VisitSegment(element, element_index, 0, 0, 0, 0); The caller and the callee both have special cases for TYPE_BYTES. Seems like one or the other of these should not be needed? Maybe add a second virtual method for VisitNonBytesSegment(e, i) https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:279: /* static */ bool BlobAsyncTransportStrategy::ShouldBeShortcut( style nit: this kind of comment in front of the return value is not used in the codebase, there are some place where it's annotated like... // static void Foo() { } /* static */ void Foo() { } https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:284: for (size_t element_index = 0; element_index < items_length; would a simpler loop construct work here and also in the ForEach method? You might be able to get rid of some locals. for (const DataElement& element : elements) https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:305: for (; memory_left > 0; memory_left -= segment_size) { can this be done with modulo math w/o looping? v.insert(v.begin(), total_memory_size / max_segment_size, max_segment_size); v.push_back(total_memory_size % max_segment_size); https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:44: // sort of flexibility, as everything has an offset and size specified. The comment is verbose, but honestly i'm not sure its very helpful? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:55: struct MemoryItemRequest { Using 'Memory' in this struct name threw me off a little since it's used for both all xfer types (file, sharedmem, inlineipc). The src in the renderer is memory but the dest in the browser is not necessarily. Idk if its worth word smithing, DataRequest? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:57: size_t browser_item_index; it's not real clear what this is an index into, builder_item_index?, at least worth a comment https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:68: class BlobSegmentVisitor { can this be private? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:83: void Initialize(Mode mode, About the packaging of this logic, feels like more of a function than a class? A function that computes the requests that should be made for a blob's elements given size constraints. It also seeds a builder with future elements waiting to be populated with the requested data. (a briefer comment like that at the top of the class might help) ResultType ComputeDataRequestsNeeded(elements, size_constraints, &data_requests_to_make, &seeded_builder); https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:93: std::vector<uint64_t>& file_handles() { return file_handles_; } The comments and member names make it sound like the strategy class returns actual handles. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:96: // with the first size, it will always be greater than the later sizes) comment could be simpler, sizes are sorted from largest to smallest. are the file_handle_sizes sorted too? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:129: static void CreateHandleSizes(SizeType total_memory_size, naming suggestion: ComputeHandleSizes to avoid having CreateHandle in the name since the class does not create handles https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:135: std::vector<uint64_t> file_handles_; naming suggestions: append sizes_ to these names
https://codereview.chromium.org/1098853003/diff/280001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:50: void BuildBlob( StartBuildingBlob?
Thanks for the comments! I was able to simplify things, which was nice, around disk reasoning. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:56: state->cancel = cancel; On 2015/11/17 at 21:55:28, michaeln wrote: > naming nit: consider appending _callback to request_memory, done, and cancel (they look bool'ish at a glance) Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:64: LOG(ERROR) << "Error initializing transport strategy: " On 2015/11/17 at 21:55:28, michaeln wrote: > is the logging needed in a release build, here and below? Can I log to debug? There's no way of telling what the error for the developer, and I can check fail because we need to tell the renderer to cancel & release the blob info. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:80: if (state_it == async_blob_map_.end()) { On 2015/11/17 at 21:55:27, michaeln wrote: > is this indicative of a bad ipc message or could this legitimately happen? Bad IPC message. I could have the check happen earlier, would that be better? So there's a IsBlobBeingBuilt(const std::string& uuid) method, and the the BlobStorageHost calls that method to see if we're currently building. Then we DCHECK everywhere else. Look good? https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:89: LOG(ERROR) << "Invalid request number " << response.request_number; On 2015/11/17 at 21:55:28, michaeln wrote: > we might want to treat this as a badipc? See above resolution. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:95: LOG(ERROR) << "Already received response for that request."; On 2015/11/17 at 21:55:28, michaeln wrote: > and this one too, when would this be expected? Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:101: if (response.inline_data.size() < request.message.size) { On 2015/11/17 at 21:55:28, michaeln wrote: > ditto Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:188: if (state->next_request == num_requests) { On 2015/11/17 at 21:55:28, michaeln wrote: > move this test/early exit up above the local vectors Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:261: if (state_it == async_blob_map_.end()) { On 2015/11/17 at 21:55:27, michaeln wrote: > ditto Changed to scoped ptr map, and cleaned this up a bit. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:31: // This class holds all blobs that are currently being built asynchronously, and On 2015/11/17 at 21:55:28, michaeln wrote: > Is the intent to have one-per child process or one-per blob storage context? Just curious. One per child process. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:14: const char* kTemporaryFileName = "TODO(dmurph) Replace with real filename"; On 2015/11/17 at 21:55:28, michaeln wrote: > what about this? make the TODO a comment and the filename something else. That only adds an extra line of code. This is deleted and redone a bit for the blob paging (this is never used now, and should never be used). https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:155: On 2015/11/17 at 21:55:28, michaeln wrote: > A comment like the one you made in the review log would help in here... > > if total_blob_size > kMaxBlobSize (say 400MB) > // Request all data in files > // (Segment all of the existing data into > // file blocks, of <= kMaxFileSize) > else if total_blob_size > kMaxIPCSize (this is 150KB) > // Request all data in shared memory > // (Segment all of the existing data into > // shared memory blocks, of <= kMaxSharedMemorySize) > else > // Request all data to be sent over IPC Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:157: BlobAsyncTransportStrategy::Mode mode, On 2015/11/17 at 21:55:28, michaeln wrote: > Is MODE_NO_DISK functionally equivalent to MODE_DISK with disk_space_left and max_file_size zero? Not quite, but I changed it so it is. After doing the disk part, I can simplify this part and reduce it's responsibilities a bit. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:201: if (total_bytes_size_ > memory_left) { On 2015/11/17 at 21:55:28, michaeln wrote: > It looks like it is provided |memory_left| is less than |max_blob_in_memory_size|. It might be nice to not have the two modes if possible. Do you think disk_space_left = 0 can be used to distinguish between the two? Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:242: On 2015/11/17 at 21:55:28, michaeln wrote: > This was the note i made at some point to help me understand what what was going on. A comment like this might help. > > Splits each |element| into one or more |segments| of a max_size, invokes the strategy to determine the request to make for each |segment| produced. Done, also added a bit to clarify that a segment can span multiple elements. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:245: const std::vector<DataElement>& items, On 2015/11/17 at 21:55:28, michaeln wrote: > it'd be good to use consistent local names for the vector<DataElement> and vector<DataRequest> in this file. i'd vote for |elements| and |requests| and the corresponding singular. Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:257: visitor->VisitSegment(element, element_index, 0, 0, 0, 0); On 2015/11/17 at 21:55:28, michaeln wrote: > The caller and the callee both have special cases for TYPE_BYTES. Seems like one or the other of these should not be needed? Maybe add a second virtual method for VisitNonBytesSegment(e, i) Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:279: /* static */ bool BlobAsyncTransportStrategy::ShouldBeShortcut( On 2015/11/17 at 21:55:28, michaeln wrote: > style nit: this kind of comment in front of the return value is not used in the codebase, there are some place where it's annotated like... > > // static > void Foo() { > } > > /* static */ > void Foo() { > } ok. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:284: for (size_t element_index = 0; element_index < items_length; On 2015/11/17 at 21:55:28, michaeln wrote: > would a simpler loop construct work here and also in the ForEach method? You might be able to get rid of some locals. > > for (const DataElement& element : elements) Yes in this one, but in the ForEach I need the index for a lot of places. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:305: for (; memory_left > 0; memory_left -= segment_size) { On 2015/11/17 at 21:55:28, michaeln wrote: > can this be done with modulo math w/o looping? > > v.insert(v.begin(), total_memory_size / max_segment_size, max_segment_size); > v.push_back(total_memory_size % max_segment_size); Sure. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:55: struct MemoryItemRequest { On 2015/11/17 at 21:55:28, michaeln wrote: > Using 'Memory' in this struct name threw me off a little since it's used for both all xfer types (file, sharedmem, inlineipc). The src in the renderer is memory but the dest in the browser is not necessarily. Idk if its worth word smithing, DataRequest? I mean, a request for an item that's memory in the renderer (as apposed to a file item or otherwise). I added renderer to clarify. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:57: size_t browser_item_index; On 2015/11/17 at 21:55:29, michaeln wrote: > it's not real clear what this is an index into, builder_item_index?, at least worth a comment Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:68: class BlobSegmentVisitor { On 2015/11/17 at 21:55:28, michaeln wrote: > can this be private? Sure. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:83: void Initialize(Mode mode, On 2015/11/17 at 21:55:28, michaeln wrote: > About the packaging of this logic, feels like more of a function than a class? A function that computes the requests that should be made for a blob's elements given size constraints. It also seeds a builder with future elements waiting to be populated with the requested data. > > (a briefer comment like that at the top of the class might help) > > ResultType ComputeDataRequestsNeeded(elements, size_constraints, &data_requests_to_make, &seeded_builder); Well, the main idea of this class is that it's the data class for storing our requests and builder, like we discussed a while ago. I added a comment to the top like you described, and to this method. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:93: std::vector<uint64_t>& file_handles() { return file_handles_; } On 2015/11/17 at 21:55:28, michaeln wrote: > The comments and member names make it sound like the strategy class returns actual handles. Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:96: // with the first size, it will always be greater than the later sizes) On 2015/11/17 at 21:55:28, michaeln wrote: > comment could be simpler, sizes are sorted from largest to smallest. are the file_handle_sizes sorted too? Not intentionally. These are the sizes of all of the file handles and shared memory handles that we use. We can be creating/sending multiple handles at once. They aren't constrained to be sorted by size, that's just how we create them in this algorithm. We refer to these handles by index in our requests. I simplified the comment and added it to file handles as well. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:129: static void CreateHandleSizes(SizeType total_memory_size, On 2015/11/17 at 21:55:28, michaeln wrote: > naming suggestion: ComputeHandleSizes to avoid having CreateHandle in the name since the class does not create handles Done. https://codereview.chromium.org/1098853003/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:135: std::vector<uint64_t> file_handles_; On 2015/11/17 at 21:55:28, michaeln wrote: > naming suggestions: append sizes_ to these names Done.
didn't have time to review this today, sending some incomplete comments though. https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:21: constexpr uint64_t kTestBlobStorageMaxFileSizeBytes = 100; constexptr is banned (https://chromium-cpp.appspot.com/#core-blacklist) https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:23: static void PopulateBytes(char* bytes, size_t start_num, size_t length) { nit: no need for static inside anon namespace https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:179: const size_t kSecondBlockByte = 19; Are k{First,Second}BlockByte used as test byte data? Why we use size_t here? https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:184: char* data = new char[kSize]; nit: given that kSize is small we should be able to just stack-allocate this
https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:21: constexpr uint64_t kTestBlobStorageMaxFileSizeBytes = 100; On 2015/11/19 at 15:22:25, kinuko wrote: > constexptr is banned (https://chromium-cpp.appspot.com/#core-blacklist) Done. https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:23: static void PopulateBytes(char* bytes, size_t start_num, size_t length) { On 2015/11/19 at 15:22:25, kinuko wrote: > nit: no need for static inside anon namespace Done. https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:179: const size_t kSecondBlockByte = 19; On 2015/11/19 at 15:22:25, kinuko wrote: > Are k{First,Second}BlockByte used as test byte data? Why we use size_t here? Whoops, they should be char. https://codereview.chromium.org/1098853003/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:184: char* data = new char[kSize]; On 2015/11/19 at 15:22:25, kinuko wrote: > nit: given that kSize is small we should be able to just stack-allocate this Done.
Still on the way, slowly reviewing... https://codereview.chromium.org/1098853003/diff/340001/content/browser/blob_s... File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/340001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:23: void PopulateBytes(char* bytes, size_t start_num, size_t length) { Probably start_num should be of char type too. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:49: BlobBuildingState* state_ptr = new BlobBuildingState(); nit: I would make this scoped_ptr<BlobBuildingState> state here, get state_ptr by state.get() and pass state in the next line. One extra line but new-ing rawptr makes me feel a bit nervous. (Otherwise I don't think you need .Pass() below) https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:78: // For testing use only. Must be called before BuildBlobAsync. nit: BuildBlobAsync -> StartBuildingBlob? https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:175: error_ = BlobAsyncTransportStrategy::ERROR_NONE; I think I've once asked this but is an instance of this class going to be used multiple times? The current usage doesn't look so, and if that's not the case resetting everything in initialize feels a bit wasting. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:204: return; nit: prefer early return (handle error case first) in general https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:215: } nit: let's add one empty line here https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:300: std::vector<SizeType>* segment_sizes) { nit: probably resize the segment_sizes first before inserting for micro-optimization
https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:78: // For testing use only. Must be called before BuildBlobAsync. On 2015/11/20 at 15:19:42, kinuko wrote: > nit: BuildBlobAsync -> StartBuildingBlob? Done. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:175: error_ = BlobAsyncTransportStrategy::ERROR_NONE; On 2015/11/20 at 15:19:42, kinuko wrote: > I think I've once asked this but is an instance of this class going to be used multiple times? The current usage doesn't look so, and if that's not the case resetting everything in initialize feels a bit wasting. Sounds good, I'll add DCHECKs then. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:204: return; On 2015/11/20 at 15:19:42, kinuko wrote: > nit: prefer early return (handle error case first) in general Done. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:215: } On 2015/11/20 at 15:19:42, kinuko wrote: > nit: let's add one empty line here Done. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:300: std::vector<SizeType>* segment_sizes) { On 2015/11/20 at 15:19:42, kinuko wrote: > nit: probably resize the segment_sizes first before inserting for micro-optimization Done. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:300: std::vector<SizeType>* segment_sizes) { On 2015/11/20 at 15:19:42, kinuko wrote: > nit: probably resize the segment_sizes first before inserting for micro-optimization Done.
https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:51: // Note: The builder given to the 'done' callback is destructed immediately would passing the builder to the completion callback as const ref work? probably it needs to be mutable, but just checking https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:52: builder->AppendFutureFile(segment_offset, size); AppendFuture and PopulateFuture, nice choice for names. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:72: // that cross file or blob boundaries. when are blob boundaries crossed? https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:121: builder->AppendFutureData(current_item_size); what if current_item_size is 0? https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:141: size_t current_storage_item; naminga this as current_item_index might help https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:163: void BlobAsyncTransportStrategy::Initialize( this method reads a lot nicer now, thnx! https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:196: // First, handle the case where we go to disk. redundant comment? https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:144: // would be used to partition a block of total_memory_size. It's easier and clearer to read the code than the comment. Method might be simple enough to not need a comment at all. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:153: // can be size_t, but same as file handles so we can use the same methods. This comment doesn't seem useful. I think its referring to a slightly different design that was not taken. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:129: old_element = nullptr; Not sure you need to do this or comment about it, the ptr is valid right up till the return statement on line 134. https://codereview.chromium.org/1098853003/diff/380001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/380001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:92: // Bad IPC, so we delete our record and ignore. We should plumb these back to the caller so the ReceivedBadMessage function can get called. You said the intent for this class is one instance per-child process? A this->bad_message_callback_ could work, or a return value, or a BAD_MESSAGE code delivered to the cancel_callback_...
https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:49: BlobBuildingState* state_ptr = new BlobBuildingState(); On 2015/11/20 at 15:19:42, kinuko wrote: > nit: I would make this scoped_ptr<BlobBuildingState> state here, get state_ptr by state.get() and pass state in the next line. One extra line but new-ing rawptr makes me feel a bit nervous. > > (Otherwise I don't think you need .Pass() below) Done. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:51: // Note: The builder given to the 'done' callback is destructed immediately On 2015/11/21 at 00:59:45, michaeln wrote: > would passing the builder to the completion callback as const ref work? probably it needs to be mutable, but just checking Yeah, I can switch this to const ref everywhere. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:72: // that cross file or blob boundaries. On 2015/11/21 at 00:59:45, michaeln wrote: > when are blob boundaries crossed? Blob reference boundaries. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:121: builder->AppendFutureData(current_item_size); On 2015/11/21 at 00:59:45, michaeln wrote: > what if current_item_size is 0? Fixed, and added test. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:196: // First, handle the case where we go to disk. On 2015/11/21 at 00:59:45, michaeln wrote: > redundant comment? Done. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:144: // would be used to partition a block of total_memory_size. On 2015/11/21 at 00:59:46, michaeln wrote: > It's easier and clearer to read the code than the comment. Method might be simple enough to not need a comment at all. Done. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:153: // can be size_t, but same as file handles so we can use the same methods. On 2015/11/21 at 00:59:45, michaeln wrote: > This comment doesn't seem useful. I think its referring to a slightly different design that was not taken. whoops, yeah you're right. https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/1098853003/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:129: old_element = nullptr; On 2015/11/21 at 00:59:46, michaeln wrote: > Not sure you need to do this or comment about it, the ptr is valid right up till the return statement on line 134. Done. https://codereview.chromium.org/1098853003/diff/380001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/380001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:92: // Bad IPC, so we delete our record and ignore. On 2015/11/21 at 00:59:46, michaeln wrote: > We should plumb these back to the caller so the ReceivedBadMessage function can get called. You said the intent for this class is one instance per-child process? A this->bad_message_callback_ could work, or a return value, or a BAD_MESSAGE code delivered to the cancel_callback_... That's a function? What's that? I thought we just ignored bad IPCs.
Some more comments. (Looks like some are dup of michael's comment) https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:48: DCHECK(async_blob_map_.find(uuid) == async_blob_map_.end()); Will this method (StartBuildingBlob) called upon IPC from renderer? I think we want to make sure (not just DCHECK) uuid is not a dup, maybe within this method or before calling this method in the hookup patch, and signal BadMessageReceived if that's the case. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:62: /* disk_space_left */ 0, memory_available, uuid, descriptions); nit: I think more common way to write this is to put the comment later, i.e. 0 /* disk_space_left */ https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:76: << state_ptr->transport_strategy.error(); BadMessageReceived? https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:76: << state_ptr->transport_strategy.error(); Hm Ok. I think probably this method (StartBuildingBlob) could return a boolean (or enum) to indicate if it could start building a blog successfully, and return false if async_blob_map_ already has the uuid or it falls into this case. (When browser receives a bad IPC from a renderer we usually signal BadMessageReceived and kill the renderer, so that browser can continue working even if a renderer was compromised/broken) https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:94: async_blob_map_.erase(state_it); Same here. (And below) https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:52: // after the callback is run. nit: this comment's probably no longer necessary/appropriate as now we pass it by const-ref and it's non-copyable? https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:60: const std::vector<uint64_t>&)> request_memory, nit: I think we usually pass callbacks by const ref https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:64: bool IsBuildingBlob(const std::string& uuid) const { doesn't seem used in this CL https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:74: void StopBuildingBlob(const std::string& uuid); ditto, doesn't seem to be used in this CL https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:111: typedef base::ScopedPtrMap<std::string, scoped_ptr<BlobBuildingState>> nit: ScopedPtrMap is now deprecated, can use map<scoped_ptr> instead (crbug.com/554291)
https://codereview.chromium.org/1098853003/diff/380001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/380001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:92: // Bad IPC, so we delete our record and ignore. On 2015/11/23 20:07:02, dmurph wrote: > On 2015/11/21 at 00:59:46, michaeln wrote: > > We should plumb these back to the caller so the ReceivedBadMessage function > can get called. You said the intent for this class is one instance per-child > process? A this->bad_message_callback_ could work, or a return value, or a > BAD_MESSAGE code delivered to the cancel_callback_... > > That's a function? What's that? I thought we just ignored bad IPCs. Yes, its a content lib function which uma stats receipt of the bad message and terminates the child process and log(error)s what happened too. See bad_message.h and .cc https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:72: // that cross file reference or blob reference boundaries. comment nit: I think i understand the case your trying to describe, it involves a file element or blob element that splits two memory elements. Is that right? But in reading this comment, i see "cross blob" and start to wonder... how the heck can data from two different blobs get intermingled in this class? That's a distraction. Given the variety of element types, it's obvious that there can be non-data elements sitting in-between memory elements. The second sentence did not help to clarify things, it hindered understanding (at least for me). It hurt more than it helped. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:126: current_storage_item++; nit: another plug for current_item_index as the name to harmonize with current_item_size https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:142: SizeType current_item_size; It looks like current_item_size and storage_element_offset always contain the same value? Can we remove one of them? https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:27: // generate data requests for that blob's memory and a seed a BlobDataBuilder typo: and seed a https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:35: // This involves two transformations: comment nit: I want to encourage you to be more frugal and more concise in your comments. The comment i suggested earlier has been added to the already lenghty comments making them even longer. Here's another cut at brevity... The Strategy computes two things: 1) How to store the blob data in the browser process: in memory or on disk. 2) How to transport the data from the renderer: ipc payload, shared memory, or file handles. // This can simply mirror the renderer representation, or it can be instead // pointing to transport representation with offsets and sizes. What is that telling me and why do i need to know it? // The initial implementation of this is simple, but the protocol allows any // sort of flexibility, as everything has an offset and size specified. Ditto. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:43: // The following information is generated: Maybe remove this block from the class comment and use them to describe the corresponding getter method. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:90: // In the current algorithm, the file sizes will always be sorted from largest If the sort order is not part of the contract, probably dont need to comment on it.
https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:48: DCHECK(async_blob_map_.find(uuid) == async_blob_map_.end()); On 2015/11/24 at 15:38:16, kinuko wrote: > Will this method (StartBuildingBlob) called upon IPC from renderer? I think we want to make sure (not just DCHECK) uuid is not a dup, maybe within this method or before calling this method in the hookup patch, and signal BadMessageReceived if that's the case. Sort of, we have a layer in between which keeps track of the UUIDs. Since we're going to be doing a map lookup anyways here, I'll return a bool from these methods so we can call BadMessageReceived. There's a blob_dispatcher_host.h file in the hookup patch which sends all IPCs to blob_storage_host, which does all of the uuid filtering. It's probably good to populate up the bad IPC issues though so we can call that. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:62: /* disk_space_left */ 0, memory_available, uuid, descriptions); On 2015/11/24 at 15:38:16, kinuko wrote: > nit: I think more common way to write this is to put the comment later, i.e. > > 0 /* disk_space_left */ Done. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:76: << state_ptr->transport_strategy.error(); On 2015/11/24 at 15:38:16, kinuko wrote: > Hm Ok. I think probably this method (StartBuildingBlob) could return a boolean (or enum) to indicate if it could start building a blog successfully, and return false if async_blob_map_ already has the uuid or it falls into this case. > > (When browser receives a bad IPC from a renderer we usually signal BadMessageReceived and kill the renderer, so that browser can continue working even if a renderer was compromised/broken) See above answer, I'll do the bool to signal bad IPC. We are generally protected (other than programmer error) for UUID clashes by the blob_storage_host, but otherwise we should populate this up. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:94: async_blob_map_.erase(state_it); On 2015/11/24 at 15:38:16, kinuko wrote: > Same here. (And below) Done. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:52: // after the callback is run. On 2015/11/24 at 15:38:16, kinuko wrote: > nit: this comment's probably no longer necessary/appropriate as now we pass it by const-ref and it's non-copyable? Nice catch, thanks. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:60: const std::vector<uint64_t>&)> request_memory, On 2015/11/24 at 15:38:16, kinuko wrote: > nit: I think we usually pass callbacks by const ref ok https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:64: bool IsBuildingBlob(const std::string& uuid) const { On 2015/11/24 at 15:38:16, kinuko wrote: > doesn't seem used in this CL removed. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:74: void StopBuildingBlob(const std::string& uuid); On 2015/11/24 at 15:38:17, kinuko wrote: > ditto, doesn't seem to be used in this CL Tested. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:111: typedef base::ScopedPtrMap<std::string, scoped_ptr<BlobBuildingState>> On 2015/11/24 at 15:38:16, kinuko wrote: > nit: ScopedPtrMap is now deprecated, can use map<scoped_ptr> instead (crbug.com/554291) Done.
https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:72: // that cross file reference or blob reference boundaries. On 2015/11/24 at 21:58:36, michaeln wrote: > comment nit: I think i understand the case your trying to describe, it involves a file element or blob element that splits two memory elements. Is that right? > > But in reading this comment, i see "cross blob" and start to wonder... how the heck can data from two different blobs get intermingled in this class? That's a distraction. > > Given the variety of element types, it's obvious that there can be non-data elements sitting in-between memory elements. The second sentence did not help to clarify things, it hindered understanding (at least for me). It hurt more than it helped. Done. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:126: current_storage_item++; On 2015/11/24 at 21:58:35, michaeln wrote: > nit: another plug for current_item_index as the name to harmonize with current_item_size Done. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:142: SizeType current_item_size; On 2015/11/24 at 21:58:35, michaeln wrote: > It looks like current_item_size and storage_element_offset always contain the same value? Can we remove one of them? mmmmm yeah nice. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:27: // generate data requests for that blob's memory and a seed a BlobDataBuilder On 2015/11/24 at 21:58:36, michaeln wrote: > typo: and seed a Done. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:35: // This involves two transformations: On 2015/11/24 at 21:58:36, michaeln wrote: > comment nit: I want to encourage you to be more frugal and more concise in your comments. The comment i suggested earlier has been added to the already lenghty comments making them even longer. Here's another cut at brevity... > > The Strategy computes two things: > 1) How to store the blob data in the browser process: in memory or on disk. > 2) How to transport the data from the renderer: ipc payload, shared memory, or file handles. > > // This can simply mirror the renderer representation, or it can be instead > // pointing to transport representation with offsets and sizes. > > What is that telling me and why do i need to know it? > > // The initial implementation of this is simple, but the protocol allows any > // sort of flexibility, as everything has an offset and size specified. > > Ditto. Done. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:43: // The following information is generated: On 2015/11/24 at 21:58:36, michaeln wrote: > Maybe remove this block from the class comment and use them to describe the corresponding getter method. Done. https://codereview.chromium.org/1098853003/diff/400001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:90: // In the current algorithm, the file sizes will always be sorted from largest On 2015/11/24 at 21:58:36, michaeln wrote: > If the sort order is not part of the contract, probably dont need to comment on it. Done.
lgtm, but what should we do about not being able to distinguish between a bad ipc and a prior memory error? https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:255: EXPECT_FALSE(BuildBlobAsync(descriptions, 5010)); tests for other 'bad ipc' cases for start() and onmemresponse() would be good https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:54: async_blob_map_[uuid] = std::move(state); fancy :) https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:89: return false; I'm not sure this one is always indicative of a bad ipc? A previous mem response that results in a memory error will have removed the entry from the map with CancelAndCleanup(). https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:34: // the renderer. short-n-sweet... thnx!
looks almost good... https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:23: void PopulateBytes(char* bytes, size_t start_num, size_t length) { start_num seems to be always 0? It looks we don't need this three-param PopulateBytes but only need two-param version? https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:94: EXPECT_EQ(*matching_builder_, builder); nit: could we add a short comment to note that this also compares internal data items? https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:100: const std::vector<base::SharedMemoryHandle>& sms, nit: what sms stands for? smh if it stands for SharedMemoryHandle? https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... File content/browser/blob_storage/blob_async_transport_strategy_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:15: const char* kFakeBlobUUID = "fakeBlob"; why we use both std::string and const char* in a mixed way? (actually I think we prefer const char[] in general for static string constants, though it's not that strict in test files) https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:17: static void AddMemoryItem(size_t length, std::vector<DataElement>* out) { nit: no need for static in anon namespace (here and below) https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:376: // one item. nit: weird line-breaking https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:399: // Same, but with 2 items and a blob in-between. nit: it feels these two blocks could be in different tests https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:51: return false; nit: is it not necessary to do this check before line 37? https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:255: DCHECK(state.get()); nit: this DCHECK and similar ones on line 262, 264 are probably not that useful, if they're null it'll just crash on the next line (and we'll get the same stack trace) https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:43: // After this method is called, the following can happen: nit: "either one of the followings can happen" ? https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:184: // See if we have enough memory nit: finish comment with period https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:62: // blobs. nit: maybe also note that it is not valid to call this twice https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:73: std::vector<uint64_t>& file_handle_sizes() { return file_handle_sizes_; } nit: do we really want/need to return non-const version here and below? https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:15: const char* kFuturePopulatingFilePath = "kFakeFilenameToBeChanged"; nit: const char kFuturePopulatingFilePath[] = ... This magic constant string is tested in blob_async_transport_strategy_unittest.cc without referring each other which looked very cryptic in the first look. Could we make this maybe static class member so that the test can refer the same constant, or we should at least add a comment in the test so that what we're comparing with.
I might have to modify the visitor pure virtual to make it work on Windows. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:23: void PopulateBytes(char* bytes, size_t start_num, size_t length) { On 2015/11/25 at 16:08:17, kinuko wrote: > start_num seems to be always 0? > It looks we don't need this three-param PopulateBytes but only need two-param version? Done. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:94: EXPECT_EQ(*matching_builder_, builder); On 2015/11/25 at 16:08:17, kinuko wrote: > nit: could we add a short comment to note that this also compares internal data items? Done. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:100: const std::vector<base::SharedMemoryHandle>& sms, On 2015/11/25 at 16:08:17, kinuko wrote: > nit: what sms stands for? smh if it stands for SharedMemoryHandle? made non-acronyms. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_builder_host_unittest.cc:255: EXPECT_FALSE(BuildBlobAsync(descriptions, 5010)); On 2015/11/24 at 23:19:40, michaeln wrote: > tests for other 'bad ipc' cases for start() and onmemresponse() would be good Done. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... File content/browser/blob_storage/blob_async_transport_strategy_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:15: const char* kFakeBlobUUID = "fakeBlob"; On 2015/11/25 at 16:08:17, kinuko wrote: > why we use both std::string and const char* in a mixed way? > > (actually I think we prefer const char[] in general for static string constants, though it's not that strict in test files) Done. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:17: static void AddMemoryItem(size_t length, std::vector<DataElement>* out) { On 2015/11/25 at 16:08:17, kinuko wrote: > nit: no need for static in anon namespace (here and below) Done. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:376: // one item. On 2015/11/25 at 16:08:17, kinuko wrote: > nit: weird line-breaking Done. https://codereview.chromium.org/1098853003/diff/440001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:399: // Same, but with 2 items and a blob in-between. On 2015/11/25 at 16:08:17, kinuko wrote: > nit: it feels these two blocks could be in different tests Done. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:51: return false; On 2015/11/25 at 16:08:17, kinuko wrote: > nit: is it not necessary to do this check before line 37? Good catch. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:54: async_blob_map_[uuid] = std::move(state); On 2015/11/24 at 23:19:40, michaeln wrote: > fancy :) :) https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:89: return false; On 2015/11/24 at 23:19:40, michaeln wrote: > I'm not sure this one is always indicative of a bad ipc? A previous mem response that results in a memory error will have removed the entry from the map with CancelAndCleanup(). Done. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.cc:255: DCHECK(state.get()); On 2015/11/25 at 16:08:17, kinuko wrote: > nit: this DCHECK and similar ones on line 262, 264 are probably not that useful, if they're null it'll just crash on the next line (and we'll get the same stack trace) makes sense. Done. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_builder_host.h (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_builder_host.h:43: // After this method is called, the following can happen: On 2015/11/25 at 16:08:17, kinuko wrote: > nit: "either one of the followings can happen" ? Clarified. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:184: // See if we have enough memory On 2015/11/25 at 16:08:17, kinuko wrote: > nit: finish comment with period Done. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:34: // the renderer. On 2015/11/24 at 23:19:40, michaeln wrote: > short-n-sweet... thnx! np https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:62: // blobs. On 2015/11/25 at 16:08:17, kinuko wrote: > nit: maybe also note that it is not valid to call this twice Done. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:73: std::vector<uint64_t>& file_handle_sizes() { return file_handle_sizes_; } On 2015/11/25 at 16:08:17, kinuko wrote: > nit: do we really want/need to return non-const version here and below? Good catch. We don't need to anymore because we are just describing the sizes. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:15: const char* kFuturePopulatingFilePath = "kFakeFilenameToBeChanged"; On 2015/11/25 at 16:08:17, kinuko wrote: > nit: const char kFuturePopulatingFilePath[] = ... > > This magic constant string is tested in blob_async_transport_strategy_unittest.cc without referring each other which looked very cryptic in the first look. > > Could we make this maybe static class member so that the test can refer the same constant, or we should at least add a comment in the test so that what we're comparing with. Done. I like the static class member idea.
https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:73: std::vector<uint64_t>& file_handle_sizes() { return file_handle_sizes_; } On 2015/11/25 16:08:17, kinuko wrote: > nit: do we really want/need to return non-const version here and below? This gets back to an earlier comment about how he packaging of this class feels a little awkward. Mutable copies of internal data members are needed because this is more struct than encapsulated object. The consumer needs to mutate the internals directly, I'm not sure if the handle sizes need to be mutable, but the builder certainly does. To me, this class is more of a function that computes sizes and preps a builder, the logic is an impl detail of the asyncbuilderhost start method and the results are a part of the asyncbuilding state. The function/logic can be easily tested either way, as a separate class or as an integral part of the builderhost. https://codereview.chromium.org/1098853003/diff/440001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:98: Error error() const { return error_; } Given how this data member is used, maybe have Initialize() return this value instead of having it as a data member.
Can you guys take one more look at blob_async_transport_strategy.cc? After talking to Jeffrey Yasskin about the windows issue, I changed the Visitor code to be exclusively in the 'cc' file, and made it templated instead of inheriting a base class. This should work now in windows, and it makes the .h file smaller.
still lgtm https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:117: std::vector<uint64_t> file_handle_sizes_; Might be more trouble than its worth to have the two vectors be of two different types? We get to decide the constraints, the max_file_size param could be expressed in terms of size_t so a vector<size_t> would be fine. Could make for a smaller binary too since no template expansion would be involved.
https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:117: std::vector<uint64_t> file_handle_sizes_; On 2015/11/25 at 23:34:54, michaeln wrote: > Might be more trouble than its worth to have the two vectors be of two different types? We get to decide the constraints, the max_file_size param could be expressed in terms of size_t so a vector<size_t> would be fine. Could make for a smaller binary too since no template expansion would be involved. I don't see why we need to add that restriction, it's not like we have a ton of template expansions. Just two. And this is correct forever! What about the files that need to be > size_t big? Then again, 256k is enough for anybody.
okay... I think this lgtm https://codereview.chromium.org/1098853003/diff/520001/content/browser/blob_s... File content/browser/blob_storage/blob_async_transport_strategy_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/520001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:46: // and we save to one file. (size < max_file_size) Um, is this comment right? Also we no longer seem to use the name 'max_blob_in_memory_size', which makes the comment confusing. Can you update? https://codereview.chromium.org/1098853003/diff/520001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:74: // and we save to one file. (size < max_file_size) ditto, here and below. comment looks stale to me? https://codereview.chromium.org/1098853003/diff/520001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/520001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:211: // shared memory blocks, of <= kMaxSharedMemorySize) kMaxFoo constant names are not defined in this file scope and are confusing. Could we instead use parameter names, e.g. |max_file_size|, |max_ipc_memory_size| etc? Also 'this is 150KB' should be 'say 150KB' https://codereview.chromium.org/1098853003/diff/520001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:327: bool hasExtraSegment = (total_memory_size % max_segment_size) > 0; nit: no camelCaseNaming in chromium, should be has_extra_segment
https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:117: std::vector<uint64_t> file_handle_sizes_; On 2015/11/26 00:37:07, dmurph wrote: > On 2015/11/25 at 23:34:54, michaeln wrote: > > Might be more trouble than its worth to have the two vectors be of two > different types? We get to decide the constraints, the max_file_size param could > be expressed in terms of size_t so a vector<size_t> would be fine. Could make > for a smaller binary too since no template expansion would be involved. > > I don't see why we need to add that restriction, it's not like we have a ton of > template expansions. Just two. And this is correct forever! What about the > files that need to be > size_t big? Then again, 256k is enough for anybody. Since the source of the data for these files is blocks of memory in the renderer process is size_t all the range that would ever be needed? 256k?
https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:117: std::vector<uint64_t> file_handle_sizes_; On 2015/11/30 at 21:07:26, michaeln wrote: > On 2015/11/26 00:37:07, dmurph wrote: > > On 2015/11/25 at 23:34:54, michaeln wrote: > > > Might be more trouble than its worth to have the two vectors be of two > > different types? We get to decide the constraints, the max_file_size param could > > be expressed in terms of size_t so a vector<size_t> would be fine. Could make > > for a smaller binary too since no template expansion would be involved. > > > > I don't see why we need to add that restriction, it's not like we have a ton of > > template expansions. Just two. And this is correct forever! What about the > > files that need to be > size_t big? Then again, 256k is enough for anybody. > > Since the source of the data for these files is blocks of memory in the renderer process is size_t all the range that would ever be needed? > > 256k? It was a joke about Bill Gates haha. I was thinking about if someone had one large array with a ton of memory and then passed that array many times. https://codereview.chromium.org/1098853003/diff/520001/content/browser/blob_s... File content/browser/blob_storage/blob_async_transport_strategy_unittest.cc (right): https://codereview.chromium.org/1098853003/diff/520001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:46: // and we save to one file. (size < max_file_size) On 2015/11/26 at 04:58:17, kinuko wrote: > Um, is this comment right? Also we no longer seem to use the name 'max_blob_in_memory_size', which makes the comment confusing. Can you update? Whoops, yes, thanks. https://codereview.chromium.org/1098853003/diff/520001/content/browser/blob_s... content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:74: // and we save to one file. (size < max_file_size) On 2015/11/26 at 04:58:17, kinuko wrote: > ditto, here and below. comment looks stale to me? It was a little stale, thanks. https://codereview.chromium.org/1098853003/diff/520001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/520001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:211: // shared memory blocks, of <= kMaxSharedMemorySize) On 2015/11/26 at 04:58:17, kinuko wrote: > kMaxFoo constant names are not defined in this file scope and are confusing. Could we instead use parameter names, e.g. |max_file_size|, |max_ipc_memory_size| etc? > > Also 'this is 150KB' should be 'say 150KB' Done. https://codereview.chromium.org/1098853003/diff/520001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:327: bool hasExtraSegment = (total_memory_size % max_segment_size) > 0; On 2015/11/26 at 04:58:17, kinuko wrote: > nit: no camelCaseNaming in chromium, should be has_extra_segment Whoops, fixed.
https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.h (right): https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.h:117: std::vector<uint64_t> file_handle_sizes_; On 2015/11/30 21:33:28, dmurph wrote: > On 2015/11/30 at 21:07:26, michaeln wrote: > > On 2015/11/26 00:37:07, dmurph wrote: > > > On 2015/11/25 at 23:34:54, michaeln wrote: > > > > Might be more trouble than its worth to have the two vectors be of two > > > different types? We get to decide the constraints, the max_file_size param > could > > > be expressed in terms of size_t so a vector<size_t> would be fine. Could > make > > > for a smaller binary too since no template expansion would be involved. > > > > > > I don't see why we need to add that restriction, it's not like we have a ton > of > > > template expansions. Just two. And this is correct forever! What about the > > > files that need to be > size_t big? Then again, 256k is enough for anybody. > > > > Since the source of the data for these files is blocks of memory in the > renderer process is size_t all the range that would ever be needed? > > > > 256k? > > It was a joke about Bill Gates haha. > > I was thinking about if someone had one large array with a ton of memory and > then passed that array many times. I guess its true that crazy api usage can describe a large amount of memory, but what if we say max_file_size_ is < numeric_limit<size_t>::max(), would the blob system ever try to create a file larger than that?
On 2015/11/30 at 21:57:19, michaeln wrote: > https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... > File storage/browser/blob/blob_async_transport_strategy.h (right): > > https://codereview.chromium.org/1098853003/diff/500001/storage/browser/blob/b... > storage/browser/blob/blob_async_transport_strategy.h:117: std::vector<uint64_t> file_handle_sizes_; > On 2015/11/30 21:33:28, dmurph wrote: > > On 2015/11/30 at 21:07:26, michaeln wrote: > > > On 2015/11/26 00:37:07, dmurph wrote: > > > > On 2015/11/25 at 23:34:54, michaeln wrote: > > > > > Might be more trouble than its worth to have the two vectors be of two > > > > different types? We get to decide the constraints, the max_file_size param > > could > > > > be expressed in terms of size_t so a vector<size_t> would be fine. Could > > make > > > > for a smaller binary too since no template expansion would be involved. > > > > > > > > I don't see why we need to add that restriction, it's not like we have a ton > > of > > > > template expansions. Just two. And this is correct forever! What about the > > > > files that need to be > size_t big? Then again, 256k is enough for anybody. > > > > > > Since the source of the data for these files is blocks of memory in the > > renderer process is size_t all the range that would ever be needed? > > > > > > 256k? > > > > It was a joke about Bill Gates haha. > > > > I was thinking about if someone had one large array with a ton of memory and > > then passed that array many times. > > I guess its true that crazy api usage can describe a large amount of memory, but what if we say max_file_size_ is < numeric_limit<size_t>::max(), would the blob system ever try to create a file larger than that? Well, each individual file wouldn't be larger than that, but the total memory would be. For 32 bit systems, that would be anything > 4 gigs (4294967295 / (1024^4)
> Well, each individual file wouldn't be larger than that, but the total memory > would be. For 32 bit systems, that would be anything > 4 gigs (4294967295 / > (1024^4) sure, i thought the data member we're talking about describes the size of individual files?
On 2015/11/30 at 22:05:02, michaeln wrote: > > Well, each individual file wouldn't be larger than that, but the total memory > > would be. For 32 bit systems, that would be anything > 4 gigs (4294967295 / > > (1024^4) > > sure, i thought the data member we're talking about describes the size of individual files? Ah, crap, yeah you're right. But we still need the specialization for the element offset. I can also still do a base class w/ impls if you're more happy with that. I consolidated the handle sizes.
i still don't see why the sizetype needs to be paramaterized? https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:261: ForEachWithSegment(blob_item_infos, static_cast<uint64_t>(max_file_size), why is this cast needed, its of type size_t? and if this cast is not needed, why is the numeric type template param needed in ForEachWithSegment? https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:272: ComputeHandleSizes(static_cast<size_t>(total_bytes_size_), i dont think this cast is needed https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:324: size_t total_memory_size, I think this first param should be uint64_t. Since the same memory data element can be added multiple times the total size can exceed size_t range. We'll be spilling directly to disk in that case.
I removed the SizeType and just did uint64_t. Is that what you had in mind? Tell me what you want me to do here haha. https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... File storage/browser/blob/blob_async_transport_strategy.cc (right): https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:261: ForEachWithSegment(blob_item_infos, static_cast<uint64_t>(max_file_size), On 2015/12/01 at 20:33:14, michaeln wrote: > why is this cast needed, its of type size_t? and if this cast is not needed, why is the numeric type template param needed in ForEachWithSegment? because it's waaaaay easier if we cast this to match the the offset type of the total memory size. See line 182 and 183. We would need to do a bunch of checked casting, or just cast the input to SizeType right away. https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:272: ComputeHandleSizes(static_cast<size_t>(total_bytes_size_), On 2015/12/01 at 20:33:14, michaeln wrote: > i dont think this cast is needed with the below change I don't need it. https://codereview.chromium.org/1098853003/diff/560001/storage/browser/blob/b... storage/browser/blob/blob_async_transport_strategy.cc:324: size_t total_memory_size, On 2015/12/01 at 20:33:14, michaeln wrote: > I think this first param should be uint64_t. Since the same memory data element can be added multiple times the total size can exceed size_t range. We'll be spilling directly to disk in that case. Done.
On 2015/12/01 20:44:16, dmurph wrote: > I removed the SizeType and just did uint64_t. Is that what you had in mind? Tell > me what you want me to do here haha. lgtm. The discussion about template voodoo got me looking more closely and then finding ways to simplify is all. Originally this was using template specialization and polymorphism and not so surprisingly was running into build problems.
Ah yeah. Well the original issue was basically a windows compiler visibility bug. I mean, feature. On Tue, Dec 1, 2015, 1:20 PM <michaeln@chromium.org> wrote: > On 2015/12/01 20:44:16, dmurph wrote: > > I removed the SizeType and just did uint64_t. Is that what you had in > > mind? > Tell > > me what you want me to do here haha. > > lgtm. The discussion about template voodoo got me looking more closely and > then > finding ways to simplify is all. Originally this was using template > specialization and polymorphism and not so surprisingly was running into > build > problems. > > https://codereview.chromium.org/1098853003/ > -- 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.
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 Link to the patchset: https://codereview.chromium.org/1098853003/#ps620001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098853003/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1098853003/620001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_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 Link to the patchset: https://codereview.chromium.org/1098853003/#ps640001 (title: "Removed Bad Test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098853003/640001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1098853003/640001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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 Link to the patchset: https://codereview.chromium.org/1098853003/#ps660001 (title: "Missed duplicate invalid test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098853003/660001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1098853003/660001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098853003/660001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1098853003/660001
Message was sent while issue was closed.
Committed patchset #34 (id:660001)
Message was sent while issue was closed.
Description was changed from ========== [BlobAsync] Patch 4: Browser Classes & Logic. This patch encompasses all browser classes and logic for blob async transportation. This is a no-op. Patches: 1: https://codereview.chromium.org/1287303002 (committed) 2: https://codereview.chromium.org/1288373002 (committed) 3: https://codereview.chromium.org/1292523002 (committed) 4: https://codereview.chromium.org/1098853003 Hookup: https://codereview.chromium.org/1234813004 See https://bit.ly/BlobStorageRefactor BUG=375297 ========== to ========== [BlobAsync] Patch 4: Browser Classes & Logic. This patch encompasses all browser classes and logic for blob async transportation. This is a no-op. Patches: 1: https://codereview.chromium.org/1287303002 (committed) 2: https://codereview.chromium.org/1288373002 (committed) 3: https://codereview.chromium.org/1292523002 (committed) 4: https://codereview.chromium.org/1098853003 Hookup: https://codereview.chromium.org/1234813004 See https://bit.ly/BlobStorageRefactor BUG=375297 Committed: https://crrev.com/233c81baddf00838335bb712a3f8cd67fd327e71 Cr-Commit-Position: refs/heads/master@{#362802} ==========
Message was sent while issue was closed.
Patchset 34 (id:??) landed as https://crrev.com/233c81baddf00838335bb712a3f8cd67fd327e71 Cr-Commit-Position: refs/heads/master@{#362802} |
