Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516713002:
[BlobStorage] Implementing disk. (Closed)
Description[BlobStorage] Enabling disk storage and paging.
Adds support for saving blobs to disk by either paging them to disk
or sending files directly to the renderer.
Note: Disk is turned off by default (limit=0). So other than tests
that explicitly set a higher disk limit, this patch should be a NOOP.
The next patch will configure this limit (and the current RAM limit)
by the OS, disk size, and available RAM.
R=michaeln,kinuko
BUG=375297
Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a
Committed: https://crrev.com/07725e0f2bd75b9649ff9b4a10096a281e56d998
Cr-Original-Commit-Position: refs/heads/master@{#436415}
Cr-Commit-Position: refs/heads/master@{#436485}
Patch Set 1 #Patch Set 2 : windows fix #Patch Set 3 : windows fix, documentation, adding browsertest #Patch Set 4 : browsertest #Patch Set 5 : test & windows fix #
Total comments: 8
Patch Set 6 : comments, and hopefully browsertest cleanup #
Total comments: 48
Patch Set 7 : comments, and attempt at force flushing all tasks so Windows/Android test works #
Total comments: 20
Patch Set 8 : comments #Patch Set 9 : ERROR LOGGING for windows debugging #Patch Set 10 : file flushing, stack track on reader error #
Total comments: 24
Patch Set 11 : narrowing down windows error - flush and then get modification time (sorry still logging in here) #Patch Set 12 : windows debugging & victor comments #
Total comments: 10
Patch Set 13 : flush maybe worked? Adding it in paging spot. #
Total comments: 12
Patch Set 14 : Comments, removed most logging (kept some dlogs at important places), windows errors fixed, hooray! #Patch Set 15 : added <numeric> include #Patch Set 16 : disabled disk, next patch will configure available disk space #Patch Set 17 : fixed syntax error #Patch Set 18 : fixed tests that assumed disk quota #Patch Set 19 : removed cleanup check, as mac doesn't run out event loops #Dependent Patchsets: Messages
Total messages: 114 (88 generated)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== [BlobStorage] Implementing disk. BUG=375297 ========== to ========== [BlobStorage] Enabling disk storage and paging. This patch enables saving blobs to disk by paging and sending files directly to the renderer. BUG=375297 ==========
dmurph@chromium.org changed reviewers: + kinuko@chromium.org, michaeln@chromium.org
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. This patch enables saving blobs to disk by paging and sending files directly to the renderer. BUG=375297 ========== to ========== [BlobStorage] Enabling disk storage and paging. This patch enables saving blobs to disk by paging and sending files directly to the renderer. R=michaeln,kinuko BUG=375297 ==========
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hello! I'm still in the process of adding a browsertest here (having trouble getting it to exit), but this is currently working otherwise. I tried to be a little more clear on the documentation. Let me know what you think! Thanks, Daniel
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Browsertest uploaded!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Haven't looked into test code but the code change itself looks reasonable to me. Will take another look but some initial comments https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... content/browser/blob_storage/blob_storage_context_unittest.cc:675: TEST_F(BlobStorageContextTest, BuildBlobFuzzer) { Fuzzer sounds like it's using fuzzer. Should we rather use a real fuzzer (e.g. base/test/fuzzed_data_provider)? Do we have some rationale why you chose some of the (random'ish) values below or they're just chosen randomly? https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... content/browser/blob_storage/chrome_blob_storage_context.cc:10: nit: unnecessary empty line https://codereview.chromium.org/2516713002/diff/80001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/80001/storage/browser/blob/bl... storage/browser/blob/blob_storage_context.cc:203: // of that and mark is as needing quota. mark is -> mark it ? https://codereview.chromium.org/2516713002/diff/80001/storage/browser/blob/bl... storage/browser/blob/blob_storage_context.cc:542: // The blob can complete during the execution of this line. This comment looks misplaced (must be placed before ReserveMemoryQuota?)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... content/browser/blob_storage/blob_storage_context_unittest.cc:675: TEST_F(BlobStorageContextTest, BuildBlobFuzzer) { On 2016/11/27 14:45:22, kinuko wrote: > Fuzzer sounds like it's using fuzzer. Should we rather use a real fuzzer (e.g. > base/test/fuzzed_data_provider)? Do we have some rationale why you chose some > of the (random'ish) values below or they're just chosen randomly? I wanted to have every combination of blob that is supported - and make sure that we hit the memory barrier so that we have blobs that depend on parent blobs that WERE memory but change to paged files. Since it's fairly specific, I want to keep it as non-fuzzed. I will rename it. https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_st... content/browser/blob_storage/chrome_blob_storage_context.cc:10: On 2016/11/27 14:45:22, kinuko wrote: > nit: unnecessary empty line Done. https://codereview.chromium.org/2516713002/diff/80001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/80001/storage/browser/blob/bl... storage/browser/blob/blob_storage_context.cc:203: // of that and mark is as needing quota. On 2016/11/27 14:45:22, kinuko wrote: > mark is -> mark it ? Done. https://codereview.chromium.org/2516713002/diff/80001/storage/browser/blob/bl... storage/browser/blob/blob_storage_context.cc:542: // The blob can complete during the execution of this line. On 2016/11/27 14:45:22, kinuko wrote: > This comment looks misplaced (must be placed before ReserveMemoryQuota?) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmurph@chromium.org changed reviewers: + mek@chromium.org, pwnall@chromium.org
+mek and +victor for a review, and to for knowledge diffusion (and michael is OOO until Friday)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
looks good https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:129: ASSERT_TRUE(temp_dir_.Delete()); It seems a bit weird to me to explicitly delete a ScopedTempDir when it is about to implicitly get deleted anyway https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:819: // TODO(michaeln): tests for the depcrecated url stuff nit: deprecated https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:44: base::FileEnumerator enumerator(blob_storage_parent, false, nit: I don't like bare true/false without a comment explaining what the true/false means (all these file APIs really shouldn't be using booleans to indicate recursive/non-recursive...) https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:124: context_.reset(new BlobStorageContext(std::move(blob_storage_dir), base::FilePath doesn't seem to have a move constructor, so this just does a copy anyway. Which also means you should probably pass FilePath by const-ref everywhere (or fix FilePath and add move constructors and move assignment operators). https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:387: : memory_controller_(std::move(storage_directory), std::move(file_runner)), nit: std::move(storage_directory) doesn't actually cause a move since storage_directory is a const-ref https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:792: void BlobStorageContext::OnEnoughSpaceForTransportByMemory( If this just calls OnEnoughSpaceForTransportByFiles, should that other method really be named "ByFiles"?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the comments! @kinuko, @michaeln, and @pwnall - This code is in a final state, the test errors are currently caused by not knowing how to force the browsertest to totally cleanup the shell. https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:129: ASSERT_TRUE(temp_dir_.Delete()); On 2016/11/29 22:25:20, Marijn Kruisselbrink wrote: > It seems a bit weird to me to explicitly delete a ScopedTempDir when it is about > to implicitly get deleted anyway I want to avoid the case on Windows when we have dangling directories that don't get deleted - as it doesn't work if anything is in the directory. I want the test to fail if this doesn't work, instead of just logging a warning. https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:819: // TODO(michaeln): tests for the depcrecated url stuff On 2016/11/29 22:25:20, Marijn Kruisselbrink wrote: > nit: deprecated Done. https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:44: base::FileEnumerator enumerator(blob_storage_parent, false, On 2016/11/29 22:25:20, Marijn Kruisselbrink wrote: > nit: I don't like bare true/false without a comment explaining what the > true/false means (all these file APIs really shouldn't be using booleans to > indicate recursive/non-recursive...) Comment added to document argument name. https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:124: context_.reset(new BlobStorageContext(std::move(blob_storage_dir), On 2016/11/29 22:25:20, Marijn Kruisselbrink wrote: > base::FilePath doesn't seem to have a move constructor, so this just does a copy > anyway. Which also means you should probably pass FilePath by const-ref > everywhere (or fix FilePath and add move constructors and move assignment > operators). I'll fix FilePath https://bugs.chromium.org/p/chromium/issues/detail?id=670043 https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:387: : memory_controller_(std::move(storage_directory), std::move(file_runner)), On 2016/11/29 22:25:20, Marijn Kruisselbrink wrote: > nit: std::move(storage_directory) doesn't actually cause a move since > storage_directory is a const-ref Done. https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:792: void BlobStorageContext::OnEnoughSpaceForTransportByMemory( On 2016/11/29 22:25:20, Marijn Kruisselbrink wrote: > If this just calls OnEnoughSpaceForTransportByFiles, should that other method > really be named "ByFiles"? Hm... Yeah I can simplify this by moving the files vector in front of success, so we can use the same callback.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... File content/test/data/blob_storage/blob_creation_and_slicing.html (right): https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:6: // We create < 2000 bytes of data, as that is the max for the browsertest. Instead of having a comment here, how about adding an assertion in the code? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:8: var numRawBlobs = 100; Can you please document the test parameters? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:16: var veryLargeDataSize = 190; How about naming the buffers according to the goals? For example, this could be pagedToDiskDataSize / pagedToDiskData. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:18: veryLargeData.fill(2); Any reason for these values? Are they set up so that the bytes in each file / shmem segment get the same value? If so, can you please document this subtlety in the test? It'd definitely help debugging. If not, can you please write a helper function that fills up an array with pseudo-random values? You can use a simple super-weak function like v[i] = (17 + 23 * i) & 0xFF to get consistent results. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:22: var largeDataSize = 15; sharedMemoryData{Size}? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:28: var smallDataSize = 2; ipcData{Size}? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:32: var i = 0; I think you can use let i inside loops so i is block-scoped. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:79: var i = 0; If you don't want to switch to let, you should at least remove this duplicate var declaration. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:82: var genReader = function(index, d) { This seems to be defined so you can capture i into index. How about using an IIFE at the function's callsite instead? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:103: var data; Can you please define these closer to where you assign them? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:116: origData = largeData; Why originalSize but origData? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... File content/test/data/blob_storage/common.js (right): https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/common.js:44: for (var i = 0; i < a.length; i++) I may not remember this correctly, but I think that you need brackets around the for block, because it's more than one line. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/common.js:68: function ab2str(buf) { arrayBufferToString? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/common.js:69: return String.fromCharCode.apply(null, new Uint8Array(buf)); ArrayBuffer instances tend to contain binary data, right? If so, I think that a more useful representation would be "(new Uint8Array(arrayBuffer)).join(',')" https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_entry.h (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_entry.h:56: // copies. (PENDING_INTERNALS) Based on the description here and in blob_storage_constants.h, a better name for PENDING_INTERNALS seems to be PENDING_REFERENCED_BLOBS. Not tackling the rename in this CL would be fine though. https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:837: if (building_state->copy_quota_request) { Should calling Cancel on BuildingState's members be a method on BuildingState? https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.h (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.h:50: // We are single threaded and should only be used on the IO thread. In Chromium "We are single threaded" -> "The class is not threadsafe"? https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.h:58: // We enable disk support if |file_runner| isn't null. We enable disk support -> disk support is enabled. https://codereview.chromium.org/2516713002/diff/100001/storage/common/blob_st... File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2516713002/diff/100001/storage/common/blob_st... storage/common/blob_storage/blob_storage_constants.h:82: PENDING_INTERNALS = 203, Should this be named PENDING_REFERENCED_BLOBS? I read "internals" as a generic name for undocumented implementation details.
Some more comments. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_flattener_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_flattener_unittest.cc:243: // We're copying item at index 1, 6, and 7. nit: item -> items https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_browsertest.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_browsertest.cc:30: // the actual implementation that lives in the browser side. Ahem. You seem to have copied the code skeleton from IndexedDB browser test? =) https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_browsertest.cc:55: // a #pass or #fail ref. ditto https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:109: void IncrementPointer(size_t* number, BlobStatus status) { nit: well, it's incrementing the pointed value rather than the pointer https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:710: } Could we avoid using nested if for deciding test parameters for readability? Or if we do this could we use a helper method or a lambda that returns an enum or bool value to pick test parameters and use it throughout this test code? https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:722: size += 14; 14u for consistency or use strlen(kTestBlobData) ? https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:731: size_t offset = sizes[i] == 1 ? 0 : i % (source_size - 1); When could sizes[i] could be 1? Could we consistently use source_size for sizes[i] after line 730? https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:734: sizes.push_back(size); When are these sizes used after this line? https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:44: base::FileEnumerator enumerator(blob_storage_parent, /* recursive */ false, nit: false /* recursive */, is probably more common https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:50: success &= base::DeleteFile(name, /* recursive */ true); ditto
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the comments! I uploaded a patch that had some logging for the windows bot problems. But otherwise it's a final patch. PTAL. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... File content/test/data/blob_storage/blob_creation_and_slicing.html (right): https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:8: var numRawBlobs = 100; On 2016/12/01 01:12:13, pwnall wrote: > Can you please document the test parameters? Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:16: var veryLargeDataSize = 190; On 2016/12/01 01:12:13, pwnall wrote: > How about naming the buffers according to the goals? For example, this could be > pagedToDiskDataSize / pagedToDiskData. Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:18: veryLargeData.fill(2); On 2016/12/01 01:12:13, pwnall wrote: > Any reason for these values? Are they set up so that the bytes in each file / > shmem segment get the same value? > > If so, can you please document this subtlety in the test? It'd definitely help > debugging. > > If not, can you please write a helper function that fills up an array with > pseudo-random values? You can use a simple super-weak function like v[i] = (17 + > 23 * i) & 0xFF to get consistent results. Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:22: var largeDataSize = 15; On 2016/12/01 01:12:13, pwnall wrote: > sharedMemoryData{Size}? Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:28: var smallDataSize = 2; On 2016/12/01 01:12:13, pwnall wrote: > ipcData{Size}? Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:32: var i = 0; On 2016/12/01 01:12:13, pwnall wrote: > I think you can use let i inside loops so i is block-scoped. Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:82: var genReader = function(index, d) { On 2016/12/01 01:12:13, pwnall wrote: > This seems to be defined so you can capture i into index. How about using an > IIFE at the function's callsite instead? isn't that what I do? https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:103: var data; On 2016/12/01 01:12:13, pwnall wrote: > Can you please define these closer to where you assign them? Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:116: origData = largeData; On 2016/12/01 01:12:13, pwnall wrote: > Why originalSize but origData? Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... File content/test/data/blob_storage/common.js (right): https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/common.js:44: for (var i = 0; i < a.length; i++) On 2016/12/01 01:12:13, pwnall wrote: > I may not remember this correctly, but I think that you need brackets around the > for block, because it's more than one line. Done. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/common.js:68: function ab2str(buf) { On 2016/12/01 01:12:13, pwnall wrote: > arrayBufferToString? Removed. https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob... content/test/data/blob_storage/common.js:69: return String.fromCharCode.apply(null, new Uint8Array(buf)); On 2016/12/01 01:12:13, pwnall wrote: > ArrayBuffer instances tend to contain binary data, right? If so, I think that a > more useful representation would be "(new Uint8Array(arrayBuffer)).join(',')" Done. https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_entry.h (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_entry.h:56: // copies. (PENDING_INTERNALS) On 2016/12/01 01:12:14, pwnall wrote: > Based on the description here and in blob_storage_constants.h, a better name for > PENDING_INTERNALS seems to be PENDING_REFERENCED_BLOBS. Not tackling the rename > in this CL would be fine though. > Yeah I'll do that in a follow up cl. crbug/670398 https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:837: if (building_state->copy_quota_request) { On 2016/12/01 01:12:14, pwnall wrote: > Should calling Cancel on BuildingState's members be a method on BuildingState? Ah that's cleaner, thanks. https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.h (right): https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.h:50: // We are single threaded and should only be used on the IO thread. In Chromium On 2016/12/01 01:12:14, pwnall wrote: > "We are single threaded" -> "The class is not threadsafe"? Done. https://codereview.chromium.org/2516713002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.h:58: // We enable disk support if |file_runner| isn't null. On 2016/12/01 01:12:14, pwnall wrote: > We enable disk support -> disk support is enabled. Done. https://codereview.chromium.org/2516713002/diff/100001/storage/common/blob_st... File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2516713002/diff/100001/storage/common/blob_st... storage/common/blob_storage/blob_storage_constants.h:82: PENDING_INTERNALS = 203, On 2016/12/01 01:12:14, pwnall wrote: > Should this be named PENDING_REFERENCED_BLOBS? I read "internals" as a generic > name for undocumented implementation details. > crbug.com/670398 https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_flattener_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_flattener_unittest.cc:243: // We're copying item at index 1, 6, and 7. On 2016/12/01 05:10:04, kinuko wrote: > nit: item -> items Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_browsertest.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_browsertest.cc:30: // the actual implementation that lives in the browser side. On 2016/12/01 05:10:04, kinuko wrote: > Ahem. You seem to have copied the code skeleton from IndexedDB browser test? =) Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_browsertest.cc:55: // a #pass or #fail ref. On 2016/12/01 05:10:04, kinuko wrote: > ditto Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:109: void IncrementPointer(size_t* number, BlobStatus status) { On 2016/12/01 05:10:04, kinuko wrote: > nit: well, it's incrementing the pointed value rather than the pointer Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:710: } On 2016/12/01 05:10:04, kinuko wrote: > Could we avoid using nested if for deciding test parameters for readability? Or > if we do this could we use a helper method or a lambda that returns an enum or > bool value to pick test parameters and use it throughout this test code? Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:722: size += 14; On 2016/12/01 05:10:04, kinuko wrote: > 14u for consistency > > or use strlen(kTestBlobData) ? Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:731: size_t offset = sizes[i] == 1 ? 0 : i % (source_size - 1); On 2016/12/01 05:10:04, kinuko wrote: > When could sizes[i] could be 1? > > Could we consistently use source_size for sizes[i] after line 730? Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:734: sizes.push_back(size); On 2016/12/01 05:10:04, kinuko wrote: > When are these sizes used after this line? They aren't removed. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:44: base::FileEnumerator enumerator(blob_storage_parent, /* recursive */ false, On 2016/12/01 05:10:04, kinuko wrote: > nit: > > false /* recursive */, > > is probably more common Done. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:50: success &= base::DeleteFile(name, /* recursive */ true); On 2016/12/01 05:10:05, kinuko wrote: > ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Here are a few more small comments on the browser test. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... File content/test/data/blob_storage/blob_creation_and_slicing.html (right): https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:6: // We create < 2000 bytes of data, as that is the max for the browsertest. 2000 or 3000? https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:28: var savedToDiskData = new Uint8Array(savedToDiskDataSize); How about moving the array allocation into the helper function? https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:115: var origData; The logic here seems to duplicate some of the logic above, where you're generating the blobs. How about creating a blobData array above (line 42), and populating it as you're creating the blobs? https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:144: if (i < numRawBlobs) { If you follow my suggestion above, this whole block goes away. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... File content/test/data/blob_storage/common.js (right): https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/common.js:65: Unnecessary change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
publishing comment responses. Continuing to debug windows. Narrowed it down to the time modified being mismatched. Continuing to debug this. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... File content/test/data/blob_storage/blob_creation_and_slicing.html (right): https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:6: // We create < 2000 bytes of data, as that is the max for the browsertest. On 2016/12/01 23:28:49, pwnall wrote: > 2000 or 3000? Done. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:28: var savedToDiskData = new Uint8Array(savedToDiskDataSize); On 2016/12/01 23:28:49, pwnall wrote: > How about moving the array allocation into the helper function? Done. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:115: var origData; On 2016/12/01 23:28:49, pwnall wrote: > The logic here seems to duplicate some of the logic above, where you're > generating the blobs. > > How about creating a blobData array above (line 42), and populating it as you're > creating the blobs? Done. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:144: if (i < numRawBlobs) { On 2016/12/01 23:28:49, pwnall wrote: > If you follow my suggestion above, this whole block goes away. Done. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... File content/test/data/blob_storage/common.js (right): https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob... content/test/data/blob_storage/common.js:65: On 2016/12/01 23:28:49, pwnall wrote: > Unnecessary change? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
other than various error logging and some more readability nits I think the code itself lgtm. (Might want to take another look after removing error loggings etc but no need to be blocked) https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_browsertest.cc (right): https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_browsertest.cc:54: // The test page will perform tests on IndexedDB, then navigate to either IndexedDB -> Blob storage https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:827: bool has_pending_memory = i < kTotalRawBlobs && (i % 2 != 0 || i % 3 == 0); && DoesBuilderHaveFutureData(i) https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:848: if (has_pending_memory && !populated[i] && nit: can just write like 'if (DoesBuilderHaveFutureData(i) && !populated[i])' https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:861: if (has_pending_memory) { nit: can just write like 'if (DoesBuilderHaveFutureData(i))' https://codereview.chromium.org/2516713002/diff/220001/content/test/data/blob... File content/test/data/blob_storage/blob_creation_and_slicing.html (right): https://codereview.chromium.org/2516713002/diff/220001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:14: var numSandwiches = 100; should we / could we use CONSTANT_NAME style for constant vars (not super sure about test js style in chromium)? It's helpful for readers to distinguish constants and variables instantly. https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:337: // can copy the handle over. nit: There seems some semantic gap between the comment above and code below, could we revise it to make it clearer how the description matches with the code (e.g. using the real method/variable/constant names to describe what we're doing) https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:528: // The blob can complete during the execution of this line. nit: 'this line' -> ReserveMemoryQuota https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:544: // The blob can complete during the execution of this line. ditto
lgtm https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... File content/browser/blob_storage/blob_flattener_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... content/browser/blob_storage/blob_flattener_unittest.cc:42: for (FileCreationInfo& info : files) { nit: this looks a lot like the three-argument std::move: std::move(files.begin(), files.end(), std::back_inserter(*files_ptr));
i think this lgtm too but please see couple comments https://codereview.chromium.org/2516713002/diff/180001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/180001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:129: ASSERT_TRUE(temp_dir_.Delete()); Possibly the test is failing because a file handle is still open. Have you tried clearing files_ and context_ prior to deleting the tempdir? https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_entry.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_entry.cc:39: BlobEntry::BuildingState::~BuildingState() {} Are the quota_requests expected to be empty/done at dtor time, can that be asserted? Or if one or the other is still pending, should they be cancelled in here? https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_entry.h (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_entry.h:106: !building_state_->copy_quota_request && what about the transport_quota_request, should that be in this test too? https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:49: LOG(ERROR) << "Error creating blob storage directory: " << error; you can use LOG_IF here https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:169: DCHECK_EQ(sizes_from_files, total_size_output) can we use std::accumulate here to avoid the unused local in release builds? https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_reader.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_reader.cc:249: LOG(ERROR) << "Async error: " << net_error is this needed in release builds or would DLOG or VLOG be more appropiate? https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:688: LOG(ERROR) << "Blob is broken, reason: " << static_cast<int>(status); In general, please avoid using LOG except for very consequential events, prefer DLOG or VLOG. This particular event is histogram logged and appears in chrome://histograms in dbg and rel builds. I think I'd vote to not add it to the console output too. https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:124: context_.reset(new BlobStorageContext(std::move(blob_storage_dir), is FilePath really moved here? https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.h (right): https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.h:59: BlobStorageContext(base::FilePath storage_directory, would this be better as a const ref?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks everyone! Ben helped me fix the browsertest issue on Windows, I wasn't flushing to disk before grabbing the file info for the 'last modified' timestamp (which then made our reading fail when it was a mismatch). The test failure should be fixed as well. https://codereview.chromium.org/2516713002/diff/180001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/180001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:129: ASSERT_TRUE(temp_dir_.Delete()); On 2016/12/02 20:41:07, michaeln wrote: > Possibly the test is failing because a file handle is still open. Have you tried > clearing files_ and context_ prior to deleting the tempdir? Yeah, that's it. Thanks! The windows browsertest failures earlier was due to me failing to flush the files before grabbing the 'last modified' time. Windows works for the browsertest now! yay! https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_entry.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_entry.cc:39: BlobEntry::BuildingState::~BuildingState() {} On 2016/12/02 20:41:07, michaeln wrote: > Are the quota_requests expected to be empty/done at dtor time, can that be > asserted? Or if one or the other is still pending, should they be cancelled in > here? We can assert that. https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_entry.h (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_entry.h:106: !building_state_->copy_quota_request && On 2016/12/02 20:41:07, michaeln wrote: > what about the transport_quota_request, should that be in this test too? The transport state are handled by the status_ code. If we need quota for transport, then the status_ will be PENDING_QUOTA. I'll write a comment to clarify. https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:49: LOG(ERROR) << "Error creating blob storage directory: " << error; On 2016/12/02 20:41:07, michaeln wrote: > you can use LOG_IF here Done. https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:169: DCHECK_EQ(sizes_from_files, total_size_output) On 2016/12/02 20:41:07, michaeln wrote: > can we use std::accumulate here to avoid the unused local in release builds? Done. https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_reader.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_reader.cc:249: LOG(ERROR) << "Async error: " << net_error On 2016/12/02 20:41:07, michaeln wrote: > is this needed in release builds or would DLOG or VLOG be more appropiate? Removed, this was for figuring out the browsertests error. https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/180001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:688: LOG(ERROR) << "Blob is broken, reason: " << static_cast<int>(status); On 2016/12/02 20:41:08, michaeln wrote: > In general, please avoid using LOG except for very consequential events, prefer > DLOG or VLOG. > > This particular event is histogram logged and appears in chrome://histograms in > dbg and rel builds. I think I'd vote to not add it to the console output too. Done, this was to figure out the browsertest error on windows. https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_browsertest.cc (right): https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_browsertest.cc:54: // The test page will perform tests on IndexedDB, then navigate to either On 2016/12/02 08:52:32, kinuko wrote: > IndexedDB -> Blob storage Done. https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:827: bool has_pending_memory = i < kTotalRawBlobs && (i % 2 != 0 || i % 3 == 0); On 2016/12/02 08:52:32, kinuko wrote: > && DoesBuilderHaveFutureData(i) Done. https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:848: if (has_pending_memory && !populated[i] && On 2016/12/02 08:52:32, kinuko wrote: > nit: can just write like 'if (DoesBuilderHaveFutureData(i) && !populated[i])' Done. https://codereview.chromium.org/2516713002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_storage_context_unittest.cc:861: if (has_pending_memory) { On 2016/12/02 08:52:32, kinuko wrote: > nit: can just write like 'if (DoesBuilderHaveFutureData(i))' Done. https://codereview.chromium.org/2516713002/diff/220001/content/test/data/blob... File content/test/data/blob_storage/blob_creation_and_slicing.html (right): https://codereview.chromium.org/2516713002/diff/220001/content/test/data/blob... content/test/data/blob_storage/blob_creation_and_slicing.html:14: var numSandwiches = 100; On 2016/12/02 08:52:32, kinuko wrote: > should we / could we use CONSTANT_NAME style for constant vars (not super sure > about test js style in chromium)? It's helpful for readers to distinguish > constants and variables instantly. Done. https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... File content/browser/blob_storage/blob_flattener_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... content/browser/blob_storage/blob_flattener_unittest.cc:42: for (FileCreationInfo& info : files) { On 2016/12/02 19:27:48, Marijn Kruisselbrink wrote: > nit: this looks a lot like the three-argument std::move: > std::move(files.begin(), files.end(), std::back_inserter(*files_ptr)); Done. https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_s... content/browser/blob_storage/chrome_blob_storage_context.cc:124: context_.reset(new BlobStorageContext(std::move(blob_storage_dir), On 2016/12/02 20:41:08, michaeln wrote: > is FilePath really moved here? It will be! https://bugs.chromium.org/p/chromium/issues/detail?id=670043 https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:337: // can copy the handle over. On 2016/12/02 08:52:33, kinuko wrote: > nit: There seems some semantic gap between the comment above and code below, > could we revise it to make it clearer how the description matches with the code > (e.g. using the real method/variable/constant names to describe what we're > doing) Done. https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:528: // The blob can complete during the execution of this line. On 2016/12/02 08:52:32, kinuko wrote: > nit: 'this line' -> ReserveMemoryQuota Done. https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:544: // The blob can complete during the execution of this line. On 2016/12/02 08:52:33, kinuko wrote: > ditto Done. https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.h (right): https://codereview.chromium.org/2516713002/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.h:59: BlobStorageContext(base::FilePath storage_directory, On 2016/12/02 20:41:08, michaeln wrote: > would this be better as a const ref? I'm going to implement the move constructor, see: https://bugs.chromium.org/p/chromium/issues/detail?id=670043
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. This patch enables saving blobs to disk by paging and sending files directly to the renderer. R=michaeln,kinuko BUG=375297 ========== to ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0), the next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0), the next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 ========== to ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 ==========
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dmurph@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, mek@chromium.org Link to the patchset: https://codereview.chromium.org/2516713002/#ps320001 (title: "fixed syntax error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dmurph@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, mek@chromium.org Link to the patchset: https://codereview.chromium.org/2516713002/#ps340001 (title: "fixed tests that assumed disk quota")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1480963488958670,
"parent_rev": "b04cb25eeae02711c46bf847f9f2037de948d4a7", "commit_rev":
"261f198f086ed86108ca16a3b8d538bfefa083df"}
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 ========== to ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 ========== to ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/2550113003/ by carlosk@chromium.org. The reason for reverting is: Seems to be breaking BlobStorageBrowserTest.BlobCombinations on "Mac10.9 Tests (dbg)"..
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415} ========== to ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415} ==========
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Apparently mac dbg doesn't run out event loops the same was as everything else? Since we have unit tests around file and memory cleanup, I'm comfortable removing this from the browsertest. New patch uploaded, and I'll re-commit soon.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_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, mek@chromium.org Link to the patchset: https://codereview.chromium.org/2516713002/#ps360001 (title: "removed cleanup check, as mac doesn't run out event loops")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1480981169351190,
"parent_rev": "1bf0af20333e407419a725276208f021d3843685", "commit_rev":
"236445a2e131b6cae0e5454242eff56605052396"}
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415} ========== to ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415} ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415} ========== to ========== [BlobStorage] Enabling disk storage and paging. Adds support for saving blobs to disk by either paging them to disk or sending files directly to the renderer. Note: Disk is turned off by default (limit=0). So other than tests that explicitly set a higher disk limit, this patch should be a NOOP. The next patch will configure this limit (and the current RAM limit) by the OS, disk size, and available RAM. R=michaeln,kinuko BUG=375297 Committed: https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Committed: https://crrev.com/07725e0f2bd75b9649ff9b4a10096a281e56d998 Cr-Original-Commit-Position: refs/heads/master@{#436415} Cr-Commit-Position: refs/heads/master@{#436485} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/07725e0f2bd75b9649ff9b4a10096a281e56d998 Cr-Commit-Position: refs/heads/master@{#436485} |
