Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

Issue 2516713002: [BlobStorage] Implementing disk. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by dmurph
Modified:
3 months, 3 weeks ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+953 lines, -213 lines) Patch
M content/browser/blob_storage/blob_flattener_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 10 chunks +91 lines, -13 lines 0 comments Download
M content/browser/blob_storage/blob_memory_controller_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/browser/blob_storage/blob_slice_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 10 chunks +36 lines, -10 lines 0 comments Download
A content/browser/blob_storage/blob_storage_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +100 lines, -0 lines 0 comments Download
M content/browser/blob_storage/blob_storage_context_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +239 lines, -3 lines 0 comments Download
M content/browser/blob_storage/chrome_blob_storage_context.h View 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/blob_storage/chrome_blob_storage_context.cc View 1 2 3 4 5 6 7 5 chunks +66 lines, -7 lines 0 comments Download
M content/child/blob_storage/blob_transport_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +23 lines, -18 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
A content/test/data/blob_storage/blob_creation_and_slicing.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +134 lines, -0 lines 0 comments Download
A + content/test/data/blob_storage/common.js View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +22 lines, -71 lines 0 comments Download
M storage/browser/blob/blob_entry.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +17 lines, -10 lines 0 comments Download
M storage/browser/blob/blob_entry.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +13 lines, -1 line 0 comments Download
M storage/browser/blob/blob_memory_controller.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M storage/browser/blob/blob_memory_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 10 chunks +16 lines, -13 lines 0 comments Download
M storage/browser/blob/blob_reader.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M storage/browser/blob/blob_storage_context.h View 1 2 3 4 5 6 7 7 chunks +28 lines, -8 lines 0 comments Download
M storage/browser/blob/blob_storage_context.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 15 chunks +153 lines, -50 lines 0 comments Download
M storage/common/blob_storage/blob_storage_constants.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +7 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 114 (88 generated)
dmurph
Hello! I'm still in the process of adding a browsertest here (having trouble getting it ...
4 months, 1 week ago (2016-11-22 02:05:59 UTC) #14
dmurph
Browsertest uploaded!
4 months, 1 week ago (2016-11-22 21:46:24 UTC) #18
kinuko
Haven't looked into test code but the code change itself looks reasonable to me. Will ...
4 months ago (2016-11-27 14:45:22 UTC) #26
dmurph
https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_storage/blob_storage_context_unittest.cc File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/80001/content/browser/blob_storage/blob_storage_context_unittest.cc#newcode675 content/browser/blob_storage/blob_storage_context_unittest.cc:675: TEST_F(BlobStorageContextTest, BuildBlobFuzzer) { On 2016/11/27 14:45:22, kinuko wrote: > ...
4 months ago (2016-11-28 19:20:36 UTC) #28
dmurph
+mek and +victor for a review, and to for knowledge diffusion (and michael is OOO ...
4 months ago (2016-11-28 19:24:14 UTC) #31
Marijn Kruisselbrink
looks good https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_storage/blob_storage_context_unittest.cc File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/100001/content/browser/blob_storage/blob_storage_context_unittest.cc#newcode129 content/browser/blob_storage/blob_storage_context_unittest.cc:129: ASSERT_TRUE(temp_dir_.Delete()); It seems a bit weird to ...
4 months ago (2016-11-29 22:25:21 UTC) #34
dmurph
Thanks for the comments! @kinuko, @michaeln, and @pwnall - This code is in a final ...
3 months, 4 weeks ago (2016-11-30 22:12:09 UTC) #37
pwnall
https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob_storage/blob_creation_and_slicing.html File content/test/data/blob_storage/blob_creation_and_slicing.html (right): https://codereview.chromium.org/2516713002/diff/100001/content/test/data/blob_storage/blob_creation_and_slicing.html#newcode6 content/test/data/blob_storage/blob_creation_and_slicing.html:6: // We create < 2000 bytes of data, as ...
3 months, 4 weeks ago (2016-12-01 01:12:14 UTC) #40
kinuko
Some more comments. https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_storage/blob_flattener_unittest.cc File content/browser/blob_storage/blob_flattener_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/120001/content/browser/blob_storage/blob_flattener_unittest.cc#newcode243 content/browser/blob_storage/blob_flattener_unittest.cc:243: // We're copying item at index ...
3 months, 4 weeks ago (2016-12-01 05:10:05 UTC) #41
dmurph
Thanks for the comments! I uploaded a patch that had some logging for the windows ...
3 months, 4 weeks ago (2016-12-01 20:41:00 UTC) #44
pwnall
Here are a few more small comments on the browser test. https://codereview.chromium.org/2516713002/diff/180001/content/test/data/blob_storage/blob_creation_and_slicing.html File content/test/data/blob_storage/blob_creation_and_slicing.html (right): ...
3 months, 4 weeks ago (2016-12-01 23:28:49 UTC) #49
dmurph
publishing comment responses. Continuing to debug windows. Narrowed it down to the time modified being ...
3 months, 4 weeks ago (2016-12-02 01:11:34 UTC) #58
kinuko
other than various error logging and some more readability nits I think the code itself ...
3 months, 3 weeks ago (2016-12-02 08:52:33 UTC) #65
Marijn Kruisselbrink
lgtm https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_storage/blob_flattener_unittest.cc File content/browser/blob_storage/blob_flattener_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/240001/content/browser/blob_storage/blob_flattener_unittest.cc#newcode42 content/browser/blob_storage/blob_flattener_unittest.cc:42: for (FileCreationInfo& info : files) { nit: this ...
3 months, 3 weeks ago (2016-12-02 19:27:48 UTC) #66
michaeln
i think this lgtm too but please see couple comments https://codereview.chromium.org/2516713002/diff/180001/content/browser/blob_storage/blob_storage_context_unittest.cc File content/browser/blob_storage/blob_storage_context_unittest.cc (right): https://codereview.chromium.org/2516713002/diff/180001/content/browser/blob_storage/blob_storage_context_unittest.cc#newcode129 ...
3 months, 3 weeks ago (2016-12-02 20:41:08 UTC) #67
dmurph
Thanks everyone! Ben helped me fix the browsertest issue on Windows, I wasn't flushing to ...
3 months, 3 weeks ago (2016-12-02 21:53:07 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2516713002/320001
3 months, 3 weeks ago (2016-12-02 23:36:41 UTC) #86
commit-bot: I haz the power
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_linux/builds/271649)
3 months, 3 weeks ago (2016-12-03 00:09:31 UTC) #88
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2516713002/340001
3 months, 3 weeks ago (2016-12-05 18:45:29 UTC) #94
commit-bot: I haz the power
Committed patchset #18 (id:340001)
3 months, 3 weeks ago (2016-12-05 21:34:19 UTC) #97
commit-bot: I haz the power
Patchset 18 (id:??) landed as https://crrev.com/89bdfe0bd9499962f9f05c30a72d46fc6510d45a Cr-Commit-Position: refs/heads/master@{#436415}
3 months, 3 weeks ago (2016-12-05 21:37:54 UTC) #99
carlosk
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/2550113003/ by carlosk@chromium.org. ...
3 months, 3 weeks ago (2016-12-05 23:16:27 UTC) #100
dmurph
Apparently mac dbg doesn't run out event loops the same was as everything else? Since ...
3 months, 3 weeks ago (2016-12-05 23:20:08 UTC) #104
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2516713002/360001
3 months, 3 weeks ago (2016-12-05 23:39:58 UTC) #109
commit-bot: I haz the power
Committed patchset #19 (id:360001)
3 months, 3 weeks ago (2016-12-06 02:29:10 UTC) #112
commit-bot: I haz the power
3 months, 3 weeks ago (2016-12-06 02:31:20 UTC) #114
Message was sent while issue was closed.
Patchset 19 (id:??) landed as
https://crrev.com/07725e0f2bd75b9649ff9b4a10096a281e56d998
Cr-Commit-Position: refs/heads/master@{#436485}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46