Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2140993002:
Add some unit tests for MemoryCache. (Closed)
|
Created:
4 years, 5 months ago by HyungwookLee Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd FragmentIdentifier(), MakeLiveAndDead(), RemoveURLFromCache() unit tests for MemoryCache.
BUG=625597
R=morrita@chromium.org
Committed: https://crrev.com/de7903c84701c01184c6ef2593c274f4d5de2bb0
Cr-Commit-Position: refs/heads/master@{#408026}
Patch Set 1 #
Total comments: 6
Patch Set 2 #
Total comments: 3
Patch Set 3 #Messages
Total messages: 17 (7 generated)
PTAL.
Description was changed from ========== Add some unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org ========== to ========== Add some unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org ==========
hyungwook.lee@navercorp.com changed reviewers: + japhet@chromium.org
Seems reasonable, just some formatting nitpicks. Also, please not in the patch description what test cases you're adding. https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp (right): https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:430: FakeResource* resource = FakeResource::create(ResourceRequest("http://test/resource#foo"), Resource::Raw); Ideally we wouldn't have "http://test/resource#foo" written out twice. https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:466: const KURL url1 = KURL(ParsedURLString, "http://test/resource1"); Declare url1 earlier and use it for resource1 init? https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:474: const KURL url2 = KURL(ParsedURLString, "http://test/resource2#foo"); Ditto?
Description was changed from ========== Add some unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org ========== to ========== Add FragmentIdentifier(), MakeLiveAndDead(), RemoveURLFromCache() unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org ==========
https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp (right): https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:430: FakeResource* resource = FakeResource::create(ResourceRequest("http://test/resource#foo"), Resource::Raw); On 2016/07/21 22:30:44, Nate Chapin wrote: > Ideally we wouldn't have "http://test/resource#foo" written out twice. Done. https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:466: const KURL url1 = KURL(ParsedURLString, "http://test/resource1"); On 2016/07/21 22:30:44, Nate Chapin wrote: > Declare url1 earlier and use it for resource1 init? Done. https://codereview.chromium.org/2140993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:474: const KURL url2 = KURL(ParsedURLString, "http://test/resource2#foo"); On 2016/07/21 22:30:44, Nate Chapin wrote: > Ditto? Done.
LGTM https://codereview.chromium.org/2140993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp (right): https://codereview.chromium.org/2140993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:437: const KURL url2 = KURL(ParsedURLString, "http://test/resource"); Optional: url2 = url1.removeFragmentIdentifier();
https://codereview.chromium.org/2140993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp (right): https://codereview.chromium.org/2140993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:437: const KURL url2 = KURL(ParsedURLString, "http://test/resource"); On 2016/07/26 19:14:18, Nate Chapin wrote: > Optional: url2 = url1.removeFragmentIdentifier(); Sorry, I meant MemoryCache::removeFragmentIdentifierIfNeeded(url1).
https://codereview.chromium.org/2140993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp (right): https://codereview.chromium.org/2140993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:437: const KURL url2 = KURL(ParsedURLString, "http://test/resource"); On 2016/07/26 19:15:22, Nate Chapin wrote: > On 2016/07/26 19:14:18, Nate Chapin wrote: > > Optional: url2 = url1.removeFragmentIdentifier(); > > Sorry, I meant MemoryCache::removeFragmentIdentifierIfNeeded(url1). Done.
The CQ bit was checked by hyungwook.lee@navercorp.com
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2140993002/#ps40001 (title: "")
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 ========== Add FragmentIdentifier(), MakeLiveAndDead(), RemoveURLFromCache() unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org ========== to ========== Add FragmentIdentifier(), MakeLiveAndDead(), RemoveURLFromCache() unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add FragmentIdentifier(), MakeLiveAndDead(), RemoveURLFromCache() unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org ========== to ========== Add FragmentIdentifier(), MakeLiveAndDead(), RemoveURLFromCache() unit tests for MemoryCache. BUG=625597 R=morrita@chromium.org Committed: https://crrev.com/de7903c84701c01184c6ef2593c274f4d5de2bb0 Cr-Commit-Position: refs/heads/master@{#408026} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/de7903c84701c01184c6ef2593c274f4d5de2bb0 Cr-Commit-Position: refs/heads/master@{#408026}
Message was sent while issue was closed.
|
