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

Issue 2784803004: [HTTP Cache] Prevent memory backend from exceeding its max size (Closed)

Created:
3 years, 8 months ago by jkarlin
Modified:
3 years, 8 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, net-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[HTTP Cache] Prevent memory backend from exceeding its max size Prevents the memory backend from writing additional bytes if they will cause the cache to exceed its max size. The HttpCache tests already verify that they can handle a failed write after the cache has already started writing, see HttpCache.SimpleGETWithDiskFailures2. BUG=705053 Review-Url: https://codereview.chromium.org/2784803004 Cr-Commit-Position: refs/heads/master@{#460826} Committed: https://chromium.googlesource.com/chromium/src/+/c383a4d067322844776e4029c86803748f0526d0

Patch Set 1 #

Patch Set 2 : Better implementation #

Patch Set 3 : Fail before adjusting memory size #

Patch Set 4 : Nits #

Total comments: 5

Patch Set 5 : Address comments from PS4 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -10 lines) Patch
M net/disk_cache/backend_unittest.cc View 1 2 3 4 2 chunks +46 lines, -7 lines 0 comments Download
M net/disk_cache/memory/mem_backend_impl.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M net/disk_cache/memory/mem_backend_impl.cc View 1 2 2 chunks +5 lines, -1 line 0 comments Download
M net/disk_cache/memory/mem_entry_impl.cc View 1 2 3 3 chunks +9 lines, -2 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 27 (19 generated)
jkarlin
rdsmith@ PTAL, thanks!
3 years, 8 months ago (2017-03-30 13:04:20 UTC) #5
jkarlin
https://codereview.chromium.org/2784803004/diff/60001/net/disk_cache/memory/mem_backend_impl.cc File net/disk_cache/memory/mem_backend_impl.cc (right): https://codereview.chromium.org/2784803004/diff/60001/net/disk_cache/memory/mem_backend_impl.cc#newcode51 net/disk_cache/memory/mem_backend_impl.cc:51: DCHECK_EQ(0, current_size_); Hmm. I'm skeptical that this DCHECK even ...
3 years, 8 months ago (2017-03-30 15:04:42 UTC) #12
Randy Smith (Not in Mondays)
I'm disturbed that I'm the best person you could find to review this, but I'm ...
3 years, 8 months ago (2017-03-30 16:38:11 UTC) #16
jkarlin
Thanks! In terms of propagating, the description shows that the HttpCache::Transaction smoothly proceeds if a ...
3 years, 8 months ago (2017-03-30 17:18:27 UTC) #19
Randy Smith (Not in Mondays)
LGTM.
3 years, 8 months ago (2017-03-30 17:20:46 UTC) #20
jkarlin
Thanks!
3 years, 8 months ago (2017-03-30 17:22:43 UTC) #23
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/2784803004/80001
3 years, 8 months ago (2017-03-30 17:22:43 UTC) #24
commit-bot: I haz the power
3 years, 8 months ago (2017-03-30 18:20:51 UTC) #27
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c383a4d067322844776e4029c868...

Powered by Google App Engine
This is Rietveld 408576698