Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2192823003:
[CacheStorage] Add README.md (Closed)
|
Created:
4 years, 4 months ago by jkarlin Modified:
4 years, 4 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, jkarlin+watch_chromium.org, nhiroki Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds a README.md to content/browser/cache_storage/.
BUG=632354
Committed: https://crrev.com/ebd9c3baf602a65d01151755ed7bedf983d35d01
Cr-Commit-Position: refs/heads/master@{#408629}
Patch Set 1 #Patch Set 2 : Nits #Patch Set 3 : Nits #
Total comments: 6
Patch Set 4 : Address comments from PS3 #Patch Set 5 : nit #Patch Set 6 : nit #Patch Set 7 : nits #
Total comments: 8
Patch Set 8 : Address comments from PS7 #Patch Set 9 : nits #
Total comments: 2
Patch Set 10 : Add date #Messages
Total messages: 19 (7 generated)
Description was changed from ========== [CacheStorage] Add documentation Adds a README.md to content/browser/cache_storage/. BUG=632354 ========== to ========== [CacheStorage] Add README.md Adds a README.md to content/browser/cache_storage/. BUG=632354 ==========
Description was changed from ========== [CacheStorage] Add README.md Adds a README.md to content/browser/cache_storage/. BUG=632354 ========== to ========== Adds a README.md to content/browser/cache_storage/. BUG=632354 ==========
jkarlin@chromium.org changed reviewers: + cmumford@chromium.org, jsbell@chromium.org, michaeln@chromium.org, nhiroki@chromium.org
All CacheStorage owners added as reviewers + cmumford@ who has recently started working on CacheStorage. PTAL!
lgtm https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... content/browser/cache_storage/README.md:11: #####`CacheStorageContextImpl`->`CacheStorageManager`-> `CacheStorage`->`CacheStorageCache` nit: consistent spacing https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... content/browser/cache_storage/README.md:135: operation freezes, then the scheduler is frozen. If a CacheStorage call winds Nit: backticks on `CacheStorage`, for consistency https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... content/browser/cache_storage/README.md:138: `CacheStorage` as well. This has happened in the past. Be careful to avoid Maybe give example (e.g. a put() op checking quota, which required checking size...)
Thanks! michaeln@, nhiroki@, cmumford@ PTAL https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... content/browser/cache_storage/README.md:11: #####`CacheStorageContextImpl`->`CacheStorageManager`-> `CacheStorage`->`CacheStorageCache` On 2016/07/28 16:25:34, jsbell wrote: > nit: consistent spacing Done. https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... content/browser/cache_storage/README.md:135: operation freezes, then the scheduler is frozen. If a CacheStorage call winds On 2016/07/28 16:25:34, jsbell wrote: > Nit: backticks on `CacheStorage`, for consistency Done. https://codereview.chromium.org/2192823003/diff/40001/content/browser/cache_s... content/browser/cache_storage/README.md:138: `CacheStorage` as well. This has happened in the past. Be careful to avoid On 2016/07/28 16:25:34, jsbell wrote: > Maybe give example (e.g. a put() op checking quota, which required checking > size...) Done.
lgtm % nits https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:8: Where '->' represents ownership, '--' is a reference, and '-/-' is a weak Just a thought, not a request for change. If you used '=>' and '->' for ownership/reference then the reader would know the direction of reference as well. Could do something like '-/->' or '~~>' for weak reference if that's not too subtle. https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:24: * currently running operations I believe you need two spaces for a nested list not one. https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:33: one of its `disk_cache::Entrys` is reachable. Otherwise, a new backend might Nit: No "Entrys" class. Maybe "one of its `disk_cache::Entry` instances is reachable"? Or put the "s" outside as done elsewhere. https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:122: 2. CacheStorage/ async methods often include several asynchronous steps. Each Is the / intentional or is another class name missing?
Thanks! michaeln@, nhiroki@ PTAL. https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:8: Where '->' represents ownership, '--' is a reference, and '-/-' is a weak On 2016/07/28 18:41:55, cmumford wrote: > Just a thought, not a request for change. If you used '=>' and '->' for > ownership/reference then the reader would know the direction of reference as > well. Could do something like '-/->' or '~~>' for weak reference if that's not > too subtle. Thanks, done. https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:24: * currently running operations On 2016/07/28 18:41:55, cmumford wrote: > I believe you need two spaces for a nested list not one. Done. https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:33: one of its `disk_cache::Entrys` is reachable. Otherwise, a new backend might On 2016/07/28 18:41:55, cmumford wrote: > Nit: No "Entrys" class. Maybe "one of its `disk_cache::Entry` instances is > reachable"? Or put the "s" outside as done elsewhere. Done. https://codereview.chromium.org/2192823003/diff/120001/content/browser/cache_... content/browser/cache_storage/README.md:122: 2. CacheStorage/ async methods often include several asynchronous steps. Each On 2016/07/28 18:41:55, cmumford wrote: > Is the / intentional or is another class name missing? Done.
https://codereview.chromium.org/2192823003/diff/160001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2192823003/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:1: # Architecture lgtm - maybe put a date on it, sometimes separate docs like this get out of date: Architecture (as of mm/dd/yy)
lgtm!
Thanks all! https://codereview.chromium.org/2192823003/diff/160001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2192823003/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:1: # Architecture On 2016/07/28 22:26:47, michaeln wrote: > lgtm - maybe put a date on it, sometimes separate docs like this get out of > date: Architecture (as of mm/dd/yy) Done.
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, cmumford@chromium.org, michaeln@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2192823003/#ps180001 (title: "Add date")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adds a README.md to content/browser/cache_storage/. BUG=632354 ========== to ========== Adds a README.md to content/browser/cache_storage/. BUG=632354 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Adds a README.md to content/browser/cache_storage/. BUG=632354 ========== to ========== Adds a README.md to content/browser/cache_storage/. BUG=632354 Committed: https://crrev.com/ebd9c3baf602a65d01151755ed7bedf983d35d01 Cr-Commit-Position: refs/heads/master@{#408629} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/ebd9c3baf602a65d01151755ed7bedf983d35d01 Cr-Commit-Position: refs/heads/master@{#408629}
Message was sent while issue was closed.
Link to doc: https://chromium.googlesource.com/chromium/src/+/ebd9c3baf602a65d01151755ed7b... |
