Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503473004:
Server push cancellation: add a new class HttpCacheLookupManager which implements ServerPushDelegate (Closed)
|
Created:
4 years, 1 month ago by Zhongyi Shi Modified:
4 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, kinuko+cache_chromium.org, Buck Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServer push cancellation: add a new class HttpCacheLookupManager which implements ServerPushDelegate
and can create cache transaction to lookup whether the server push has a cached response.
BUG=232040
Committed: https://crrev.com/8f72aeb11c5f8c8824a335fb72aadfa5b03df674
Cr-Commit-Position: refs/heads/master@{#433983}
Patch Set 1 #
Total comments: 31
Patch Set 2 : address rch's comments 1 #
Total comments: 6
Patch Set 3 : Use LookupTransaction::StartLookup #
Total comments: 1
Patch Set 4 : sync with master #
Total comments: 5
Patch Set 5 : address rch's comments #
Total comments: 14
Patch Set 6 : address jkarlin's comments #Patch Set 7 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into SPC_CacheLookupDelegate #
Messages
Total messages: 29 (13 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
zhongyi@chromium.org changed reviewers: + rch@chromium.org
sweet https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:15: request.reset(new HttpRequestInfo()); nit: you can do both of these in the initializer list. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:17: request->method = "GET"; Is GET the only method which can be pushed? https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:18: request->load_flags = LOAD_ONLY_FROM_CACHE | LOAD_SKIP_CACHE_VALIDATION; I'm not super familiar with all the details of the cache. Why skip validation? https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:35: return; Why does simply returning cancel this push? Is that 'cause of the destructor of ServerPushHelper? https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:38: new LookupTransaction(std::move(push_helper))); nit: auto lookup = base::MakeUnique<LookupTransaction>(std::move(push_helper)); https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:47: lookup_transactions_[pushed_url] = std::move(lookup_transaction); Perhaps this should only be done if Start returns ERR_IO_PENDING? https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:52: net_log_); I think it might be simpler to move all of this logic into LookupTransaction and have it expose something like int StartLookup(HttpCache* cache, CompletionCallback callback); This would both create the transaction and start it. (You'd also need to make LookupTransaction a class, but I think that would be a good idea anyway. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:54: if (rv == ERR_CACHE_MISS) { What if start returns some other error? https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.h (right): https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:15: class NET_EXPORT_PRIVATE HttpCacheLookupManager : public ServerPushDelegate { Class comment, please. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:17: explicit HttpCacheLookupManager(HttpCache* http_cache, Can you comment on the lifetime of |http_cache|. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:18: const NetLogWithSource& net_log); nit: no need for explicit. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:23: void OnPushFilteringComplete(const GURL& url, int rv); Comment. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:26: struct LookupTransaction { comment.
Patchset #2 (id:60001) has been deleted
Thanks for the review, Ryan, ptal! https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:15: request.reset(new HttpRequestInfo()); On 2016/11/15 23:16:06, Ryan Hamilton wrote: > nit: you can do both of these in the initializer list. Done. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:17: request->method = "GET"; On 2016/11/15 23:16:06, Ryan Hamilton wrote: > Is GET the only method which can be pushed? I believe both "GET" and "HEAD" can be pushed for QUIC, and only "GET" for SPDY(https://cs.chromium.org/chromium/src/net/spdy/spdy_http_stream.cc?sq=package:chromium&rcl=1479284732&l=75). https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:18: request->load_flags = LOAD_ONLY_FROM_CACHE | LOAD_SKIP_CACHE_VALIDATION; On 2016/11/15 23:16:06, Ryan Hamilton wrote: > I'm not super familiar with all the details of the cache. Why skip validation? Cauz this is the step 1: we only want to check whether the url is cached in the HttpCache. Do we want to validating the cache entry? https://cs.chromium.org/chromium/src/net/http/http_cache_transaction.cc?sq=pa... https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:35: return; On 2016/11/15 23:16:06, Ryan Hamilton wrote: > Why does simply returning cancel this push? Is that 'cause of the destructor of > ServerPushHelper? Sorry for the confusion, I meant to say cancel send out the lookup transaction. We won't cancel this push but cancel the lookup for this push. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:38: new LookupTransaction(std::move(push_helper))); On 2016/11/15 23:16:06, Ryan Hamilton wrote: > nit: auto lookup = base::MakeUnique<LookupTransaction>(std::move(push_helper)); Done. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:47: lookup_transactions_[pushed_url] = std::move(lookup_transaction); On 2016/11/15 23:16:06, Ryan Hamilton wrote: > Perhaps this should only be done if Start returns ERR_IO_PENDING? Ah, good suggestion! I then could skip storing in the ERR_CACHE_MISS case. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:52: net_log_); On 2016/11/15 23:16:05, Ryan Hamilton wrote: > I think it might be simpler to move all of this logic into LookupTransaction and > have it expose something like > > int StartLookup(HttpCache* cache, CompletionCallback callback); > > This would both create the transaction and start it. (You'd also need to make > LookupTransaction a class, but I think that would be a good idea anyway. It's hard because in that case we will have the manager having a map storing the lookup_transactions(which is already what we are doing now). And the lookup transaction will then need a extra pointer to manager to notify the manager when the callback has been invoked and the lookup transaction can be deleted from the map. If that doesn't sound problematic, I am fine to move in that direction. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:54: if (rv == ERR_CACHE_MISS) { On 2016/11/15 23:16:06, Ryan Hamilton wrote: > What if start returns some other error? I think wither ERR_IO_PENDING or ERR_CACHE_MISS. ERR_IO_PENDING eventually will call OnPushFilteringComplete which also removes the lookup from the map. However, I now just changed the code to only store the lookup if ERR_IO_PENDING. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.h (right): https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:15: class NET_EXPORT_PRIVATE HttpCacheLookupManager : public ServerPushDelegate { On 2016/11/15 23:16:06, Ryan Hamilton wrote: > Class comment, please. Done. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:17: explicit HttpCacheLookupManager(HttpCache* http_cache, On 2016/11/15 23:16:06, Ryan Hamilton wrote: > Can you comment on the lifetime of |http_cache|. Done. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:18: const NetLogWithSource& net_log); On 2016/11/15 23:16:06, Ryan Hamilton wrote: > nit: no need for explicit. Done. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:23: void OnPushFilteringComplete(const GURL& url, int rv); On 2016/11/15 23:16:06, Ryan Hamilton wrote: > Comment. Done. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:26: struct LookupTransaction { On 2016/11/15 23:16:06, Ryan Hamilton wrote: > comment. Done.
sweet https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:18: request->load_flags = LOAD_ONLY_FROM_CACHE | LOAD_SKIP_CACHE_VALIDATION; On 2016/11/16 21:04:42, Zhongyi Shi wrote: > On 2016/11/15 23:16:06, Ryan Hamilton wrote: > > I'm not super familiar with all the details of the cache. Why skip validation? > > Cauz this is the step 1: we only want to check whether the url is cached in the > HttpCache. Do we want to validating the cache entry? > https://cs.chromium.org/chromium/src/net/http/http_cache_transaction.cc?sq=pa... Ah! I wasn't actually sure what validation meant there :> I was thinking "is the cache corrupt", which is totally wrong! How 'bout a comment? https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:52: net_log_); On 2016/11/16 21:04:42, Zhongyi Shi wrote: > On 2016/11/15 23:16:05, Ryan Hamilton wrote: > > I think it might be simpler to move all of this logic into LookupTransaction > and > > have it expose something like > > > > int StartLookup(HttpCache* cache, CompletionCallback callback); > > > > This would both create the transaction and start it. (You'd also need to make > > LookupTransaction a class, but I think that would be a good idea anyway. > > It's hard because in that case we will have the manager having a map storing the > lookup_transactions(which is already what we are doing now). And the lookup > transaction will then need a extra pointer to manager to notify the manager when > the callback has been invoked and the lookup transaction can be deleted from the > map. If that doesn't sound problematic, I am fine to move in that direction. I may totally be missing the problem you're thinking about. I was imagining: auto lookup = base::MakeUnique<LookupTransaction>(std::move(push_helper)); int rv = lookup->StartLookup(http_cache_, base::Bind(...)); if (rv == ERR_IO_PENDING) { lookup_transactions_[pushed_url] = std::move(lookup); } Would that work? That being said, the code here is much simpler now so keeping it the way you have it now is also fine. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:54: if (rv == ERR_CACHE_MISS) { On 2016/11/16 21:04:42, Zhongyi Shi wrote: > On 2016/11/15 23:16:06, Ryan Hamilton wrote: > > What if start returns some other error? > > I think wither ERR_IO_PENDING or ERR_CACHE_MISS. ERR_IO_PENDING eventually will > call OnPushFilteringComplete which also removes the lookup from the map. > However, I now just changed the code to only store the lookup if ERR_IO_PENDING. I think that makes sense. The callback will only be invoked after this method returns ERR_IO_PENDING. So if it returns anything else, the callback will not be invoked and we need to cleanup now. https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:41: if (rv != OK) Can this return ERR_IO_PENDING? It looks like no. how 'bout: DCHECK_NE(ERR_IO_PENDING, rv); https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.h (right): https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:17: // response to the pushed URL is cached. How about: An implementation of ServerPushDelegate that issues an HttpCache::Transaction to lookup whether the pushed URL is cached and cancel the push in that case. https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:29: void OnPushFilteringComplete(const GURL& url, int rv); nit: How about simply OnLookupComplete?
zhongyi@chromium.org changed reviewers: + jkarlin@chromium.org
+jkarlin, could you help review the FLAG setting in PS1 LookupTransaction::LookupTransaction()? Ryan and I were thinking about skip response validation as the main goal to issue the HttpCache::Transaction currently is to find whether the response to the pushed URL is cached based on URL. For sure we want a valid cache entry. Thanks!
Update: Move start transaction logic to LookupTransaction::StartLookup(). Leave the HttpCache::Transaction flags setting to jkarlin for review. Ryan, ptal on the rest of the code, thanks! https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:52: net_log_); On 2016/11/17 22:47:24, Ryan Hamilton wrote: > On 2016/11/16 21:04:42, Zhongyi Shi wrote: > > On 2016/11/15 23:16:05, Ryan Hamilton wrote: > > > I think it might be simpler to move all of this logic into LookupTransaction > > and > > > have it expose something like > > > > > > int StartLookup(HttpCache* cache, CompletionCallback callback); > > > > > > This would both create the transaction and start it. (You'd also need to > make > > > LookupTransaction a class, but I think that would be a good idea anyway. > > > > It's hard because in that case we will have the manager having a map storing > the > > lookup_transactions(which is already what we are doing now). And the lookup > > transaction will then need a extra pointer to manager to notify the manager > when > > the callback has been invoked and the lookup transaction can be deleted from > the > > map. If that doesn't sound problematic, I am fine to move in that direction. > > I may totally be missing the problem you're thinking about. I was imagining: > > auto lookup = base::MakeUnique<LookupTransaction>(std::move(push_helper)); > int rv = lookup->StartLookup(http_cache_, base::Bind(...)); > > if (rv == ERR_IO_PENDING) { > lookup_transactions_[pushed_url] = std::move(lookup); > } > > Would that work? > > That being said, the code here is much simpler now so keeping it the way you > have it now is also fine. Acknowledged. https://codereview.chromium.org/2503473004/diff/40001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:54: if (rv == ERR_CACHE_MISS) { On 2016/11/17 22:47:24, Ryan Hamilton wrote: > On 2016/11/16 21:04:42, Zhongyi Shi wrote: > > On 2016/11/15 23:16:06, Ryan Hamilton wrote: > > > What if start returns some other error? > > > > I think wither ERR_IO_PENDING or ERR_CACHE_MISS. ERR_IO_PENDING eventually > will > > call OnPushFilteringComplete which also removes the lookup from the map. > > However, I now just changed the code to only store the lookup if > ERR_IO_PENDING. > > I think that makes sense. The callback will only be invoked after this method > returns ERR_IO_PENDING. So if it returns anything else, the callback will not be > invoked and we need to cleanup now. Acknowledged. https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.cc:41: if (rv != OK) On 2016/11/17 22:47:24, Ryan Hamilton wrote: > Can this return ERR_IO_PENDING? It looks like no. how 'bout: > > DCHECK_NE(ERR_IO_PENDING, rv); Actually, the only response from CreateTransaction should be OK. That implies we could just skip checking. https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... File net/http/http_cache_lookup_manager.h (right): https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:17: // response to the pushed URL is cached. On 2016/11/17 22:47:24, Ryan Hamilton wrote: > How about: > > An implementation of ServerPushDelegate that issues an HttpCache::Transaction to > lookup whether the pushed URL is cached and cancel the push in that case. Done. https://codereview.chromium.org/2503473004/diff/80001/net/http/http_cache_loo... net/http/http_cache_lookup_manager.h:29: void OnPushFilteringComplete(const GURL& url, int rv); On 2016/11/17 22:47:24, Ryan Hamilton wrote: > nit: How about simply OnLookupComplete? Done. https://codereview.chromium.org/2503473004/diff/100001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager.h (right): https://codereview.chromium.org/2503473004/diff/100001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.h:32: // A class that takes the ownership of ServerPushHelper and issues and owns an The comment has been updated in next PS // // A class that takes the ownership of ServerPushHelper, issues and owns an
Looks great. 2 nits. https://codereview.chromium.org/2503473004/diff/120001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/120001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.cc:46: if (lookup_transactions_.find(pushed_url) != lookup_transactions_.end()) nit: if (ContainsKey(lookup_transactions_, pushed_url)) { https://codereview.chromium.org/2503473004/diff/120001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.cc:58: } nit: no {}s on 1-line ifs https://codereview.chromium.org/2503473004/diff/120001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.cc:59: } This method looks really clean now!
Update: address Ryan's comments. PTAL, thanks! https://codereview.chromium.org/2503473004/diff/120001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/120001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.cc:46: if (lookup_transactions_.find(pushed_url) != lookup_transactions_.end()) On 2016/11/18 20:47:02, Ryan Hamilton wrote: > nit: if (ContainsKey(lookup_transactions_, pushed_url)) { Done. https://codereview.chromium.org/2503473004/diff/120001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.cc:58: } On 2016/11/18 20:47:02, Ryan Hamilton wrote: > nit: no {}s on 1-line ifs > Done.
lgtm, modulo your question for jkarlin
https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.cc:25: request->load_flags = LOAD_ONLY_FROM_CACHE | LOAD_SKIP_CACHE_VALIDATION; So, if the resource is in the cache (valid or not) then you want to cancel the push? If so, these are the right flags. Why is it you want to cancel the push if the resource is stale in the cache? Because it's better to do a validation request than push all of the bytes? https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager.h (right): https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.h:49: std::unique_ptr<ServerPushHelper> push_helper; push_helper_ https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.h:50: std::unique_ptr<HttpRequestInfo> request; request_ https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.h:51: std::unique_ptr<HttpTransaction> transaction; transaction_ https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager_unittest.cc (right): https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager_unittest.cc:90: std::unique_ptr<MockServerPushHelper> push_helper( std::unique_ptr<MockServerPushHelper> push_helper = base::MakeUnique<MockServerPushHelper>(request_url); https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager_unittest.cc:217: // Test the server push lookup is base on the full url. s/base/based/ https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager_unittest.cc:249: // Receive another server push and should not cancel the push. How is this testing the full URL? Even if you used request_url on the second push it still wouldn't cancel, as evidenced by the test above this one.
jkarlin@: thanks for helping the review, PTAL! Updated: - address nits and comments - correct the unittest for Full Url matching. https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager.cc (right): https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.cc:25: request->load_flags = LOAD_ONLY_FROM_CACHE | LOAD_SKIP_CACHE_VALIDATION; On 2016/11/21 14:40:41, jkarlin wrote: > So, if the resource is in the cache (valid or not) then you want to cancel the > push? If so, these are the right flags. Yup, we want to cancel the server push if the url is found in cached. > Why is it you want to cancel the push if the resource is stale in the cache? > Because it's better to do a validation request than push all of the bytes? For the time being, the cancel rule is aggressive: just url matching. Server push is not used widely. To move forward, we want to use this rule first, and it turns out that we cancel too aggressive, we will then move forward with extra validation to cancel less often. We have the design doc here: http://goo.gl/OgWqwO https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager.h (right): https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.h:49: std::unique_ptr<ServerPushHelper> push_helper; On 2016/11/21 14:40:41, jkarlin wrote: > push_helper_ Ah, thanks for catching this! LookupTransaction used to a simple struct, I forgot to change the member variable name when switch to class. https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.h:50: std::unique_ptr<HttpRequestInfo> request; On 2016/11/21 14:40:41, jkarlin wrote: > request_ Done. https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager.h:51: std::unique_ptr<HttpTransaction> transaction; On 2016/11/21 14:40:41, jkarlin wrote: > transaction_ Done. https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... File net/http/http_cache_lookup_manager_unittest.cc (right): https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager_unittest.cc:90: std::unique_ptr<MockServerPushHelper> push_helper( On 2016/11/21 14:40:41, jkarlin wrote: > std::unique_ptr<MockServerPushHelper> push_helper = > base::MakeUnique<MockServerPushHelper>(request_url); Done. https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager_unittest.cc:217: // Test the server push lookup is base on the full url. On 2016/11/21 14:40:41, jkarlin wrote: > s/base/based/ Done. https://codereview.chromium.org/2503473004/diff/140001/net/http/http_cache_lo... net/http/http_cache_lookup_manager_unittest.cc:249: // Receive another server push and should not cancel the push. On 2016/11/21 14:40:41, jkarlin wrote: > How is this testing the full URL? Even if you used request_url on the second > push it still wouldn't cancel, as evidenced by the test above this one. Thanks for catching this, I added RunUntilIdle to make sure the previous lookup is complete. And I add a same server push lookup in between, make sure the same full url should be cancelled. Wait it until finish, then test a similar url, but should not cancel.
lgtm
The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks a lot for help reviewing this cl, landing now :)
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2503473004/#ps180001 (title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src into SPC_CacheLookupDelegate")
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": 180001, "attempt_start_ts": 1479849292832980,
"parent_rev": "3d59c9753a85767715585e11c0f509013a159be0", "commit_rev":
"ef4651919748796dfcc0749dccc83bff2ec0791e"}
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Server push cancellation: add a new class HttpCacheLookupManager which implements ServerPushDelegate and can create cache transaction to lookup whether the server push has a cached response. BUG=232040 ========== to ========== Server push cancellation: add a new class HttpCacheLookupManager which implements ServerPushDelegate and can create cache transaction to lookup whether the server push has a cached response. BUG=232040 Committed: https://crrev.com/8f72aeb11c5f8c8824a335fb72aadfa5b03df674 Cr-Commit-Position: refs/heads/master@{#433983} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8f72aeb11c5f8c8824a335fb72aadfa5b03df674 Cr-Commit-Position: refs/heads/master@{#433983} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
