Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(159)

Issue 2448353002: [BlobAsync] Moving async handling into BlobStorageContext & quota out. (Closed)

Created:
4 years, 1 month ago by dmurph
Modified:
4 years, 1 month ago
CC:
blink-worker-reviews_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, eroman, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+fileapi, kinuko+watch, nhiroki, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[BlobAsync] Moving async handling into BlobStorageContext & quota out. This change moves async building logic, including waiting on referenced blobs, out of the BlobAsyncBuilderHost into the BlobStorageContext. It also moves the quota handling out of BlobStorageContext and into BlobMemoryController. This change does not enable paging to files (or backing blobs by files right away) yet. It's just doing the required system and state refactoring. BUG=375297, 612261, 600528 R=michaeln@chromium.org,kinuko@chromium.org,pwnall@chromium.org,mek@chromium.org Committed: https://crrev.com/cf848a6957fb402bdffeb5a2ce51e9f0cbbb98f2 Cr-Commit-Position: refs/heads/master@{#432960}

Patch Set 1 #

Patch Set 2 : compile fix #

Total comments: 38

Patch Set 3 : comments & windows/chromeos compile #

Total comments: 14

Patch Set 4 : hopefully fixed android/windows compile, and comments #

Total comments: 4

Patch Set 5 : Fixed unused function #

Total comments: 23

Patch Set 6 : comments & changing 'population' to 'transport' #

Patch Set 7 : rebase #

Total comments: 2

Patch Set 8 : comments #

Total comments: 35

Patch Set 9 : comments #

Patch Set 10 : comments from Marijn #

Total comments: 49

Patch Set 11 : comments #

Total comments: 1

Patch Set 12 : restructure of dispatcher host and transport host code #

Patch Set 13 : Cleaned up more #

Total comments: 14

Patch Set 14 : comments #

Total comments: 16

Patch Set 15 : comments #

Patch Set 16 : forgot UMA metrics! #

Patch Set 17 : removed unused code #

Total comments: 8

Patch Set 18 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3636 lines, -3879 lines) Patch
M chrome/browser/chrome_security_exploit_browsertest.cc View 1 2 3 4 5 1 chunk +3 lines, -8 lines 0 comments Download
M content/browser/blob_storage/blob_async_builder_host_unittest.cc View 1 2 1 chunk +0 lines, -617 lines 0 comments Download
D content/browser/blob_storage/blob_async_transport_request_builder_unittest.cc View 1 2 3 4 5 1 chunk +0 lines, -350 lines 0 comments Download
M content/browser/blob_storage/blob_dispatcher_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +28 lines, -30 lines 0 comments Download
M content/browser/blob_storage/blob_dispatcher_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 12 chunks +103 lines, -166 lines 0 comments Download
M content/browser/blob_storage/blob_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 36 chunks +238 lines, -356 lines 0 comments Download
A content/browser/blob_storage/blob_flattener_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +204 lines, -0 lines 0 comments Download
M content/browser/blob_storage/blob_memory_controller_unittest.cc View 1 chunk +2 lines, -3 lines 0 comments Download
M content/browser/blob_storage/blob_reader_unittest.cc View 1 2 3 4 5 7 chunks +33 lines, -9 lines 0 comments Download
A content/browser/blob_storage/blob_slice_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +195 lines, -0 lines 0 comments Download
M content/browser/blob_storage/blob_storage_context_unittest.cc View 1 2 3 4 5 19 chunks +258 lines, -97 lines 0 comments Download
M content/browser/blob_storage/blob_storage_registry_unittest.cc View 1 2 3 chunks +7 lines, -10 lines 0 comments Download
A content/browser/blob_storage/blob_transport_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +512 lines, -0 lines 0 comments Download
A + content/browser/blob_storage/blob_transport_request_builder_unittest.cc View 1 2 3 4 5 9 chunks +9 lines, -9 lines 0 comments Download
M content/browser/service_worker/service_worker_url_request_job.h View 1 2 3 4 5 6 7 8 4 chunks +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_url_request_job.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 7 chunks +88 lines, -116 lines 0 comments Download
M content/child/blob_storage/blob_consolidation.cc View 1 2 1 chunk +16 lines, -35 lines 0 comments Download
M content/child/blob_storage/blob_message_filter.h View 1 2 3 4 5 1 chunk +1 line, -4 lines 0 comments Download
M content/child/blob_storage/blob_message_filter.cc View 2 chunks +4 lines, -10 lines 0 comments Download
M content/child/blob_storage/blob_transport_controller.h View 1 2 3 4 5 3 chunks +4 lines, -6 lines 0 comments Download
M content/child/blob_storage/blob_transport_controller.cc View 9 chunks +20 lines, -31 lines 0 comments Download
M content/child/blob_storage/blob_transport_controller_unittest.cc View 6 chunks +12 lines, -18 lines 0 comments Download
M content/child/blob_storage/webblobregistry_impl.h View 1 2 3 4 5 2 chunks +2 lines, -0 lines 0 comments Download
M content/child/blob_storage/webblobregistry_impl.cc View 3 chunks +2 lines, -3 lines 0 comments Download
M content/common/fileapi/webblob_messages.h View 3 chunks +8 lines, -15 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -2 lines 0 comments Download
M net/log/net_log_event_type_list.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -4 lines 0 comments Download
M storage/browser/BUILD.gn View 1 2 3 4 5 3 chunks +6 lines, -6 lines 0 comments Download
D storage/browser/blob/blob_async_builder_host.h View 1 2 3 4 5 1 chunk +0 lines, -205 lines 0 comments Download
M storage/browser/blob/blob_async_builder_host.cc View 1 2 1 chunk +0 lines, -465 lines 0 comments Download
M storage/browser/blob/blob_async_transport_request_builder.h View 1 2 3 4 5 1 chunk +0 lines, -138 lines 0 comments Download
M storage/browser/blob/blob_async_transport_request_builder.cc View 1 2 3 4 5 1 chunk +0 lines, -299 lines 0 comments Download
M storage/browser/blob/blob_data_builder.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M storage/browser/blob/blob_data_builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -0 lines 0 comments Download
M storage/browser/blob/blob_data_handle.h View 1 2 3 4 5 7 chunks +13 lines, -5 lines 0 comments Download
M storage/browser/blob/blob_data_handle.cc View 1 2 3 4 5 5 chunks +16 lines, -7 lines 0 comments Download
M storage/browser/blob/blob_data_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +4 lines, -0 lines 0 comments Download
A storage/browser/blob/blob_entry.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +164 lines, -0 lines 0 comments Download
A storage/browser/blob/blob_entry.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +69 lines, -0 lines 0 comments Download
M storage/browser/blob/blob_memory_controller.h View 2 chunks +2 lines, -2 lines 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 15 3 chunks +20 lines, -3 lines 0 comments Download
M storage/browser/blob/blob_reader.h View 1 chunk +1 line, -2 lines 0 comments Download
M storage/browser/blob/blob_reader.cc View 2 chunks +15 lines, -11 lines 0 comments Download
M storage/browser/blob/blob_storage_context.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +168 lines, -85 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 14 15 16 17 3 chunks +608 lines, -342 lines 0 comments Download
M storage/browser/blob/blob_storage_registry.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +11 lines, -52 lines 0 comments Download
M storage/browser/blob/blob_storage_registry.cc View 1 2 3 4 5 5 chunks +11 lines, -29 lines 0 comments Download
A storage/browser/blob/blob_transport_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +186 lines, -0 lines 0 comments Download
A storage/browser/blob/blob_transport_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +506 lines, -0 lines 0 comments Download
A + storage/browser/blob/blob_transport_request_builder.h View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +9 lines, -10 lines 0 comments Download
A + storage/browser/blob/blob_transport_request_builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 12 chunks +19 lines, -37 lines 0 comments Download
D storage/browser/blob/blob_transport_result.h View 1 chunk +0 lines, -27 lines 0 comments Download
M storage/browser/blob/internal_blob_data.h View 1 2 1 chunk +0 lines, -83 lines 0 comments Download
M storage/browser/blob/internal_blob_data.cc View 1 2 1 chunk +0 lines, -97 lines 0 comments Download
M storage/browser/blob/shareable_blob_data_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +7 lines, -14 lines 0 comments Download
M storage/browser/blob/shareable_blob_data_item.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +1 line, -8 lines 0 comments Download
M storage/browser/blob/view_blob_internals_job.h View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M storage/browser/blob/view_blob_internals_job.cc View 1 2 5 chunks +13 lines, -11 lines 0 comments Download
M storage/common/blob_storage/blob_storage_constants.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +8 lines, -35 lines 0 comments Download
M storage/common/blob_storage/blob_storage_constants.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M storage/common/data_element.h View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 112 (73 generated)
dmurph
Hey everyone, I tried to split the paging patch up again. This patch encompasses the ...
4 years, 1 month ago (2016-10-26 19:15:52 UTC) #1
michaeln
i'd like to look closer still (and take a look at my comments from the ...
4 years, 1 month ago (2016-10-28 19:38:29 UTC) #10
michaeln
in general, i think this makes a good intermediary stepping stone in CL engineering
4 years, 1 month ago (2016-10-28 19:40:12 UTC) #11
dmurph
Thanks! https://codereview.chromium.org/2448353002/diff/20001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2448353002/diff/20001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode330 content/browser/blob_storage/blob_dispatcher_host.cc:330: blobs_inuse_map_.erase(uuid); On 2016/10/28 19:38:29, michaeln wrote: > is ...
4 years, 1 month ago (2016-10-28 22:13:56 UTC) #13
pwnall
Can you please get the CL in a state where it compiles? https://codereview.chromium.org/2448353002/diff/40001/chrome/browser/chrome_security_exploit_browsertest.cc File chrome/browser/chrome_security_exploit_browsertest.cc ...
4 years, 1 month ago (2016-11-04 02:26:54 UTC) #17
dmurph
Thanks for the comments! This should be compiling everywhere now. https://codereview.chromium.org/2448353002/diff/40001/chrome/browser/chrome_security_exploit_browsertest.cc File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2448353002/diff/40001/chrome/browser/chrome_security_exploit_browsertest.cc#newcode94 ...
4 years, 1 month ago (2016-11-04 23:23:01 UTC) #24
michaeln
i have a couple code questions/comments, but mostly naming consistency sort of comments https://codereview.chromium.org/2448353002/diff/20001/content/browser/blob_storage/blob_dispatcher_host.cc File ...
4 years, 1 month ago (2016-11-07 21:47:06 UTC) #27
dmurph
Thanks for the comments! Fixes, language changes, and rebase. PTAL! Thanks, Daniel https://codereview.chromium.org/2448353002/diff/20001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc ...
4 years, 1 month ago (2016-11-08 21:19:59 UTC) #29
michaeln
thnx, lgtm! https://codereview.chromium.org/2448353002/diff/120001/storage/browser/blob/blob_transport_host.cc File storage/browser/blob/blob_transport_host.cc (right): https://codereview.chromium.org/2448353002/diff/120001/storage/browser/blob/blob_transport_host.cc#newcode161 storage/browser/blob/blob_transport_host.cc:161: return; return statement not needed
4 years, 1 month ago (2016-11-08 21:56:15 UTC) #30
dmurph
Thanks! +Matt for the ServiceURLRequestJob changes, these are the same as the last patch here: ...
4 years, 1 month ago (2016-11-08 22:53:27 UTC) #33
falken
Looking good, mostly naming nits. https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc File content/browser/service_worker/service_worker_url_request_job.cc (left): https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc#oldcode530 content/browser/service_worker/service_worker_url_request_job.cc:530: } So happy about ...
4 years, 1 month ago (2016-11-09 02:06:20 UTC) #37
kinuko
Sorry for slow review, I'm still reading the patch but looking good in general. Sending ...
4 years, 1 month ago (2016-11-09 16:45:07 UTC) #38
Marijn Kruisselbrink
Just some minor comments https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc#newcode129 content/browser/service_worker/service_worker_url_request_job.cc:129: TRACE_EVENT_ASYNC_BEGIN1("ServiceWorker", "BlobFilesWaiter", this, "URL", Is ...
4 years, 1 month ago (2016-11-09 18:39:40 UTC) #39
dmurph
Thanks everyone https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc File content/browser/service_worker/service_worker_url_request_job.cc (left): https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc#oldcode530 content/browser/service_worker/service_worker_url_request_job.cc:530: } On 2016/11/09 02:06:19, falken wrote: > ...
4 years, 1 month ago (2016-11-09 19:13:27 UTC) #44
mmenke
Anything in particular you want me to look at? Skimming over ServiceWorkerURLRequestJob, it doesn't look ...
4 years, 1 month ago (2016-11-09 19:47:59 UTC) #45
dmurph
No, I don't think I have any changes in this CL that correspond to you, ...
4 years, 1 month ago (2016-11-09 20:00:54 UTC) #46
falken
service worker lgtm, thanks https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2448353002/diff/140001/content/browser/service_worker/service_worker_url_request_job.cc#newcode196 content/browser/service_worker/service_worker_url_request_job.cc:196: callback_.Run(success); On 2016/11/09 19:13:27, dmurph ...
4 years, 1 month ago (2016-11-10 01:25:05 UTC) #49
kinuko
Some more comments. https://codereview.chromium.org/2448353002/diff/180001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2448353002/diff/180001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode111 content/browser/blob_storage/blob_dispatcher_host.cc:111: context->IncrementBlobRefCount(uuid); Could you help me understand, ...
4 years, 1 month ago (2016-11-10 05:16:37 UTC) #50
dmurph
Thanks! https://codereview.chromium.org/2448353002/diff/180001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2448353002/diff/180001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode111 content/browser/blob_storage/blob_dispatcher_host.cc:111: context->IncrementBlobRefCount(uuid); On 2016/11/10 05:16:36, kinuko wrote: > Could ...
4 years, 1 month ago (2016-11-10 19:53:20 UTC) #52
kinuko
The ref-counting things still keep me bothering a bit, thanks for being patient. https://codereview.chromium.org/2448353002/diff/180001/content/browser/blob_storage/blob_dispatcher_host.cc File ...
4 years, 1 month ago (2016-11-11 17:51:15 UTC) #56
dmurph
On 2016/11/11 17:51:15, kinuko wrote: > The ref-counting things still keep me bothering a bit, ...
4 years, 1 month ago (2016-11-11 22:04:28 UTC) #57
dmurph
Hello! Can you take another look at the BlobDispatcherHost and BlobTransportHost code again? I've changed ...
4 years, 1 month ago (2016-11-15 01:21:03 UTC) #60
michaeln
still lgtm, but why change class of map? https://codereview.chromium.org/2448353002/diff/240001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2448353002/diff/240001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode190 content/browser/blob_storage/blob_dispatcher_host.cc:190: uuid, ...
4 years, 1 month ago (2016-11-16 00:55:36 UTC) #67
dmurph
Thanks! https://codereview.chromium.org/2448353002/diff/240001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2448353002/diff/240001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode190 content/browser/blob_storage/blob_dispatcher_host.cc:190: uuid, BlobStatus::ERR_BLOB_DEREFERENCED_WHILE_BUILDING, context); On 2016/11/16 00:55:35, michaeln wrote: ...
4 years, 1 month ago (2016-11-16 02:15:29 UTC) #69
kinuko
Thanks for the clean up, I like the new code better! LGTM % nits https://codereview.chromium.org/2448353002/diff/260001/content/browser/blob_storage/blob_flattener_unittest.cc ...
4 years, 1 month ago (2016-11-16 03:53:52 UTC) #73
dmurph
https://codereview.chromium.org/2448353002/diff/260001/content/browser/blob_storage/blob_flattener_unittest.cc File content/browser/blob_storage/blob_flattener_unittest.cc (right): https://codereview.chromium.org/2448353002/diff/260001/content/browser/blob_storage/blob_flattener_unittest.cc#newcode67 content/browser/blob_storage/blob_flattener_unittest.cc:67: ASSERT_TRUE(base::CreateNewTempDirectory("BlobFlattenerTest", &temp_dir_)); On 2016/11/16 03:53:51, kinuko wrote: > nit: ...
4 years, 1 month ago (2016-11-16 20:05:01 UTC) #76
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/2448353002/280001
4 years, 1 month ago (2016-11-16 20:12:03 UTC) #80
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/305992)
4 years, 1 month ago (2016-11-16 20:25:06 UTC) #82
dmurph
Michael & Kinuko, I forgot to add the UMA metrics back in (and there was ...
4 years, 1 month ago (2016-11-16 23:37:15 UTC) #85
kinuko
I'm ok with landing this one, but some histogram names might start to look a ...
4 years, 1 month ago (2016-11-17 07:53:02 UTC) #92
dmurph
Yeah I wanted the stats to be continuous. I think I can also update the ...
4 years, 1 month ago (2016-11-17 18:05:23 UTC) #95
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/2448353002/340001
4 years, 1 month ago (2016-11-17 18:30:26 UTC) #101
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/307165)
4 years, 1 month ago (2016-11-17 18:43:17 UTC) #103
dmurph
+palmer for content/common/fileapi/webblob_messages.h +asanka for net/log/net_log_event_type_list.h
4 years, 1 month ago (2016-11-17 18:48:37 UTC) #105
palmer
IPC LGTM.
4 years, 1 month ago (2016-11-17 19:20:44 UTC) #106
asanka
net/log lgtm
4 years, 1 month ago (2016-11-17 19:28:07 UTC) #107
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/2448353002/340001
4 years, 1 month ago (2016-11-17 19:41:23 UTC) #109
commit-bot: I haz the power
Committed patchset #18 (id:340001)
4 years, 1 month ago (2016-11-17 21:25:38 UTC) #110
commit-bot: I haz the power
4 years, 1 month ago (2016-11-17 21:29:24 UTC) #112
Message was sent while issue was closed.
Patchset 18 (id:??) landed as
https://crrev.com/cf848a6957fb402bdffeb5a2ce51e9f0cbbb98f2
Cr-Commit-Position: refs/heads/master@{#432960}

Powered by Google App Engine
This is Rietveld 408576698