Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2637023003:
[BlobStorage] Adding explainer for blob storage system. (Closed)
Description[BlobStorage] Adding explainer for blob storage system.
BUG=682059
Review-Url: https://codereview.chromium.org/2637023003
Cr-Commit-Position: refs/heads/master@{#447012}
Committed: https://chromium.googlesource.com/chromium/src/+/224e43ce1ca4f6ca6f2cd8d677b8684f4b7c2351
Patch Set 1 #
Total comments: 11
Patch Set 2 : Comments #Patch Set 3 : added more information #
Total comments: 79
Patch Set 4 : Comments, and added Limits and Pitfalls sections #
Total comments: 2
Patch Set 5 : comments #Messages
Total messages: 15 (6 generated)
dmurph@chromium.org changed reviewers: + jsbell@chromium.org
This isn't complete, but wondering if this is the best way to do this? Should I wrap to 80 characters here? WDYT? Markdown lets you use a single newline and treats it as a space character AFAIK.
Initial notes. Also have pwnall@ review? I would wrap to 80 chars so it's easier to read in a terminal. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README.md File storage/browser/blob/README.md (right): https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:2: Elaboration of the blob storage system in Chrome. Leave a blank after headers (# ## etc) Some markdown processors don't end the header on the newline :( https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:5: Please see [Mozilla's Blob documentation](https://developer.mozilla.org/en-US/docs/Web/API/Blob) for a description of how Blobs are used in the Web Platform in general. For the purposes of this document, the important aspects of blobs are: I'd wrap to 80 Also, I'd like to the File API spec as well. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:13: In Chrome, after blob creation the actual blob 'data' (gets transported to and) lives in the browser process. The renderer just holds a reference - specifically a string UUID - to the blob, which it can use to read the blob or pass it to other processes. Why () here? https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:17: Blobs are created in the renderer process, where their data is temporarily held for the browser (while javascript execution can continue). When the browser has enough memory quota for the blob, it requests the data from the renderer. Once all data is transported and construction is complete, any pending reads for the blob are allowed to complete. Consistently capitalize (or not) javascript throughout. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:17: Blobs are created in the renderer process, where their data is temporarily held for the browser (while javascript execution can continue). When the browser has enough memory quota for the blob, it requests the data from the renderer. Once all data is transported and construction is complete, any pending reads for the blob are allowed to complete. Maybe note that blobs can be huge, e.g. GBs; otherwise a reader may be confused as to why there's a quota. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:21: Blob reading goes through the network layer, where the renderer dispatches a network request for the blob. Maybe mention the type here, e.g. URLRequest (or whatever it is) https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:23: Generally Chrome terminology: Generally -> General ?
dmurph@chromium.org changed reviewers: + pwnall@chromium.org
+pwnall for thoughts. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README.md File storage/browser/blob/README.md (right): https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:5: Please see [Mozilla's Blob documentation](https://developer.mozilla.org/en-US/docs/Web/API/Blob) for a description of how Blobs are used in the Web Platform in general. For the purposes of this document, the important aspects of blobs are: On 2017/01/18 01:05:50, jsbell wrote: > I'd wrap to 80 > > Also, I'd like to the File API spec as well. Done. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:13: In Chrome, after blob creation the actual blob 'data' (gets transported to and) lives in the browser process. The renderer just holds a reference - specifically a string UUID - to the blob, which it can use to read the blob or pass it to other processes. On 2017/01/18 01:05:50, jsbell wrote: > Why () here? Done. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:21: Blob reading goes through the network layer, where the renderer dispatches a network request for the blob. On 2017/01/18 01:05:50, jsbell wrote: > Maybe mention the type here, e.g. URLRequest (or whatever it is) Done. https://codereview.chromium.org/2637023003/diff/1/storage/browser/blob/README... storage/browser/blob/README.md:23: Generally Chrome terminology: On 2017/01/18 01:05:50, jsbell wrote: > Generally -> General ? Done.
This document looks very useful. Thanks for taking the time to put it together! https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... File storage/browser/blob/README.md (right): https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:15: 3. Blobs can be 'sliced', which creates a blob that is a subsection of another How about having the word "sliced" link to https://developer.mozilla.org/docs/Web/API/Blob/slice https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:17: 4. Reading blobs is asynchronous. Is it worth noting that obtaining blob metadata (especially size) is synchronous? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:28: Blobs are created in the renderer process, where their data is temporarily held in _a_ renderer process? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:31: all data is transported and construction is complete, any pending reads for the I'd emphasize the word "transported" in some way, because you're effectively defining it here, and the word is used elsewhere later. So, it's a concept too. I'd also say "all data is transported from the renderer to the browser", because the redundancy should help readers get adjusted to the new concepts. I'd qualify "construction" as "blob construction". You used the word "created" for the same concept earlier in the paragraph, so I wouldn't be 100% sure about what you're referring to. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:32: blob are allowed to complete. Blobs can be small (bytes) or huge (GBs), so "small (bytes)" does not seem to add value here https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:43: General Chrome terminology: I'd like to https://www.chromium.org/developers/design-documents/multi-process-architecture as the authoritative source for these descriptions. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:63: can also represent "future" file and "future" bytes, which is used to signify a "future" files? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:65: * **dependent blobs**: These are blobs that our blob depends on to be "blobs that a blob has data dependencies on"? The word "dependent" seems a bit confusing to me here, because my dependents are people who depend on me, whereas in this case a blob depends on the data in its "dependents". Would it be terribly hard to switch dependent blobs to referenced blobs? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:66: constructed. As in, we were constructed with a dependency on another blob I'm not a big fan of "we" and "our" usage here. I'd prefer to have clear subjects and objects. At the same time, your usage of pronouns definitely makes the document shorter, and resolving them is not too much mental effort. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:70: * **transportation strategy**: We can have one of 3 transportation strategies : a method for sending the data in a BlobItem from a renderer to the browser. You can add that the system currently implements three strategies: IPC, Shared Memory, Files. I'm slightly skeptical about the benefits of documenting specific methods, as the code is pretty obvious, and can change. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:75: ### Building ## instead of ###? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:77: built using a `BlobDataBuilder`, and as long as you don't use any any chance you could move the caveat after the main point of the phrase? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:83: `AppendFuture*` methods no the builder, but you must use no -> on? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:88: In general, this sections seems to assume that I've somehow read enough of the headers to know the method names, but not enough to know which ones to use. Most of it seems like it'd be a good fit for class-level documentation. I think I'd find this section more useful if it had an example for the happy path, and pointers to headers that contain alternative methods, and describe the caveats you mentioned. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:96: done or broken (due to not enough space, filesystem error, etc). done -> constructed? broken (construction failed due to ... https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:130: ## Blob Transportation, Renderer I think it'd be more consistent to end the heading in "(Renderer)" https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:142: 1. Send items and populate the data in IPC ([code]( I think line-level links like this one are quite brittle, because any code change will drive them out of date. Please consider adding a search query, like below, to make the link a bit more robust. https://cs.chromium.org/chromium/src/content/child/blob_storage/blob_transpor... Alternatively, you can write out the link in text, like this. -- Look for "case IPCBlobItemRequestStrategy::IPC" in `blob_transport_controller.cc`. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:152: data. It does this by using the [incrementing and decrementing the process ref remove "using the"? also, ref -> reference? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:156: # Blob Transportation & Storage (Browser). the period looks inappropriate here https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:161: 2. If so, how do we want to transport it? IPC? Shared Memory? IPC? Do you mean "File" instead of the last "IPC"? Alternatively, consider using the keywords you introduced earlier. For example -- Pick transport strategies for the blob's components. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:162: 3. Can I save this in memory right now? Or do I need to wait for older blob Does this mean "Is there enough free memory to transport the blob right now?" https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:172: 2. Create our browser-side representation of the blob data, including any data our -> the https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:173: items from dependent blobs. We try to share data items as much as possible, and Does the 2nd sentence here mean that data items are shared between blobs to save memory? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:176: manages our blob storage limits. Quota can be requested for both transportation can be requested -> is necessary? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:189: (without thinking about blob dependencies) with diagrams and details in [this thinking about -> accounting for https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:195: from the renderer to the browser. When the initial description of the blob is I like "description of the blob" / "initial description of the blob" as a term. Would it be worth explaining what this description consists of somewhere up above? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:197: 'strategy' (IPC, Shared Memory, or File) it should use to transport the file. I don't think you need quotes here, you introduced the term earlier. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:198: Based on this strategy it can transform the memory items sent from the renderer transform -> translate? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:204: Once the transport host decides it's strategy, it will create it's own it's -> its (twice) https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:210: transport the blob data, this class's responsability is to populate the class' ? Or, better yet, "the transport host populates ..., and then signals ..." https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:230: * When this is approved, it notifies the `BlobTransportHost` that it can it notifies -> notify (for consistency with the other steps in the list) Also, "when the quota request is granted"? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:232: 4. Ask the `BlobMemoryManager` for memory quota for any copies necessary from necessary for blob slicing? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:234: 5. Adds completion callbacks to any dependent blobs that our blob depends on. the word "dependent" here seems redundant https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:249: The BlobStatus outlines this procedure (specifically the transport process), As a reader, I am unsure what "this procedure" refers to. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:253: Once a blob is finished constructing, the status is set to `DONE`, or any of I think you can say "to `DONE`, or to one of the `ERR_*` values". The meaning of the prefix seems pretty obvious to me. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:258: During construction, 'slices' are created for dependent blobs using the given I don't think slices needs quotes here. It's a concept you introduced earlier. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:270: representation of the new blob, where all blob references are replaced with the remove "the"? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:271: 'BlobSlice' items. It also stores any copy data from the slices. I think you want backticks instead of single quotes here? https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:280: 3. Allocating memory quota. It seems to me that "tracking" is a slightly better fit than "allocating", because the latter makes me think that the controller allocates memory (malloc / new).
Thanks for all the comments! I did all of the corrections, and I also added a section for "Blob Limits" and "Common Pitfalls" PTAL! https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... File storage/browser/blob/README.md (right): https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:15: 3. Blobs can be 'sliced', which creates a blob that is a subsection of another On 2017/01/20 02:10:54, pwnall wrote: > How about having the word "sliced" link to > https://developer.mozilla.org/docs/Web/API/Blob/slice Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:17: 4. Reading blobs is asynchronous. On 2017/01/20 02:10:55, pwnall wrote: > Is it worth noting that obtaining blob metadata (especially size) is > synchronous? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:28: Blobs are created in the renderer process, where their data is temporarily held On 2017/01/20 02:10:55, pwnall wrote: > in _a_ renderer process? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:31: all data is transported and construction is complete, any pending reads for the On 2017/01/20 02:10:54, pwnall wrote: > I'd emphasize the word "transported" in some way, because you're effectively > defining it here, and the word is used elsewhere later. So, it's a concept too. > > I'd also say "all data is transported from the renderer to the browser", because > the redundancy should help readers get adjusted to the new concepts. > > I'd qualify "construction" as "blob construction". You used the word "created" > for the same concept earlier in the paragraph, so I wouldn't be 100% sure about > what you're referring to. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:32: blob are allowed to complete. Blobs can be small (bytes) or huge (GBs), so On 2017/01/20 02:10:54, pwnall wrote: > "small (bytes)" does not seem to add value here Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:43: General Chrome terminology: On 2017/01/20 02:10:55, pwnall wrote: > I'd like to > https://www.chromium.org/developers/design-documents/multi-process-architecture > as the authoritative source for these descriptions. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:63: can also represent "future" file and "future" bytes, which is used to signify a On 2017/01/20 02:10:54, pwnall wrote: > "future" files? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:65: * **dependent blobs**: These are blobs that our blob depends on to be On 2017/01/20 02:10:54, pwnall wrote: > "blobs that a blob has data dependencies on"? > > The word "dependent" seems a bit confusing to me here, because my dependents are > people who depend on me, whereas in this case a blob depends on the data in its > "dependents". > > Would it be terribly hard to switch dependent blobs to referenced blobs? mmmmmmm I think I saw it as I'm 'dependent' on these blobs. Since we use this terminology thoughout the code, I'm going to keep it. But I'll clarify. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:66: constructed. As in, we were constructed with a dependency on another blob On 2017/01/20 02:10:55, pwnall wrote: > I'm not a big fan of "we" and "our" usage here. I'd prefer to have clear > subjects and objects. > > At the same time, your usage of pronouns definitely makes the document shorter, > and resolving them is not too much mental effort. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:70: * **transportation strategy**: We can have one of 3 transportation strategies On 2017/01/20 02:10:55, pwnall wrote: > : a method for sending the data in a BlobItem from a renderer to the browser. > > You can add that the system currently implements three strategies: IPC, Shared > Memory, Files. I'm slightly skeptical about the benefits of documenting specific > methods, as the code is pretty obvious, and can change. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:75: ### Building On 2017/01/20 02:10:54, pwnall wrote: > ## instead of ###? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:77: built using a `BlobDataBuilder`, and as long as you don't use any On 2017/01/20 02:10:55, pwnall wrote: > any chance you could move the caveat after the main point of the phrase? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:83: `AppendFuture*` methods no the builder, but you must use On 2017/01/20 02:10:55, pwnall wrote: > no -> on? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:88: On 2017/01/20 02:10:55, pwnall wrote: > In general, this sections seems to assume that I've somehow read enough of the > headers to know the method names, but not enough to know which ones to use. Most > of it seems like it'd be a good fit for class-level documentation. > > I think I'd find this section more useful if it had an example for the happy > path, and pointers to headers that contain alternative methods, and describe the > caveats you mentioned. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:96: done or broken (due to not enough space, filesystem error, etc). On 2017/01/20 02:10:54, pwnall wrote: > done -> constructed? > > broken (construction failed due to ... Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:130: ## Blob Transportation, Renderer On 2017/01/20 02:10:54, pwnall wrote: > I think it'd be more consistent to end the heading in "(Renderer)" Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:142: 1. Send items and populate the data in IPC ([code]( On 2017/01/20 02:10:54, pwnall wrote: > I think line-level links like this one are quite brittle, because any code > change will drive them out of date. Please consider adding a search query, like > below, to make the link a bit more robust. > > https://cs.chromium.org/chromium/src/content/child/blob_storage/blob_transpor... > > Alternatively, you can write out the link in text, like this. -- Look for "case > IPCBlobItemRequestStrategy::IPC" in `blob_transport_controller.cc`. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:152: data. It does this by using the [incrementing and decrementing the process ref On 2017/01/20 02:10:54, pwnall wrote: > remove "using the"? > > also, ref -> reference? > Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:156: # Blob Transportation & Storage (Browser). On 2017/01/20 02:10:54, pwnall wrote: > the period looks inappropriate here Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:161: 2. If so, how do we want to transport it? IPC? Shared Memory? IPC? On 2017/01/20 02:10:54, pwnall wrote: > Do you mean "File" instead of the last "IPC"? > > Alternatively, consider using the keywords you introduced earlier. For example > -- Pick transport strategies for the blob's components. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:162: 3. Can I save this in memory right now? Or do I need to wait for older blob On 2017/01/20 02:10:55, pwnall wrote: > Does this mean "Is there enough free memory to transport the blob right now?" Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:172: 2. Create our browser-side representation of the blob data, including any data On 2017/01/20 02:10:54, pwnall wrote: > our -> the Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:173: items from dependent blobs. We try to share data items as much as possible, and On 2017/01/20 02:10:54, pwnall wrote: > Does the 2nd sentence here mean that data items are shared between blobs to save > memory? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:176: manages our blob storage limits. Quota can be requested for both transportation On 2017/01/20 02:10:54, pwnall wrote: > can be requested -> is necessary? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:189: (without thinking about blob dependencies) with diagrams and details in [this On 2017/01/20 02:10:54, pwnall wrote: > thinking about -> accounting for Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:195: from the renderer to the browser. When the initial description of the blob is On 2017/01/20 02:10:55, pwnall wrote: > I like "description of the blob" / "initial description of the blob" as a term. > Would it be worth explaining what this description consists of somewhere up > above? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:197: 'strategy' (IPC, Shared Memory, or File) it should use to transport the file. On 2017/01/20 02:10:54, pwnall wrote: > I don't think you need quotes here, you introduced the term earlier. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:198: Based on this strategy it can transform the memory items sent from the renderer On 2017/01/20 02:10:54, pwnall wrote: > transform -> translate? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:204: Once the transport host decides it's strategy, it will create it's own On 2017/01/20 02:10:55, pwnall wrote: > it's -> its (twice) Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:210: transport the blob data, this class's responsability is to populate the On 2017/01/20 02:10:54, pwnall wrote: > class' ? > > Or, better yet, "the transport host populates ..., and then signals ..." Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:230: * When this is approved, it notifies the `BlobTransportHost` that it can On 2017/01/20 02:10:55, pwnall wrote: > it notifies -> notify (for consistency with the other steps in the list) > > Also, "when the quota request is granted"? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:232: 4. Ask the `BlobMemoryManager` for memory quota for any copies necessary from On 2017/01/20 02:10:55, pwnall wrote: > necessary for blob slicing? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:234: 5. Adds completion callbacks to any dependent blobs that our blob depends on. On 2017/01/20 02:10:54, pwnall wrote: > the word "dependent" here seems redundant Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:249: The BlobStatus outlines this procedure (specifically the transport process), On 2017/01/20 02:10:54, pwnall wrote: > As a reader, I am unsure what "this procedure" refers to. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:253: Once a blob is finished constructing, the status is set to `DONE`, or any of On 2017/01/20 02:10:54, pwnall wrote: > I think you can say "to `DONE`, or to one of the `ERR_*` values". The meaning of > the prefix seems pretty obvious to me. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:258: During construction, 'slices' are created for dependent blobs using the given On 2017/01/20 02:10:54, pwnall wrote: > I don't think slices needs quotes here. It's a concept you introduced earlier. Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:270: representation of the new blob, where all blob references are replaced with the On 2017/01/20 02:10:54, pwnall wrote: > remove "the"? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:271: 'BlobSlice' items. It also stores any copy data from the slices. On 2017/01/20 02:10:55, pwnall wrote: > I think you want backticks instead of single quotes here? Done. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:280: 3. Allocating memory quota. On 2017/01/20 02:10:55, pwnall wrote: > It seems to me that "tracking" is a slightly better fit than "allocating", > because the latter makes me think that the controller allocates memory (malloc / > new). Done.
LGTM, because I think the docs are as good as they'll get, without a code change. https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... File storage/browser/blob/README.md (right): https://codereview.chromium.org/2637023003/diff/40001/storage/browser/blob/RE... storage/browser/blob/README.md:65: * **dependent blobs**: These are blobs that our blob depends on to be On 2017/01/20 20:23:17, dmurph wrote: > On 2017/01/20 02:10:54, pwnall wrote: > > "blobs that a blob has data dependencies on"? > > > > The word "dependent" seems a bit confusing to me here, because my dependents > are > > people who depend on me, whereas in this case a blob depends on the data in > its > > "dependents". > > > > Would it be terribly hard to switch dependent blobs to referenced blobs? > > mmmmmmm I think I saw it as I'm 'dependent' on these blobs. Since we use this > terminology thoughout the code, I'm going to keep it. But I'll clarify. Precisely -- this blob is dependent on the other blobs. When you call them dependent blobs, the "dependent" adjective modifies "blobs", implying that those blobs depend on something. But those blobs are dependencies, not dependents. https://codereview.chromium.org/2637023003/diff/60001/storage/browser/blob/RE... File storage/browser/blob/README.md (right): https://codereview.chromium.org/2637023003/diff/60001/storage/browser/blob/RE... storage/browser/blob/README.md:68: So think "I am dependent on these other blobs". Close parenthesis, or remove the opening one.
https://codereview.chromium.org/2637023003/diff/60001/storage/browser/blob/RE... File storage/browser/blob/README.md (right): https://codereview.chromium.org/2637023003/diff/60001/storage/browser/blob/RE... storage/browser/blob/README.md:68: So think "I am dependent on these other blobs". On 2017/01/21 02:56:23, pwnall wrote: > Close parenthesis, or remove the opening one. Done.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2637023003/#ps80001 (title: "comments")
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": 80001, "attempt_start_ts": 1485798581424320,
"parent_rev": "933415de4dc11b5f983ba3f48ea5d85e42d814f8", "commit_rev":
"224e43ce1ca4f6ca6f2cd8d677b8684f4b7c2351"}
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Adding explainer for blob storage system. BUG=682059 ========== to ========== [BlobStorage] Adding explainer for blob storage system. BUG=682059 Review-Url: https://codereview.chromium.org/2637023003 Cr-Commit-Position: refs/heads/master@{#447012} Committed: https://chromium.googlesource.com/chromium/src/+/224e43ce1ca4f6ca6f2cd8d677b8... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/224e43ce1ca4f6ca6f2cd8d677b8... |
