Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2040833002:
AppCache: Add nullptr check where AppCacheStorageImpl::disk_cache() is called (Closed)
|
Created:
4 years, 6 months ago by nhiroki Modified:
4 years, 6 months ago CC:
chromium-reviews, michaeln, darin-cc_chromium.org, jam, 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. |
DescriptionAppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache()
BUG=617200
Committed: https://crrev.com/9a7f903e95b9ff3b16dc18b8ab6d6eaa4f1a987d
Cr-Commit-Position: refs/heads/master@{#397989}
Patch Set 1 #
Total comments: 4
Patch Set 2 : ensure disk_cache_ #
Total comments: 2
Messages
Total messages: 22 (9 generated)
Description was changed from ========== void Peer::contextDestroyed() AppCache: Add nullptr check where AppCacheStorageImpl::disk_cache() is called BUG=617200 ========== to ========== AppCache: Add nullptr check where AppCacheStorageImpl::disk_cache() is called BUG=617200 ==========
Description was changed from ========== AppCache: Add nullptr check where AppCacheStorageImpl::disk_cache() is called BUG=617200 ========== to ========== AppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache() BUG=617200 ==========
nhiroki@chromium.org changed reviewers: + falken@chromium.org, michaeln@chromium.org
PTAL, thanks!
lgtm
Description was changed from ========== AppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache() BUG=617200 ========== to ========== AppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache() BUG=617200 ==========
nhiroki@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko, can you review this as an OWNER of content/ because michaeln@ seems to be ooo for a while?
https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_storage_impl.cc (right): https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_storage_impl.cc:1816: if (is_disabled_) { nit: !disk_cache() and is_disabled_ seems to have slight difference, did you make sure this change exactly does what's expected?
https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_storage_impl.cc (right): https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_storage_impl.cc:1816: if (is_disabled_) { On 2016/06/06 04:06:17, kinuko wrote: > nit: !disk_cache() and is_disabled_ seems to have slight difference, did you > make sure this change exactly does what's expected? disk_cache() returns nullptr only if is_disabled_ is true. When a diskcache instance hasn't been created yet, disk_cache() creates a new one and returns it. So, I think this is completely equal to the previous.
https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_storage_impl.cc (right): https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_storage_impl.cc:1816: if (is_disabled_) { On 2016/06/06 04:41:35, nhiroki wrote: > On 2016/06/06 04:06:17, kinuko wrote: > > nit: !disk_cache() and is_disabled_ seems to have slight difference, did you > > make sure this change exactly does what's expected? > > disk_cache() returns nullptr only if is_disabled_ is true. When a diskcache > instance hasn't been created yet, disk_cache() creates a new one and returns it. > So, I think this is completely equal to the previous. It seems to mean disk_cache() has side-effect, is directly accessing disk_cache_ below on line 1825 always safe in new code? (just checking)
Thank you. Updated! https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_storage_impl.cc (right): https://codereview.chromium.org/2040833002/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_storage_impl.cc:1816: if (is_disabled_) { On 2016/06/06 04:56:21, kinuko wrote: > On 2016/06/06 04:41:35, nhiroki wrote: > > On 2016/06/06 04:06:17, kinuko wrote: > > > nit: !disk_cache() and is_disabled_ seems to have slight difference, did you > > > make sure this change exactly does what's expected? > > > > disk_cache() returns nullptr only if is_disabled_ is true. When a diskcache > > instance hasn't been created yet, disk_cache() creates a new one and returns > it. > > So, I think this is completely equal to the previous. > > It seems to mean disk_cache() has side-effect, is directly accessing disk_cache_ > below on line 1825 always safe in new code? (just checking) Ohh, good point! I'm not 100% sure that disk_cache_ has been ensured before line 1825. Maybe we can call disk_cache() on line 1825 instead of directly accessing disk_cache. Changed.
lgtm https://codereview.chromium.org/2040833002/diff/20001/content/browser/appcach... File content/browser/appcache/appcache_storage_impl.cc (right): https://codereview.chromium.org/2040833002/diff/20001/content/browser/appcach... content/browser/appcache/appcache_storage_impl.cc:1825: int rv = disk_cache()->DoomEntry( Actually I feel dcheck'ing disk_cache_ might be just ok, but not super sure either... I assume changing this disk_cache() is not that harmful as it's what we used to be doing
https://codereview.chromium.org/2040833002/diff/20001/content/browser/appcach... File content/browser/appcache/appcache_storage_impl.cc (right): https://codereview.chromium.org/2040833002/diff/20001/content/browser/appcach... content/browser/appcache/appcache_storage_impl.cc:1825: int rv = disk_cache()->DoomEntry( On 2016/06/06 06:59:06, kinuko wrote: > Actually I feel dcheck'ing disk_cache_ might be just ok, but not super sure > either... I assume changing this disk_cache() is not that harmful as it's what > we used to be doing Acknowledged.
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2040833002/#ps20001 (title: "ensure disk_cache_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2040833002/20001
Message was sent while issue was closed.
Description was changed from ========== AppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache() BUG=617200 ========== to ========== AppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache() BUG=617200 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== AppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache() BUG=617200 ========== to ========== AppCache: Check whether AppCacheStorageImpl is enabled before calling disk_cache() BUG=617200 Committed: https://crrev.com/9a7f903e95b9ff3b16dc18b8ab6d6eaa4f1a987d Cr-Commit-Position: refs/heads/master@{#397989} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9a7f903e95b9ff3b16dc18b8ab6d6eaa4f1a987d Cr-Commit-Position: refs/heads/master@{#397989} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
