Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1041993004:
content::ResourceDispatcherHostImpl changes for stale-while-revalidate (Closed)
|
Created:
5 years, 8 months ago by Adam Rice Modified:
3 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Randy Smith (Not in Mondays), loading-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@s-w-r-yhirano-patch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncontent::ResourceDispatcherHostImpl changes for stale-while-revalidate
Inform //net that we support async revalidation using the net::LOAD_SUPPORT_ASYNC_REVALIDATION flag. Issue async revalidations when response info had the async_revalidation_required flag.
See design doc at
https://docs.google.com/a/chromium.org/document/d/1nBhr25nSJgoyAh4S1-U5h2sH70Iz4RR0NAfXNL79G5Y/edit
BUG=348877
TEST=content_unittests
Committed: https://crrev.com/96e3ce421fbe8b8133bf9852b7d7cf16e8521026
Cr-Commit-Position: refs/heads/master@{#364253}
Patch Set 1 #Patch Set 2 : Add BeginAsyncRevalidation() #Patch Set 3 : Add NullResourceHandler and use it for async revalidations. #Patch Set 4 : Remove ability for the renderer to launch an async revalidation. #Patch Set 5 : Rebase. #Patch Set 6 : Formatting fixups. #Patch Set 7 : Make it compile. #Patch Set 8 : Revert change to chrome_browser_field_trials.cc. #Patch Set 9 : Actually trigger the async revalidation when needed. #Patch Set 10 : First working version. #Patch Set 11 : Add tests and make NullResourceHandler call ResourceDispatcherHostDelegate callbacks. #Patch Set 12 : Suppress duplicate async revalidations to the same URL. #
Total comments: 16
Patch Set 13 : Cleanups from tyoshino review. #
Total comments: 8
Patch Set 14 : Readability improvements suggested by tyoshino #
Total comments: 4
Patch Set 15 : Factor out refactoring of ResourceDispatcherHostImpl::DidReceiveResponse #Patch Set 16 : Comment why ResourceHostMsg_Request can be safely copied #
Total comments: 14
Patch Set 17 : Block SSL client cert selection and error recovery. #Patch Set 18 : Rebase. #Patch Set 19 : Remove dependency of ResourceScheduler on ResourceRequestInfo. #Patch Set 20 : WIP: Tests fail. #Patch Set 21 : Revert unneeded changes. #Patch Set 22 : Split ResourceScheduler changes into crrev.com/1257113002 #Patch Set 23 : Fix tiny regression. #Patch Set 24 : Rebase against parent CL. #Patch Set 25 : The tests work again. #Patch Set 26 : Rebase on latest ResourceScheduler changes; remove AsyncRevalidationDriver::Delegate. #Patch Set 27 : Rebase. #Patch Set 28 : Disable async revalidations for requests with redirects. #Patch Set 29 : Minor fixes. #Patch Set 30 : Rebase. #Patch Set 31 : Workaround iwyu bug in prerender_resource #
Total comments: 70
Patch Set 32 : Changes from davidben and mmenke reviews. #Patch Set 33 : Don't read from cache on 304. Add test for that case. Fix test failures. #Patch Set 34 : Copy only headers from the original IPC. Move most code to AsyncRevalidationManager. #Patch Set 35 : Remove unnecessary deferral logic from AsyncRevalidationDriver #
Total comments: 112
Patch Set 36 : Changes from bnc review. #Patch Set 37 : clang-format fixes. #Patch Set 38 : Remove unnecessary copied comment. #
Total comments: 31
Patch Set 39 : Rebase. #Patch Set 40 : Fixes from kinuko review. #Patch Set 41 : Functional change: An initial redirect leg is now async revalidated. s-w-r is still ignored after t… #
Total comments: 78
Patch Set 42 : Fixes from davidben review. #
Total comments: 8
Patch Set 43 : URLRequest to be copied is passed const ref. Vary is not a problem. Comment fixes by kinuko. #Patch Set 44 : Rebase. #
Total comments: 26
Patch Set 45 : Add actions AsyncRevalidationResponseTimeout and AsyncRevalidationReadTimeout to actions.xml. Remov… #Patch Set 46 : Fixes from kinuko review. #Patch Set 47 : Rebase and remove workaround for URLRequest cancel bug. #
Total comments: 10
Patch Set 48 : AsyncRevalidationDriver test reorganisation. Comment nits. #
Total comments: 31
Patch Set 49 : Remove Driver::CancelRequest(). Simplify tests. #
Total comments: 2
Patch Set 50 : Remove redundant test AsyncRevalidationManagerRecordingTest.InitialRedirectLegRevalidated #Patch Set 51 : Rebase. URLRequestJob is no longer ref-counted. #
Total comments: 2
Patch Set 52 : Use histograms instead of user actions. #Messages
Total messages: 71 (11 generated)
tyoshino@chromium.org changed reviewers: + tyoshino@chromium.org
https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1493: scoped_ptr<ResourceHandler> ResourceDispatcherHostImpl::AddStandardHandlers( now this should be called e.g. AddThrottlingResourceHandler? though it also contains RequestBeginning call. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2470: ResourceHostMsg_Request* original_request = info->original_request(); original_request_data? https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2471: DCHECK(original_request); blank line https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2472: ResourceHostMsg_Request new_request = *original_request; new_request_data? https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2479: new_request.priority = net::MINIMUM_PRIORITY; blank line https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2484: bool is_async_revalidation = true; blink line https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2495: } blank line https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2503: CHECK(ContainsKey(active_resource_contexts_, resource_context)); blank line
https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1493: scoped_ptr<ResourceHandler> ResourceDispatcherHostImpl::AddStandardHandlers( On 2015/06/12 08:20:44, tyoshino wrote: > now this should be called e.g. AddThrottlingResourceHandler? though it also > contains RequestBeginning call. I tentatively renamed it AddThrottlesAndSchedule(). I'm welcome to other suggestions. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2470: ResourceHostMsg_Request* original_request = info->original_request(); On 2015/06/12 08:20:44, tyoshino wrote: > original_request_data? Done. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2471: DCHECK(original_request); On 2015/06/12 08:20:43, tyoshino wrote: > blank line Done. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2472: ResourceHostMsg_Request new_request = *original_request; On 2015/06/12 08:20:44, tyoshino wrote: > new_request_data? Done. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2479: new_request.priority = net::MINIMUM_PRIORITY; On 2015/06/12 08:20:44, tyoshino wrote: > blank line Done. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2484: bool is_async_revalidation = true; On 2015/06/12 08:20:44, tyoshino wrote: > blink line Done. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2495: } On 2015/06/12 08:20:44, tyoshino wrote: > blank line Done. https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2503: CHECK(ContainsKey(active_resource_contexts_, resource_context)); On 2015/06/12 08:20:44, tyoshino wrote: > blank line Done.
https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:440: std::pair<net::HttpCache*, std::string> AsyncRevalidationKey( how about GetAsyncRevalidationKeyFor? or Create...? https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1318: QualifiesForAsyncRevalidation(request_data)); Could you please move "!is_async_revalidation" to the head of this condition to make it easier to know that this flag won't set when this method is called for async revalidation? The name "support_async_revalidation" looked a bit confusing when I saw it in checking the call path from BeginAsyncRevalidation(). I tried to rename it to something else so that it's clear that this is set only when this is not a BeginRequestFromData() call for BeginAsyncRevalidation. But couldn't. So, I'd request only order change in this line. https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1633: NULL); // original_message original_request https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2513: child_id, route_id, is_async_revalidation, Do the similar as L2090? I.e. true, // is_async_revalidation
https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:440: std::pair<net::HttpCache*, std::string> AsyncRevalidationKey( On 2015/06/15 09:30:39, tyoshino wrote: > how about GetAsyncRevalidationKeyFor? > > or Create...? I chose GenerateAsyncRevalidationKeyFor() to be like net::HttpCache::GenerateCacheKey(). https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1318: QualifiesForAsyncRevalidation(request_data)); On 2015/06/15 09:30:39, tyoshino wrote: > Could you please move "!is_async_revalidation" to the head of this condition to > make it easier to know that this flag won't set when this method is called for > async revalidation? > > The name "support_async_revalidation" looked a bit confusing when I saw it in > checking the call path from BeginAsyncRevalidation(). > > I tried to rename it to something else so that it's clear that this is set only > when this is not a BeginRequestFromData() call for BeginAsyncRevalidation. But > couldn't. So, I'd request only order change in this line. Done. https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1633: NULL); // original_message On 2015/06/15 09:30:39, tyoshino wrote: > original_request Done. https://codereview.chromium.org/1041993004/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2513: child_id, route_id, is_async_revalidation, On 2015/06/15 09:30:39, tyoshino wrote: > Do the similar as L2090? > > I.e. > > true, // is_async_revalidation Done.
https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:860: request->url().SchemeIs(url::kHttpScheme)) { you can split this refactoring into a small patch. https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... File content/browser/loader/resource_request_info_impl.cc (right): https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... content/browser/loader/resource_request_info_impl.cc:165: original_request_.reset(new ResourceHostMsg_Request(*original_request)); write some analysis (maybe in the CL description?) about safety of this cloning. The IPC protocol should have been designed assuming that there's only one code that receives and handles this message. We should explain that it's ok to reuse the values on it for another request (async revalidation).
Patchset #15 (id:280001) has been deleted
https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:860: request->url().SchemeIs(url::kHttpScheme)) { On 2015/06/16 13:04:42, tyoshino wrote: > you can split this refactoring into a small patch. Done: https://codereview.chromium.org/1183723006/ https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... File content/browser/loader/resource_request_info_impl.cc (right): https://codereview.chromium.org/1041993004/diff/260001/content/browser/loader... content/browser/loader/resource_request_info_impl.cc:165: original_request_.reset(new ResourceHostMsg_Request(*original_request)); On 2015/06/16 13:04:42, tyoshino wrote: > write some analysis (maybe in the CL description?) about safety of this cloning. > The IPC protocol should have been designed assuming that there's only one code > that receives and handles this message. We should explain that it's ok to reuse > the values on it for another request (async revalidation). It's not an IPC::Message, it's just a struct, so there's no particular problem with copying it. I've added an explanation of why we can copy it and a DCHECK that the potentially dangerous |request_body| member is not set.
davidben@chromium.org changed reviewers: + davidben@chromium.org
I think this is not lgtm until the design issues raised on net-dev have been resolved. I do not believe the RDH is a reasonable place for this logic. I skimmed the CL and noted some issues that I noticed at a glance. A more thorough review might find others. They're all of a similar theme where the code makes incorrect assumptions the behavior of code many layers down (HttpCache is very far from the RDH) or breaks API contracts made to layers above (the RDH exposes many hooks to //chrome which may not behave well against unexpected signals). The problem is not the assumptions themselves but that the RDH is too far away from the HTTP cache to make the decisions correctly. Even URLRequest is kind of high due to redirects. It probably should happen in HttpCache. Let's continue this on the net-dev thread for broader visibility and try to resolve the problems that motivated putting this in the RDH in the first place. (You cited HSTS, HPKP, and cookies. HSTS and HPKP I'm pretty confident could be moved to HttpNetworkTransaction without issues, and then that would Just Work. Cookies I'm less sure of the correct interaction with range requests, but we could easily factor that code out and call it in both the URLRequest layer and async revalidations.) https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:436: return false; This check can't be relied on enforce anything meaningful. What if it's an FTP URL or something more exotic? What if it's something exotic that internally or via extensions redirects back to HTTP? What if it's HTTP and an extension redirects it to something exotic? Revalidation is a property of an individual redirect leg, not the entire URLRequest. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:865: if (request->response_info().async_revalidation_required) Will ServiceWorker propagate this flag up? That would result in two async revalidations happening. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:866: BeginAsyncRevalidation(request); This is only called on the final redirect leg. What if an intermediate request required async revalidation? https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1320: load_flags |= net::LOAD_SUPPORT_ASYNC_REVALIDATION; This means that only requests which go through the RDH participate in async revalidation. Putting it at a lower layer means that our entire net stack understands it. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2465: net::URLRequest* for_request) { By using the RDH, requests are limited in lifetime to the RenderFrameHost, however the renderer does not know about the request. This is wrong. Logic should not be sensitive to whether a navigation switch processes or not; document switches may happen in-process without changing RFH which won't cancel these requests while cross-process switches will cancel the requests. Whether process swaps happen is not a tight policy and will likely change as Site Isolation progresses. One possible set of semantics that would be sound is that they're scoped to the *document*. But that requires that the renderer know about and own the request. However, I don't think that's the ideal set of semantics anyway. It's not well-defined for navigation requests. And, fundamentally, these requests are an internal implementation detail of the low-level HTTP caching logic, so it would not be reasonable for their lifetime to be bound by a web-platform construct. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2473: ResourceHostMsg_Request new_request_data = *original_request_data; BUG: These cannot retain the old resource type. Navigation-related resource requests have many special-cases due to navigation's layering violations (see PlzNavigate project, CrossSiteResourceHandler). https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2483: int route_id = info->GetRouteID(); This will cause UI to show up all throughout the stack without ResourceThrottles and the like. We explicitly do not want an association with the WebContents. Everything will need to special-case async revalidations which is a red flag for the design being at the wrong layer. (Not to mention ServiceWorker which no doubt complicates this considerably.)
https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2483: int route_id = info->GetRouteID(); On 2015/06/19 15:35:14, David Benjamin wrote: > This will cause UI to show up all throughout the stack without ResourceThrottles Sorry, that was a typo. "without" should be "with". ResourceThrottles sometimes trigger UI. (SafeBrowsing, for instance, hooks into interstitials.) > and the like. We explicitly do not want an association with the WebContents. > Everything will need to special-case async revalidations which is a red flag for > the design being at the wrong layer. > > (Not to mention ServiceWorker which no doubt complicates this considerably.)
https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:436: return false; On 2015/06/19 15:35:14, David Benjamin wrote: > This check can't be relied on enforce anything meaningful. What if it's an FTP > URL or something more exotic? What if it's something exotic that internally or > via extensions redirects back to HTTP? What if it's HTTP and an extension > redirects it to something exotic? > > Revalidation is a property of an individual redirect leg, not the entire > URLRequest. While it's true that this check doesn't do anything much, since the LOAD_SUPPORT_ASYNC_REVALIDATION flag only does anything for HTTP and HTTPS, I feel it's better to be conservative and not claim to support async revalidation for other protocols. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:865: if (request->response_info().async_revalidation_required) On 2015/06/19 15:35:14, David Benjamin wrote: > Will ServiceWorker propagate this flag up? That would result in two async > revalidations happening. No. ServiceWorker knows nothing about the flag. The current policy when ServiceWorker controls a page is to leave caching policy to ServiceWorker, although we might adopt a more refined policy in future. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:866: BeginAsyncRevalidation(request); On 2015/06/19 15:35:14, David Benjamin wrote: > This is only called on the final redirect leg. What if an intermediate request > required async revalidation? Yes. This is not good. However, until there actually exist some resources using redirects and stale-while-revalidate together, I think it will pass. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2465: net::URLRequest* for_request) { On 2015/06/19 15:35:14, David Benjamin wrote: > By using the RDH, requests are limited in lifetime to the RenderFrameHost, > however the renderer does not know about the request. This is wrong. Logic > should not be sensitive to whether a navigation switch processes or not; > document switches may happen in-process without changing RFH which won't cancel > these requests while cross-process switches will cancel the requests. Whether > process swaps happen is not a tight policy and will likely change as Site > Isolation progresses. > > One possible set of semantics that would be sound is that they're scoped to the > *document*. But that requires that the renderer know about and own the request. > However, I don't think that's the ideal set of semantics anyway. It's not > well-defined for navigation requests. > > And, fundamentally, these requests are an internal implementation detail of the > low-level HTTP caching logic, so it would not be reasonable for their lifetime > to be bound by a web-platform construct. The requests are not limited in lifetime to the RenderFrameHost. See the change to CancelRequestsForRoute above. The requests are limited to the lifetime of the ResourceContext by necessity. See above. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2473: ResourceHostMsg_Request new_request_data = *original_request_data; On 2015/06/19 15:35:14, David Benjamin wrote: > BUG: These cannot retain the old resource type. Navigation-related resource > requests have many special-cases due to navigation's layering violations (see > PlzNavigate project, CrossSiteResourceHandler). I concluded that I can't change the ResourceType for similar reasons: there's too much code that depends on it. What I can do is limit the ResourceTypes which we try to support to those known to be safe. https://codereview.chromium.org/1041993004/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2483: int route_id = info->GetRouteID(); On 2015/06/19 15:35:14, David Benjamin wrote: > This will cause UI to show up all throughout the stack without ResourceThrottles > and the like. We explicitly do not want an association with the WebContents. > Everything will need to special-case async revalidations which is a red flag for > the design being at the wrong layer. > > (Not to mention ServiceWorker which no doubt complicates this considerably.) On the other hand, the ChildID and RouteID have to be same in order for the ResourceScheduler to be able to schedule the async revalidations after other resources on the same page. The lack of coordination with the ResourceScheduler, and resulting competition for bandwidth, was a major performance issue in the old implementation.
davidben@, PTAL.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Did a first pass over it. Sorry about the delay. Though see the thread I started on net-dev for proposed change to the directive. It's mostly orthogonal, but I think lets us simplify some of these. (Notably, the load flag goes away completely.) https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:112: request_->CancelAuth(); This needs to cancel the request; CancelAuth is to continue the request with no credentials which is not the same thing. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:119: CancelRequestInternal(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED); The default implementation already cancels. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:127: CancelRequestInternal(net::MapCertStatusToNetError(ssl_info.cert_status)); The default implementation already cancels. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:235: return; This isn't possible, unless you implement Cancel, right? https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/async_revalidation_driver.h (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:25: } // namespace net If there's just a few forward decls, I think we prefer without the comment and newline, so namespace net { class HttpCache; } https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:51: // forms the first part of the key to make that operation efficient. It would be better to make this key into a per-URLRequestContext object. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:55: // always have a distinct HttpCache. A ResourceContext doesn't have an HttpCache. There are multiple URLRequestContexts per ResourceContext. Both because there is both the normal and the media URLRequestContext and because a BrowserContext has multiple StoragePartitions. (I want to make there be one ResourceContext per StoragePartition, but that's not how it works today.) https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:63: // (accessed on 2014-07-17), so we have to overload operator() instead. This comment is unnecessary. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:94: // net::URLRequest* request() { return request_.get(); } ? https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:149: // make it stop. OnResponseStarted should happen. That's to signal that Start() completed, success or no. You check the URLRequest's status. Not sure about OnReadCompleted, but that deserves investigation. (That we signal back to you again when you cancel a URLRequest is kinda odd, but how the API works... I wish it didn't, but that would require we fix all code which externally cancels a URLRequest outside of is owner.) https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:874: request->SetLoadFlags(new_load_flags); This isn't sufficient. The first redirect leg already was sent with LOAD_SUPPORT_ASYNC_REVALIDATION, which means we've used a stale request while systematically not doing an async revalidation for it. (See also proposal on net-dev.) One option is to do the BeginAsyncRevalidation check BOTH here and in in DidReceiveResponse. Also in BeginAsyncRevalidation, you should DCHECK that the redirect chain has length 1 because that's the only case you're capable of handling right now. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1663: NULL); // original_request nullptr https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2107: NULL); // original_message nullptr https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2107: NULL); // original_message original_request https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2495: // https://groups.google.com/a/chromium.org/d/msg/net-dev/lHh764ZFdr0/phJIWXokt7cJ I must have missed the context when you made the original thread posting because this is not accurate. While you can assume that info's ResourceContext is valid, info->filter() MAY still be NULL. There is no guarantee that, at this point, the child is still around. (Again, I want to fix the cases where the request may outlive its child, which is part of why the original version of this CL was not okay, but right now there are cases where the request will outlive the child.) https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2515: headers.AddHeadersFromString(new_request_data.headers); If the request did not change, it really needs to be conditionalized. I.e. if we're replaying a response like: Cache-Control: max-age=5000, stale-while-revalidate=10000 ETag: "123456" The revalidation should only ever request: If-None-Match: "123456". That means either we do it within HttpCache which already makes such requests or, if making a new URLRequest (I've come around to this interpretation, pending the change from net-dev), we externally conditionalize it. (The HTTP cache should be able to handle externally conditionalized requests, but you should test this.) This is important as, in the common case, we do not expect the request to have changed. This implementation requires we read the cached body twice. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2518: params.child_id = child_id; This request cannot be associated with this process. It may outlive it. (In some cases, the child may not even be around anymore. Those also should get fixed, but don't add new cases.) https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2526: new_request_data.first_party_for_cookies)) { We should avoid duplicating this code. Ideally by not making it an issue to begin with. Matt and I had been talking about, for redirects, having APIs within //net to return a new URLRequest with the right set of parameters for starting at a particular redirect leg. (We were thinking parameters for replaying the redirect at this point.) It was intended as a "for when we want to bother with redirects" thing, but given this logic, it's probably worth doing this now. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2536: child_id, route_id, false, new_request.get()); Associating the request with child_id / route_id doesn't make sense. It outlives the document, but we may navigate in-process or cross-process, which means quirks of process allocation control scheduling. This also doesn't make much sense when combined with AsyncRevalidationKey (maybe the async revalidation would have happened sooner if we used the other route.) It also means that we call Start() on the request as soon as you close the tab which seems especially bizarre. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2552: CHECK(it != in_progress_async_revalidations_.end()); I don't think we do CHECK vs DCHECK this way. Otherwise we'd never DCHECK it != foo.end() checks ever. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2553: delete it->second, it->second = nullptr; Style: Don't use the comma operator. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2568: delete current_it->second, current_it->second = nullptr; Style: Don't use the comma operator. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_request_info_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.cc:71: NULL); // original_request nullptr https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.cc:165: // for an asynchronous revalidation. This class cannot depend on ResourceDispatcherHostImpl's behavior. This is a caller obligation and should be documented as such in the public API. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:44: // TODO(ricea): Something about this constructor. ? https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:230: // Original request information required to clone the request if an async See comment in the RDH re how to avoid this. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:231: // revalidation will be performed. NULL if async revalidation is not nullptr https://codereview.chromium.org/1041993004/diff/610001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/1041993004/diff/610001/content/public/browser... content/public/browser/resource_throttle.h:9: class GURL; This appears to be fixed.
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1296: QualifiesForAsyncRevalidation(request_data)); We shouldn't do async revalidations when "predictive network actions" is false. That's a per-profile setting managed that lives chrome-side (See chrome/browser/net/prediction_options.h)
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:446: net::LOAD_ONLY_FROM_CACHE)) { LOAD_IGNORE_LIMITS is also kinda weird. You either need to remove it for the revalidation request, use HIGHEST priority for the revalidation (Which is weird), or just not allow async revalidation when it's present. I think it may make sense to have a LOAD_FLAGS whitelist, instead of a blacklist. Also don't think we should preserve the LOAD_MAIN_FRAME flag. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:900: BeginAsyncRevalidation(request); We shouldn't call this on failures, right? Particularly cancellations - maybe the page was navigated away from, or maybe safebrowsing or group policy blocked the request or something. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2500: return; Can this happen while shutting down? Also, why isn't this first? https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2542: base::Unretained(this), async_revalidation_key)); Not creating a ResourceRequestInfo for the request will have some side effects, like ChromeNetworkDelegate::OnCanSetCookie behaving differently (At least not posting a task), different behavior when it comes to flywheel/freighter magic, skipping the blacklist logic, and who knows what else. Should dig through ChromeNetworkDelegate and figure what it breaks. I'm also concerned about possible future breakages.
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:900: BeginAsyncRevalidation(request); On 2015/10/09 16:02:22, mmenke wrote: > We shouldn't call this on failures, right? Particularly cancellations - maybe > the page was navigated away from, or maybe safebrowsing or group policy blocked > the request or something. David pointed out to me that we don't reach this point on requests that failed before headers were received, so this is a non-issue. Still may want to not do revalidation on requests that were cancelled before they completed, but that's a minor consideration.
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:112: request_->CancelAuth(); On 2015/10/08 21:57:51, David Benjamin wrote: > This needs to cancel the request; CancelAuth is to continue the request with no > credentials which is not the same thing. Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:119: CancelRequestInternal(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED); On 2015/10/08 21:57:51, David Benjamin wrote: > The default implementation already cancels. Removed. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:127: CancelRequestInternal(net::MapCertStatusToNetError(ssl_info.cert_status)); On 2015/10/08 21:57:51, David Benjamin wrote: > The default implementation already cancels. Removed. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:235: return; On 2015/10/08 21:57:51, David Benjamin wrote: > This isn't possible, unless you implement Cancel, right? Thanks. Removed. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/async_revalidation_driver.h (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:25: } // namespace net On 2015/10/08 21:57:51, David Benjamin wrote: > If there's just a few forward decls, I think we prefer without the comment and > newline, so > > namespace net { > class HttpCache; > } Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:51: // forms the first part of the key to make that operation efficient. On 2015/10/08 21:57:51, David Benjamin wrote: > It would be better to make this key into a per-URLRequestContext object. I don't understand this comment. Each request must have a distinct key, so how can it be a per-URLRequestContext object? https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:55: // always have a distinct HttpCache. On 2015/10/08 21:57:51, David Benjamin wrote: > A ResourceContext doesn't have an HttpCache. There are multiple > URLRequestContexts per ResourceContext. Both because there is both the normal > and the media URLRequestContext and because a BrowserContext has multiple > StoragePartitions. (I want to make there be one ResourceContext per > StoragePartition, but that's not how it works today.) This comment was confusing, sorry. I have verified that the HttpCache objects are owned by the ResourceContexts and not shared between them. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:63: // (accessed on 2014-07-17), so we have to overload operator() instead. On 2015/10/08 21:57:51, David Benjamin wrote: > This comment is unnecessary. Removed. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:94: // net::URLRequest* request() { return request_.get(); } On 2015/10/08 21:57:51, David Benjamin wrote: > ? Good question. Removed. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:446: net::LOAD_ONLY_FROM_CACHE)) { On 2015/10/09 16:02:22, mmenke wrote: > LOAD_IGNORE_LIMITS is also kinda weird. You either need to remove it for the > revalidation request, use HIGHEST priority for the revalidation (Which is > weird), or just not allow async revalidation when it's present. > > I think it may make sense to have a LOAD_FLAGS whitelist, instead of a > blacklist. Also don't think we should preserve the LOAD_MAIN_FRAME flag. LOAD_IGNORE_LIMITS is never set by Blink, but I added anyway in case Blink starts setting it in future. Sync XHR is already prevented from using s-w-r. The nice thing about the blacklist is that it's easy to see why each item is there. Whereas a whitelist would be a random grab-bag of whatever flags are left over. I added code to remove the LOAD_MAIN_FRAME flag from async revalidations. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:874: request->SetLoadFlags(new_load_flags); On 2015/10/08 21:57:51, David Benjamin wrote: > This isn't sufficient. The first redirect leg already was sent with > LOAD_SUPPORT_ASYNC_REVALIDATION, which means we've used a stale request while > systematically not doing an async revalidation for it. (See also proposal on > net-dev.) > > One option is to do the BeginAsyncRevalidation check BOTH here and in in > DidReceiveResponse. Also in BeginAsyncRevalidation, you should DCHECK that the > redirect chain has length 1 because that's the only case you're capable of > handling right now. I think will just modify net::HttpCache::Transaction to avoid returning a stale response with a 300 status code for the purposes of the experiment. I need to fix it properly before shipping anyway (assuming the experiment succeeds). https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1296: QualifiesForAsyncRevalidation(request_data)); On 2015/10/08 at 21:58:26, mmenke wrote: > We shouldn't do async revalidations when "predictive network actions" is false. That's a per-profile setting managed that lives chrome-side (See chrome/browser/net/prediction_options.h) I don't agree that stale-while-revalidate is a predictive feature. We are not guessing that a resource will be used. A resource has been used, and we are revalidating it. The Prefetch settings toggle exists to preserve privacy and conserve bandwidth. However, forcing revalidations to be synchronous neither improves privacy nor reduces bandwidth usage. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1663: NULL); // original_request On 2015/10/08 21:57:52, David Benjamin wrote: > nullptr Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2107: NULL); // original_message On 2015/10/08 21:57:51, David Benjamin wrote: > original_request Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2107: NULL); // original_message On 2015/10/08 21:57:51, David Benjamin wrote: > original_request Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2495: // https://groups.google.com/a/chromium.org/d/msg/net-dev/lHh764ZFdr0/phJIWXokt7cJ On 2015/10/08 at 21:57:51, David Benjamin wrote: > I must have missed the context when you made the original thread posting because this is not accurate. While you can assume that info's ResourceContext is valid, info->filter() MAY still be NULL. There is no guarantee that, at this point, the child is still around. (Again, I want to fix the cases where the request may outlive its child, which is part of why the original version of this CL was not okay, but right now there are cases where the request will outlive the child.) If the child is already gone at this point, I think it's reasonable to just pretend the request never happened. What happens if the request is transferred to a different renderer? https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2500: return; On 2015/10/09 at 16:02:22, mmenke wrote: > Can this happen while shutting down? Also, why isn't this first? 1. Based on other uses in this file: probably. 2. Based on other uses in this file, it seems quite popular to do a bunch of other processing before checking is_shutdown_, but I can't see a good reason for it. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2515: headers.AddHeadersFromString(new_request_data.headers); On 2015/10/08 at 21:57:51, David Benjamin wrote: > If the request did not change, it really needs to be conditionalized. I.e. if we're replaying a response like: > > Cache-Control: max-age=5000, stale-while-revalidate=10000 > ETag: "123456" > > The revalidation should only ever request: > > If-None-Match: "123456". > > That means either we do it within HttpCache which already makes such requests or, if making a new URLRequest (I've come around to this interpretation, pending the change from net-dev), we externally conditionalize it. (The HTTP cache should be able to handle externally conditionalized requests, but you should test this.) > > This is important as, in the common case, we do not expect the request to have changed. This implementation requires we read the cached body twice. The cache will conditionalize the request if If-Modified-Since or ETag headers are present in the cached response. See https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach... It's better to let the cache do it, because it is fussy about externally conditionalized requests and won't store the response if the request doesn't exactly match its expectations. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2518: params.child_id = child_id; On 2015/10/08 21:57:52, David Benjamin wrote: > This request cannot be associated with this process. It may outlive it. (In some > cases, the child may not even be around anymore. Those also should get fixed, > but don't add new cases.) This is only used by BuildLoadFlagsForRequest to decide whether to set LOAD_REPORT_RAW_HEADERS. Once devtools learn how to display async revalidations this will be good to have, so I am tempted to leave it in. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2526: new_request_data.first_party_for_cookies)) { On 2015/10/08 21:57:52, David Benjamin wrote: > We should avoid duplicating this code. Ideally by not making it an issue to > begin with. Matt and I had been talking about, for redirects, having APIs within > //net to return a new URLRequest with the right set of parameters for starting > at a particular redirect leg. (We were thinking parameters for replaying the > redirect at this point.) It was intended as a "for when we want to bother with > redirects" thing, but given this logic, it's probably worth doing this now. I would rather do the experiment first before worrying too much about the redirect case. If the experiment fails to show benefit then any extra work we do now is wasted. If we do demonstrate benefit, then we need to fix redirects before shipping. Either way, this code duplication is extremely temporary. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2536: child_id, route_id, false, new_request.get()); On 2015/10/08 21:57:51, David Benjamin wrote: > Associating the request with child_id / route_id doesn't make sense. It outlives > the document, but we may navigate in-process or cross-process, which means > quirks of process allocation control scheduling. The async request has to be scheduled after the other resources for the same page. In my experiments, this is critical to the performance of the feature. Since the ResourceScheduler identifies a page (strictly speaking, a "Client") by the (child_id, route_id) pair, the async request has to share the (child_id, route_id) pair with the other resources on the page. I have a CL which modifies the ResourceScheduler to schedule async revalidations after everything else: https://codereview.chromium.org/1209013003/ I abandoned it because it offered no significant benefit over the normal FIFO behaviour in my experiments, but I didn't test what happens on navigations, so it may be worth revisiting. > This also doesn't make much sense when combined with AsyncRevalidationKey (maybe > the async revalidation would have happened sooner if we used the other route.) AsyncRevalidationKey is used to de-dupe async revalidations, and also to cancel them when the ResourceContext goes away. It has nothing to do with scheduling. > It also means that we call Start() on the request as soon as you close the tab > which seems especially bizarre. Yes. That is the behaviour of the ResourceScheduler, and I agree that it is bizarre and should be changed, but I think it is outside the scope of this CL. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2542: base::Unretained(this), async_revalidation_key)); On 2015/10/09 16:02:22, mmenke wrote: > Not creating a ResourceRequestInfo for the request will have some side effects, > like ChromeNetworkDelegate::OnCanSetCookie behaving differently (At least not > posting a task), different behavior when it comes to flywheel/freighter magic, > skipping the blacklist logic, and who knows what else. Should dig through > ChromeNetworkDelegate and figure what it breaks. I'm also concerned about > possible future breakages. Sorry, I thought we had agreed that the request should not have a ResourceRequestInfo to avoid NetworkDelegate hooks that perform UI operations. I will verify that flywheel works manually. Please don't do any heavy lifting on this until we have experimental validation that stale-while-revalidate delivers the benefits we expect. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2552: CHECK(it != in_progress_async_revalidations_.end()); On 2015/10/08 21:57:52, David Benjamin wrote: > I don't think we do CHECK vs DCHECK this way. Otherwise we'd never DCHECK it != > foo.end() checks ever. My understanding is that the security people would rather we never did DCHECK(it != foo.end()) ever. They hate off-the-end iterators. The cost of getting it wrong is very high. I'll change it if you insist. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2553: delete it->second, it->second = nullptr; On 2015/10/08 21:57:51, David Benjamin wrote: > Style: Don't use the comma operator. Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2568: delete current_it->second, current_it->second = nullptr; On 2015/10/08 21:57:52, David Benjamin wrote: > Style: Don't use the comma operator. Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_request_info_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.cc:71: NULL); // original_request On 2015/10/08 21:57:52, David Benjamin wrote: > nullptr Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.cc:165: // for an asynchronous revalidation. On 2015/10/08 21:57:52, David Benjamin wrote: > This class cannot depend on ResourceDispatcherHostImpl's behavior. This is a > caller obligation and should be documented as such in the public API. Done. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:44: // TODO(ricea): Something about this constructor. On 2015/10/08 21:57:52, David Benjamin wrote: > ? I feel that 26 arguments is excessive. TODO removed. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:230: // Original request information required to clone the request if an async On 2015/10/08 21:57:52, David Benjamin wrote: > See comment in the RDH re how to avoid this. I'm sorry, I can't find the comment that you are referring to. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:231: // revalidation will be performed. NULL if async revalidation is not On 2015/10/08 21:57:52, David Benjamin wrote: > nullptr Done. https://codereview.chromium.org/1041993004/diff/610001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/1041993004/diff/610001/content/public/browser... content/public/browser/resource_throttle.h:9: class GURL; On 2015/10/08 21:57:52, David Benjamin wrote: > This appears to be fixed. Thanks. Removed.
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1296: QualifiesForAsyncRevalidation(request_data)); On 2015/10/13 22:53:17, Adam Rice wrote: > On 2015/10/08 at 21:58:26, mmenke wrote: > > We shouldn't do async revalidations when "predictive network actions" is > false. That's a per-profile setting managed that lives chrome-side (See > chrome/browser/net/prediction_options.h) > > I don't agree that stale-while-revalidate is a predictive feature. We are not > guessing that a resource will be used. A resource has been used, and we are > revalidating it. > > The Prefetch settings toggle exists to preserve privacy and conserve bandwidth. > However, forcing revalidations to be synchronous neither improves privacy nor > reduces bandwidth usage. You are guessing that it will be used in the future, and using a user's bandwidth based on that assumption. If it won't be used, we're just wasting bandwidth on a resource that can, in fact, still be used. https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2542: base::Unretained(this), async_revalidation_key)); On 2015/10/13 22:53:17, Adam Rice wrote: > On 2015/10/09 16:02:22, mmenke wrote: > > Not creating a ResourceRequestInfo for the request will have some side > effects, > > like ChromeNetworkDelegate::OnCanSetCookie behaving differently (At least not > > posting a task), different behavior when it comes to flywheel/freighter magic, > > skipping the blacklist logic, and who knows what else. Should dig through > > ChromeNetworkDelegate and figure what it breaks. I'm also concerned about > > possible future breakages. > > Sorry, I thought we had agreed that the request should not have a > ResourceRequestInfo to avoid NetworkDelegate hooks that perform UI operations. > > I will verify that flywheel works manually. Please don't do any heavy lifting on > this until we have experimental validation that stale-while-revalidate delivers > the benefits we expect. I'm not saying we should have the object, but that the request will have different behavior in subtly different ways, even ignoring the UI stuff.
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1296: QualifiesForAsyncRevalidation(request_data)); On 2015/10/13 23:01:53, mmenke wrote: > On 2015/10/13 22:53:17, Adam Rice wrote: > > On 2015/10/08 at 21:58:26, mmenke wrote: > > > We shouldn't do async revalidations when "predictive network actions" is > > false. That's a per-profile setting managed that lives chrome-side (See > > chrome/browser/net/prediction_options.h) > > > > I don't agree that stale-while-revalidate is a predictive feature. We are not > > guessing that a resource will be used. A resource has been used, and we are > > revalidating it. > > > > The Prefetch settings toggle exists to preserve privacy and conserve > bandwidth. > > However, forcing revalidations to be synchronous neither improves privacy nor > > reduces bandwidth usage. > > You are guessing that it will be used in the future, and using a user's > bandwidth based on that assumption. If it won't be used, we're just wasting > bandwidth on a resource that can, in fact, still be used. Worth noting it also potentially drains battery by keeping the cell modem up longer than usual.
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1296: QualifiesForAsyncRevalidation(request_data)); On 2015/10/13 23:01:53, mmenke wrote: > On 2015/10/13 22:53:17, Adam Rice wrote: > > On 2015/10/08 at 21:58:26, mmenke wrote: > > > We shouldn't do async revalidations when "predictive network actions" is > > false. That's a per-profile setting managed that lives chrome-side (See > > chrome/browser/net/prediction_options.h) > > > > I don't agree that stale-while-revalidate is a predictive feature. We are not > > guessing that a resource will be used. A resource has been used, and we are > > revalidating it. > > > > The Prefetch settings toggle exists to preserve privacy and conserve > bandwidth. > > However, forcing revalidations to be synchronous neither improves privacy nor > > reduces bandwidth usage. > > You are guessing that it will be used in the future, and using a user's > bandwidth based on that assumption. If it won't be used, we're just wasting > bandwidth on a resource that can, in fact, still be used. The feature is not speculative. We are not making guesses about future behaviour, we are fulfilling a contract with the server that says we can use the resource stale while we revalidate it. We're not wasting bandwidth, because in absence of s-w-r we would have had to perform a synchronous revalidation. We are performing the same operations, but in a different order. There are cases in which we cannot perform the async revalidation, but that does not mean that the revalidation is optional. Although I do not consider it necessary for experimental purposes, before shipping I will have to implement the feature of preventing further stale use of the resource after an async revalidation fails. The only place where any heuristics factor into it at all is in scheduling the asynchronous request. Since the same scheduling algorithm is applied to almost all other requests, this cannot by itself be considered to place the request in the category of "predictive network actions". I think it's entirely possible that the ResourceScheduler will in some cases use more battery for a low-priority request that it would for the same resource requested at higher priority. However, that has no bearing on the semantics of stale-while-revalidate, it is simply a deficiency of our scheduling implementation.
On 2015/10/14 15:03:45, Adam Rice wrote: > https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... > content/browser/loader/resource_dispatcher_host_impl.cc:1296: > QualifiesForAsyncRevalidation(request_data)); > On 2015/10/13 23:01:53, mmenke wrote: > > On 2015/10/13 22:53:17, Adam Rice wrote: > > > On 2015/10/08 at 21:58:26, mmenke wrote: > > > > We shouldn't do async revalidations when "predictive network actions" is > > > false. That's a per-profile setting managed that lives chrome-side (See > > > chrome/browser/net/prediction_options.h) > > > > > > I don't agree that stale-while-revalidate is a predictive feature. We are > not > > > guessing that a resource will be used. A resource has been used, and we are > > > revalidating it. > > > > > > The Prefetch settings toggle exists to preserve privacy and conserve > > bandwidth. > > > However, forcing revalidations to be synchronous neither improves privacy > nor > > > reduces bandwidth usage. > > > > You are guessing that it will be used in the future, and using a user's > > bandwidth based on that assumption. If it won't be used, we're just wasting > > bandwidth on a resource that can, in fact, still be used. > > The feature is not speculative. We are not making guesses about future > behaviour, we are fulfilling a contract with the server that says we can > use the resource stale while we revalidate it. > > We're not wasting bandwidth, because in absence of s-w-r we would have > had to perform a synchronous revalidation. We are performing the same > operations, but in a different order. > > There are cases in which we cannot perform the async revalidation, but that > does not mean that the revalidation is optional. > > Although I do not consider it necessary for experimental purposes, before > shipping I will have to implement the feature of preventing further stale use > of the resource after an async revalidation fails. > > The only place where any heuristics factor into it at all is in scheduling the > asynchronous request. Since the same scheduling algorithm is applied > to almost all other requests, this cannot by itself be considered to place > the request in the category of "predictive network actions". > > I think it's entirely possible that the ResourceScheduler will in some cases use > more battery for a low-priority request that it would for the same resource > requested at higher priority. However, that has no bearing on the semantics > of stale-while-revalidate, it is simply a deficiency of our scheduling > implementation. I think this is the key thing to resolve and is, yeah, the crux behind the otherwise seemingly cosmetic proposal on net-dev. How were you even planning on implementing the logic to prevent stale use after a revalidation failure? The current proposal seems not well-suited for this. I'd actually initially interepreted s-w-r in the way you seem to be, which is why I much preferred signalling this out of the HttpCache, not touching URLRequest, and having URLRequest::Delegate vend out some scoped_ptr<AsyncRevalidation>. Then the HttpCache could assemble the AsyncRevalidation correctly, it wouldn't have data to read, and you could watch the destructor or the request to then invalidate the resource. If this is the interpretation, I think this is really the more viable implementation strategy. It was only after talking with Matt that I realized this doesn't actually make sense. And really isn't worth the complexity cost when an optional version does the exact same thing, but without all the muddled semantics of this one. (See net-dev@ thread.) It's really not clear to me whether s-w-r as spec'd today is even an optional operation. The spec says SHOULD. The spec certainly doesn't say anything about invalidating the resource if the revalidation failed, and I don't remember it coming up in our previous discussion. In talking with other people who advocated s-w-r over speculative-revalidate, others seem to claim unreliability is okay precisely because it's optional! But this clashes with what you're saying now. If we don't consider it an optional operation, thing a TON of things become needlessly complex, as in this CL (except this CL isn't complex enough). We have to think about how much network activity a page should be able to induce while it isn't running. (sendBeacon is already a hole here, but one that we still can and probably should plug.) Little details like the mistake around redirects, extensions messing things up, merging by URL alone being wrong (custom headers + Vary!), etc. The ResourceScheduler may also delay the revalidation indefinitely. Say we delay the request. At what point do we say the parrot has died (failed to revalidate), triggering a concrete action, vs. it just resting and pining for the fjords (waiting for the network to be idle)? These are all extra parameters, details, and complexity for what should be at face a simple operation. And getting any of these complexities wrong breaks the promise you're proposing that cashing in on an s-w-r carries. Thus my desire to change the spelling of the feature slightly to make these actually speculative. This gets rid of the promise, and we don't care anymore. It makes the URLRequest strategy make more sense, and has minimal affect on your CL. Just makes a lot of considerations simpler. I agree with Matt that, even as-is, this is inherently a predictive feature. The s-w-r spelling muddles the water, but it's still fundamentally the same thing. We are making a request that is of zero use to anyone unless we hit the resource again in a particular time window. (Within the revalidation's max-age or max-age + s-w-r timeout.) If the resource is not hit in that time window, whether we revalidated or not doesn't matter. No one cares what an HTTP cache stores, just how it behaves. (Devil's advocate: the difference is observable in that the server may be changing state based on that request happening. I think we can largely discount that one. It doesn't seem to be the use case we care about. If it were, you wouldn't be able to merge by URL either.) I don't think it's reasonable to say that, because we're doing the same operations in a different order, it's not speculative. Imagine if HTTP caching did not have max-age, only conditionalized requests. And now someone proposes to add max-age, but we want to go further and add an async revalidation to max-age. Relative to a world without max-age, it's the same operations, but in a different order! But I don't think that makes it not predictive. If you compare to the current state of the world and instead say we want to add an async revalidation on cashing in max-age, it's certainly not the same operations and would I think clearly be considered predictive. (In fact, what I'm describing is exactly my speculative-revalidate proposal, but there's a cutoff time before which we don't bother considering to speculatively revalidate. I'm just taking the s-w-r timeout and making it subtractive instead of additive.)
Matt and I chatted about the higher-level technical problems more after the
meeting (I'd wanted to get into those during the meeting, but it had to get cut
short), and also with rdsmith (CC'd) who is much more in tune with how
prioritization works and the deficiencies.
The general agreement seems to be:
- Despite having the word "scheduler" in the name, the ResourceScheduler is
really not fit for this purpose anyway. (As all the problems around what happens
while a child dies and association with a child to begin with demonstrate.)
Unfortunately, we really don't have a good story at all for background,
low-priority, predictive requests. The short-term proposal that seemed least
displeasing was that, rather than hack it into the ResourceScheduler which
doesn't make much sense anyway, it should go into some
per-{URLRequestContext,ResourceContext,global} AsyncRevalidationManager thing
and we'll just enforce some very simplistic policy like "only two async
revalidations in flight at a time" or such. This, of course, requires the
revalidation be best-effort as discussed.
- It is very important that we NOT read from the cache in the very common case
that a revalidation comes back with no change. This is a waste of disk activity
and such. I'd previously suggested externally conditionalizing the request, but
Matt pointed out we can just use was_cached and cancel the request when Start
completes if it comes back true.
- Grabbing a copy of the IPC parameters is rather unreasonable. It would be
better to grab the parameters out of the URLRequest at the time.
- Being outside the ResourceScheduler means, as came up when this whole issue
began, there is no reason to touch the RDH stack at all. You could potentially
even check the boolean on NetworkDelegate or something and avoid this
altogether.
On 16 October 2015 at 09:02, <davidben@chromium.org> wrote: > - Despite having the word "scheduler" in the name, the ResourceScheduler is > really not fit for this purpose anyway. (As all the problems around what > happens > while a child dies and association with a child to begin with demonstrate.) > Unfortunately, we really don't have a good story at all for background, > low-priority, predictive requests. The short-term proposal that seemed > least > displeasing was that, rather than hack it into the ResourceScheduler which > doesn't make much sense anyway, it should go into some > per-{URLRequestContext,ResourceContext,global} AsyncRevalidationManager > thing > and we'll just enforce some very simplistic policy like "only two async > revalidations in flight at a time" or such. This, of course, requires the > revalidation be best-effort as discussed. > I want to reiterate that, to the best of my knowledge, scheduling the async revalidation after resources needed to load the page is critical to getting significant benefit from this feature. > - It is very important that we NOT read from the cache in the very common > case > that a revalidation comes back with no change. This is a waste of disk > activity > and such. I'd previously suggested externally conditionalizing the > request, but > Matt pointed out we can just use was_cached and cancel the request when > Start > completes if it comes back true. > Good point. - Grabbing a copy of the IPC parameters is rather unreasonable. It would be > better to grab the parameters out of the URLRequest at the time. > I'm more optimistic about this than I used to be, but there are still tricky issues. In particular, lots of things like to modify net::URLRequest::extra_request_headers_. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/10/16 00:02:16, David Benjamin wrote: > - Grabbing a copy of the IPC parameters is rather unreasonable. It would be > better to grab the parameters out of the URLRequest at the time. I may have misinterpreted you. Did you mean copy the parameters out of URLRequest at creation time? Because that would work.
https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/610001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:235: return; On 2015/10/13 22:53:16, Adam Rice wrote: > On 2015/10/08 21:57:51, David Benjamin wrote: > > This isn't possible, unless you implement Cancel, right? > > Thanks. Removed. This turned out to be possible with the sequence of calls StartRequest() -> CancelRequest() -> Resume(), so I added it back.
davidben: Please review everything except async_revalidation_manager_unittest.cc. I want to take another look at that, because I think it would be better to split tests involving the RDH from those that don't.
davidben@chromium.org changed reviewers: + bnc@chromium.org
+bnc to do an initial pass here.
On 2015/11/06 18:18:50, davidben wrote: > +bnc to do an initial pass here. I'm very sorry, I am caught up with a Beta blocker bug right now. I'll get back to reviewing this as soon as I can.
bnc: Do you mind if I rebase? The base revision of this CL is now really old.
Adam: sorry for the delay. Very nice CL, I especially enjoyed thoughtfully written comments that explain lifetime issue cornercases. Please forgive me if I am asking something that has already been answered in earlier reviewer correspondence, on the net-dev@ thread, or in the design doc. You might want to rebase again, run git cl format, and run trybots at some point. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:7: #include "base/location.h" What do you need this include for? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:8: #include "base/logging.h" Please include base/macros.h for DCHECK. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:10: #include "base/single_thread_task_runner.h" What do you need this include for? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:16: #include "net/http/http_transaction_factory.h" What do you need this include for? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:19: #include "net/url_request/url_request.h" Please remove this include, because you already incuded it from the header file. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:24: using base::TimeTicks; Please remove both these forward declarations, because you have base/time/time.h included. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:83: Please add a comment along the lines of "The revalidated cache entry should not follow redirects, because caching is a property of an individual HTTP resource." (paraphrasing from davidben@'s e-mail on net-dev@). https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:114: has_already_failed_ = true; Consider moving these two lines (DCHECK and set |has_already_failed_|) to ResponseCompleted() from here and from every other occurrence before a call to ResponseCompleted(). In that case, |has_already_called_callback_| or something similar would be a better name. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:161: // URLRequest reported an EOF. Call ResponseCompleted. Consider removing "Call ResponseCompleted." as it does not add carry additional information to what is trivial from code. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:190: // to call ResponseCompleted(). Consider adding DCHECK(has_already_failed_) to document this. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver.h (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:75: bool is_deferred() const { return is_deferred_; } You DCHECK this twice accessor twice and DCHECK this member twice. Consider getting rid of the accessor and using the member every time. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:11: #include "base/location.h" What do you need this include for? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:16: #include "net/base/io_buffer.h" Please remove this include, because you have already included it in the header file. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:20: #include "net/url_request/url_request.h" Please remove this include, because you have already included it in the header file. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:42: // net::URLRequestTestJob: Add " implementation" right before colon. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:54: ADD_FAILURE() << "Certificate supplied"; Optional: consider adding a period to end this sentence. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:60: DISALLOW_COPY_AND_ASSIGN(MockClientCertURLRequestJob); This class does not have any members so it should be safe to copy or to assign. What are you trying to express here? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:81: // net::URLRequestTestJob: Add " implementation" right before colon. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:93: ADD_FAILURE() << "ContinueDespiteLastError called"; Optional: consider adding a full stop to end this sentence. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:99: DISALLOW_COPY_AND_ASSIGN(MockSSLErrorURLRequestJob); Why? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:162: // purposes. nullptr is not allowed. Caller does *not* take ownership of the Optional: consider writing |nullptr| to mitigate the ugh-feeling one might get when reading a non-capitalised first word of a sentence. (Just to be clear, I am definitely not asking you to capitalise nullptr.) https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:174: DISALLOW_COPY_AND_ASSIGN(ResourceThrottleStub); Copying and assigning this class seems to be safe to me, why bother disallowing? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:179: class URLRequestTestDelayedNetworkJob : public net::URLRequestTestJob { This class does not seem to be used anywhere. Please remove it, or better yet, add the test that you had in mind when crafting this class. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:191: net::URLRequestTestJob::Start(); Early returns seems a bit contorted to me. Consider inverting the if condition and save one line. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:199: DISALLOW_COPY_AND_ASSIGN(URLRequestTestDelayedNetworkJob); Why? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:231: scoped_ptr<ResourceThrottleStub> resource_throttle( Optional: throw away |resource_throttle| local variable and use make_scoped_ptr(raw_ptr_resource_throttle) on line 237. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:261: net::URLRequest* raw_ptr_to_request_; Please harmonise prefix: raw_ptr_ or raw_ptr_to_ for both. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:329: implicit_cast<ResourceController*>(driver_.get())->Resume(); Please do not do implicit_cast, because it is used very-very rarely in the codebase [1], so developers might not be familiar with it. [1] https://code.google.com/p/chromium/codesearch#search/&q=implicit_cast%20-file... https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:392: DISALLOW_COPY_AND_ASSIGN(FromCacheURLRequestJob); Why bother? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:15: #include "content/public/browser/resource_context.h" You only have pointers to ResourceContext, I don't believe you need this include. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:16: #include "content/public/browser/resource_throttle.h" I believe a forward declaration would be enough for a scoped_ptr<>, but I'm not sure. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:78: ResourceContext* resource_context = NULL; Please use nullptr in this line and the one below. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:80: // This |request_context| needs to be valid until Optional: add newline above this comment for better structure. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:83: // https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... Please don't use such URL, because (1) the awkwardness of the unusually long line outweights the convenience of having a clickable URL in your editor (and some environments cannot use URLs correctly if it is broken into multiple lines, like vim in tmux), (2) since there is no tagged revision, the line number will go stale very quickly, and (3) there is no guarantee on how long this hostname will work. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:106: for_request->url(), net::MINIMUM_PRIORITY, NULL); Please use nullptr instead of NULL. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:144: auto next_it = in_progress_.lower_bound(partial_key); Optional: use |it| instead of |next_it|, |prev_it| instead of |current_it|. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:145: while (next_it != in_progress_.end() && Optional: use for loop instead, make |next_it| local to loop by defining it in the init-statement. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:148: auto current_it = next_it++; Please don't do assignment and increment in the same expression, the code reads easier if the readers are not expected to remember whether the postfix syntax evaluates to the value before or after incrementing. (I certainly didn't remember.) https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:170: return true; Please add an empty line above this final return. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:49: // The key used to lookup an AsyncRevalidationDriver in the map of pending Optional: feel free to remove "used to lookup". It is okay to say that this is "the key of the map". https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:60: // additional async revalidations will not improve matters. Please reflow this paragraph. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:79: // forms the first part of the key. This last sentence might be more appropriate as a comment for LessThan, because the order of keys is really decided there. The order of members in the struct is irrelevant. Optional: this is an implementation detail, maybe no need to mention the order in which keys are used in the lexicographic order at all in the header file. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:82: // Each ResourceContext owns one or more HttpCache objects. Optional: write "There might be multiple HttpCache objects per ResourceContext." or something similar instead. It took me a very long time to understand why this comment was relevant at, and this alternate wording might have been helpful. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:94: // Map of AsyncRevalidationDriver object. Optional: remove this comment, fold into the one below. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:95: typedef std::map<AsyncRevalidationKey, Use "using" instead of "typedef", see https://chromium-cpp.appspot.com. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:101: // Async revalidations that are currently in-flight: either waiting to be Optional: "AsyncRevalidationDriver instances that are currently in-flight". https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:60: typedef base::Callback<net::URLRequestJob*(net::URLRequest*, Use "using" instead of "typedef", see https://chromium-cpp.appspot.com. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:69: net::URLRequestJob* MaybeCreateJobWithProtocolHandler( Optional: feel free to inline function definition if you prefer, it should be okay in a test. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:171: NULL, Use nullptr instead of NULL. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:276: void MakeTestRequest(int render_view_id, int request_id, const GURL& url); Optional: feel free to inline if you prefer, it should be okay in a test. Besides, constructor is longer and is inlined anyway. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:844: // Remove the LOAD_SUPPORT_ASYNC_REVALIDATION flag if it is present. Please explain briefly why you do this for redirects. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1313: // Check if request is eligible for async revalidation. Optional: this comment seems to be redundant with the nicely chosen local variable name, consider removing it.
On 2015/11/11 19:07:21, Adam Rice wrote: > bnc: Do you mind if I rebase? The base revision of this CL is now really old. Oops, I missed this question. Feel free to go ahead with rebasing. Thanks.
Thank you for the thorough review. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:7: #include "base/location.h" On 2015/11/17 13:12:21, Bence wrote: > What do you need this include for? For FROM_HERE. Added a comment. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:8: #include "base/logging.h" On 2015/11/17 13:12:21, Bence wrote: > Please include base/macros.h for DCHECK. Is is going to move there? It is currently in base/logging.h. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:10: #include "base/single_thread_task_runner.h" On 2015/11/17 13:12:21, Bence wrote: > What do you need this include for? base::ThreadTaskRunnerHandle::Get() returns a SingleThreadTaskRunner. I added a comment. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:16: #include "net/http/http_transaction_factory.h" On 2015/11/17 13:12:21, Bence wrote: > What do you need this include for? It looks like a mistake. I have removed it. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:19: #include "net/url_request/url_request.h" On 2015/11/17 13:12:21, Bence wrote: > Please remove this include, because you already incuded it from the header file. Thanks, done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:24: using base::TimeTicks; On 2015/11/17 13:12:21, Bence wrote: > Please remove both these forward declarations, because you have base/time/time.h > included. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:83: On 2015/11/17 13:12:21, Bence wrote: > Please add a comment along the lines of "The revalidated cache entry should not > follow redirects, because caching is a property of an individual HTTP resource." > (paraphrasing from davidben@'s e-mail on net-dev@). Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:114: has_already_failed_ = true; On 2015/11/17 13:12:21, Bence wrote: > Consider moving these two lines (DCHECK and set |has_already_failed_|) to > ResponseCompleted() from here and from every other occurrence before a call to > ResponseCompleted(). In that case, |has_already_called_callback_| or something > similar would be a better name. Done. I changed ResponseCompleted() to reset the callback, so we can avoid calling it twice without needing a separate flag. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:161: // URLRequest reported an EOF. Call ResponseCompleted. On 2015/11/17 13:12:21, Bence wrote: > Consider removing "Call ResponseCompleted." as it does not add carry additional > information to what is trivial from code. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:190: // to call ResponseCompleted(). On 2015/11/17 13:12:21, Bence wrote: > Consider adding DCHECK(has_already_failed_) to document this. In this case CancelRequestInternal calls ResponseCompleted() asynchronously via PostTask(), so it may not actually have run yet. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver.h (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:75: bool is_deferred() const { return is_deferred_; } On 2015/11/17 13:12:21, Bence wrote: > You DCHECK this twice accessor twice and DCHECK this member twice. Consider > getting rid of the accessor and using the member every time. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:11: #include "base/location.h" On 2015/11/17 13:12:22, Bence wrote: > What do you need this include for? For FROM_HERE. Comment added. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:20: #include "net/url_request/url_request.h" On 2015/11/17 13:12:22, Bence wrote: > Please remove this include, because you have already included it in the header > file. Thanks, done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:42: // net::URLRequestTestJob: On 2015/11/17 13:12:22, Bence wrote: > Add " implementation" right before colon. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:54: ADD_FAILURE() << "Certificate supplied"; On 2015/11/17 13:12:22, Bence wrote: > Optional: consider adding a period to end this sentence. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:60: DISALLOW_COPY_AND_ASSIGN(MockClientCertURLRequestJob); On 2015/11/17 13:12:22, Bence wrote: > This class does not have any members so it should be safe to copy or to assign. > What are you trying to express here? It's not actually copyable because of the base class. I am just being explicit about the fact that it's not copyable. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:81: // net::URLRequestTestJob: On 2015/11/17 13:12:22, Bence wrote: > Add " implementation" right before colon. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:93: ADD_FAILURE() << "ContinueDespiteLastError called"; On 2015/11/17 13:12:22, Bence wrote: > Optional: consider adding a full stop to end this sentence. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:99: DISALLOW_COPY_AND_ASSIGN(MockSSLErrorURLRequestJob); On 2015/11/17 13:12:22, Bence wrote: > Why? No particular reason, just making it explicit. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:162: // purposes. nullptr is not allowed. Caller does *not* take ownership of the On 2015/11/17 13:12:22, Bence wrote: > Optional: consider writing |nullptr| to mitigate the ugh-feeling one might get > when reading a non-capitalised first word of a sentence. (Just to be clear, I > am definitely not asking you to capitalise nullptr.) It appears for some reason I copied this comment verbatim from resource_throttle.h. Maybe I should just remove it completely? https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:174: DISALLOW_COPY_AND_ASSIGN(ResourceThrottleStub); On 2015/11/17 13:12:22, Bence wrote: > Copying and assigning this class seems to be safe to me, why bother disallowing? I don't want to provide a guarantee that copying is safe, because a change in the base class could make it unsafe. There would also be a risk of slicing if in future someone derived from this class. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:179: class URLRequestTestDelayedNetworkJob : public net::URLRequestTestJob { On 2015/11/17 13:12:22, Bence wrote: > This class does not seem to be used anywhere. Please remove it, or better yet, > add the test that you had in mind when crafting this class. Ah, sorry, I failed to remove this. The history is that originally AsyncRevalidationDriver supported various throttling scenarios that are not actually used by ResourceScheduler. I realised that was just pointless complexity so I removed the unused features, including deferral on WillStartUsingNetwork. But I failed to remove this test support class. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:191: net::URLRequestTestJob::Start(); On 2015/11/17 13:12:22, Bence wrote: > Early returns seems a bit contorted to me. Consider inverting the if condition > and save one line. Acknowledged. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:199: DISALLOW_COPY_AND_ASSIGN(URLRequestTestDelayedNetworkJob); On 2015/11/17 13:12:22, Bence wrote: > Why? I like to be explicit. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:231: scoped_ptr<ResourceThrottleStub> resource_throttle( On 2015/11/17 13:12:22, Bence wrote: > Optional: throw away |resource_throttle| local variable and use > make_scoped_ptr(raw_ptr_resource_throttle) on line 237. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:261: net::URLRequest* raw_ptr_to_request_; On 2015/11/17 13:12:22, Bence wrote: > Please harmonise prefix: raw_ptr_ or raw_ptr_to_ for both. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:329: implicit_cast<ResourceController*>(driver_.get())->Resume(); On 2015/11/17 13:12:22, Bence wrote: > Please do not do implicit_cast, because it is used very-very rarely in the > codebase [1], so developers might not be familiar with it. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=implicit_cast%20-file... Also, since I wrote this I accidentally got implicit_cast removed from //base. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:392: DISALLOW_COPY_AND_ASSIGN(FromCacheURLRequestJob); On 2015/11/17 13:12:22, Bence wrote: > Why bother? It seems like the right thing to do. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:15: #include "content/public/browser/resource_context.h" On 2015/11/17 13:12:22, Bence wrote: > You only have pointers to ResourceContext, I don't believe you need this > include. Good point. Removed. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:16: #include "content/public/browser/resource_throttle.h" On 2015/11/17 13:12:22, Bence wrote: > I believe a forward declaration would be enough for a scoped_ptr<>, but I'm not > sure. A scoped_ptr<ResourceThrottle> object is instantiated in AsyncRevalidationManager::BeginAsyncRevalidation(). The scoped_ptr destructor references the ResourceThrottle destructor, which requires the declaration of the destructor to be visible. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:78: ResourceContext* resource_context = NULL; On 2015/11/17 13:12:23, Bence wrote: > Please use nullptr in this line and the one below. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:80: // This |request_context| needs to be valid until On 2015/11/17 13:12:23, Bence wrote: > Optional: add newline above this comment for better structure. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:83: // https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... On 2015/11/17 13:12:22, Bence wrote: > Please don't use such URL, because (1) the awkwardness of the unusually long > line outweights the convenience of having a clickable URL in your editor (and > some environments cannot use URLs correctly if it is broken into multiple lines, > like vim in tmux), (2) since there is no tagged revision, the line number will > go stale very quickly, and (3) there is no guarantee on how long this hostname > will work. Okay, replaced with a simple reference to the file. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:106: for_request->url(), net::MINIMUM_PRIORITY, NULL); On 2015/11/17 13:12:22, Bence wrote: > Please use nullptr instead of NULL. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:144: auto next_it = in_progress_.lower_bound(partial_key); On 2015/11/17 13:12:22, Bence wrote: > Optional: use |it| instead of |next_it|, |prev_it| instead of |current_it|. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:145: while (next_it != in_progress_.end() && On 2015/11/17 13:12:22, Bence wrote: > Optional: use for loop instead, make |next_it| local to loop by defining it in > the init-statement. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:148: auto current_it = next_it++; On 2015/11/17 13:12:22, Bence wrote: > Please don't do assignment and increment in the same expression, the code reads > easier if the readers are not expected to remember whether the postfix syntax > evaluates to the value before or after incrementing. (I certainly didn't > remember.) Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:170: return true; On 2015/11/17 13:12:23, Bence wrote: > Please add an empty line above this final return. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:49: // The key used to lookup an AsyncRevalidationDriver in the map of pending On 2015/11/17 13:12:23, Bence wrote: > Optional: feel free to remove "used to lookup". It is okay to say that this is > "the key of the map". Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:60: // additional async revalidations will not improve matters. On 2015/11/17 13:12:23, Bence wrote: > Please reflow this paragraph. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:79: // forms the first part of the key. On 2015/11/17 13:12:23, Bence wrote: > This last sentence might be more appropriate as a comment for LessThan, because > the order of keys is really decided there. The order of members in the struct > is irrelevant. > > Optional: this is an implementation detail, maybe no need to mention the order > in which keys are used in the lexicographic order at all in the header file. I think you're right. I have removed the implementation detail from this comment. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:82: // Each ResourceContext owns one or more HttpCache objects. On 2015/11/17 13:12:23, Bence wrote: > Optional: write "There might be multiple HttpCache objects per ResourceContext." > or something similar instead. It took me a very long time to understand why > this comment was relevant at, and this alternate wording might have been > helpful. Sorry about that. I changed it to "There are multiple independent HttpCache objects per ResourceContext." since at the moment there are always at least two. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:94: // Map of AsyncRevalidationDriver object. On 2015/11/17 13:12:23, Bence wrote: > Optional: remove this comment, fold into the one below. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:95: typedef std::map<AsyncRevalidationKey, On 2015/11/17 13:12:23, Bence wrote: > Use "using" instead of "typedef", see https://chromium-cpp.appspot.com. Thanks. Old habits die hard. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:101: // Async revalidations that are currently in-flight: either waiting to be On 2015/11/17 13:12:23, Bence wrote: > Optional: "AsyncRevalidationDriver instances that are currently in-flight". Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:60: typedef base::Callback<net::URLRequestJob*(net::URLRequest*, On 2015/11/17 13:12:23, Bence wrote: > Use "using" instead of "typedef", see https://chromium-cpp.appspot.com. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:69: net::URLRequestJob* MaybeCreateJobWithProtocolHandler( On 2015/11/17 13:12:23, Bence wrote: > Optional: feel free to inline function definition if you prefer, it should be > okay in a test. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:171: NULL, On 2015/11/17 13:12:23, Bence wrote: > Use nullptr instead of NULL. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:276: void MakeTestRequest(int render_view_id, int request_id, const GURL& url); On 2015/11/17 13:12:23, Bence wrote: > Optional: feel free to inline if you prefer, it should be okay in a test. > Besides, constructor is longer and is inlined anyway. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:844: // Remove the LOAD_SUPPORT_ASYNC_REVALIDATION flag if it is present. On 2015/11/17 13:12:23, Bence wrote: > Please explain briefly why you do this for redirects. Done. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1313: // Check if request is eligible for async revalidation. On 2015/11/17 13:12:23, Bence wrote: > Optional: this comment seems to be redundant with the nicely chosen local > variable name, consider removing it. Done.
Excellent. This CL looks swell to me, but I am not competent in design questions, so I defer to the other reviewers. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:8: #include "base/logging.h" On 2015/11/17 17:45:51, Adam Rice wrote: > On 2015/11/17 13:12:21, Bence wrote: > > Please include base/macros.h for DCHECK. > > Is is going to move there? It is currently in base/logging.h. You are right. Sorry. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:60: DISALLOW_COPY_AND_ASSIGN(MockClientCertURLRequestJob); On 2015/11/17 17:45:52, Adam Rice wrote: > On 2015/11/17 13:12:22, Bence wrote: > > This class does not have any members so it should be safe to copy or to > assign. > > What are you trying to express here? > > It's not actually copyable because of the base class. I am just being explicit > about the fact that it's not copyable. Oh okay, that makes sense. Thank you for clarifying. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:99: DISALLOW_COPY_AND_ASSIGN(MockSSLErrorURLRequestJob); On 2015/11/17 17:45:52, Adam Rice wrote: > On 2015/11/17 13:12:22, Bence wrote: > > Why? > > No particular reason, just making it explicit. Sounds good. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:162: // purposes. nullptr is not allowed. Caller does *not* take ownership of the On 2015/11/17 17:45:52, Adam Rice wrote: > On 2015/11/17 13:12:22, Bence wrote: > > Optional: consider writing |nullptr| to mitigate the ugh-feeling one might get > > when reading a non-capitalised first word of a sentence. (Just to be clear, I > > am definitely not asking you to capitalise nullptr.) > > It appears for some reason I copied this comment verbatim from > resource_throttle.h. > Maybe I should just remove it completely? That would also be fine with me https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:174: DISALLOW_COPY_AND_ASSIGN(ResourceThrottleStub); On 2015/11/17 17:45:51, Adam Rice wrote: > On 2015/11/17 13:12:22, Bence wrote: > > Copying and assigning this class seems to be safe to me, why bother > disallowing? > > I don't want to provide a guarantee that copying is safe, because a change in > the > base class could make it unsafe. There would also be a risk of slicing if in > future > someone derived from this class. Fair enough. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:392: DISALLOW_COPY_AND_ASSIGN(FromCacheURLRequestJob); On 2015/11/17 17:45:52, Adam Rice wrote: > On 2015/11/17 13:12:22, Bence wrote: > > Why bother? > > It seems like the right thing to do. Okay then. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:16: #include "content/public/browser/resource_throttle.h" On 2015/11/17 17:45:52, Adam Rice wrote: > On 2015/11/17 13:12:22, Bence wrote: > > I believe a forward declaration would be enough for a scoped_ptr<>, but I'm > not > > sure. > > A scoped_ptr<ResourceThrottle> object is instantiated in > AsyncRevalidationManager::BeginAsyncRevalidation(). The scoped_ptr destructor > references the ResourceThrottle destructor, which requires the declaration of > the destructor to be visible. Of course, what was I thinking? Thanks for clarifying. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:83: // https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... On 2015/11/17 17:45:52, Adam Rice wrote: > On 2015/11/17 13:12:22, Bence wrote: > > Please don't use such URL, because (1) the awkwardness of the unusually long > > line outweights the convenience of having a clickable URL in your editor (and > > some environments cannot use URLs correctly if it is broken into multiple > lines, > > like vim in tmux), (2) since there is no tagged revision, the line number will > > go stale very quickly, and (3) there is no guarantee on how long this hostname > > will work. > > Okay, replaced with a simple reference to the file. Neat. https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/690001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:82: // Each ResourceContext owns one or more HttpCache objects. On 2015/11/17 17:45:53, Adam Rice wrote: > On 2015/11/17 13:12:23, Bence wrote: > > Optional: write "There might be multiple HttpCache objects per > ResourceContext." > > or something similar instead. It took me a very long time to understand why > > this comment was relevant at, and this alternate wording might have been > > helpful. > > Sorry about that. I changed it to "There are multiple independent HttpCache > objects per ResourceContext." since at the moment there are always at least two. Splendid.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
Haven't looked all files yet, but let me send early comments https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:7: #include "base/location.h" // For FROM_HERE nit: I see you added these comments per review feedback, while I feel this is not very common and could be probably omitted. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:11: #include "base/thread_task_runner_handle.h" // from ThreadTaskRunnerHandle. These seem obvious from header names. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:41: throttle_(throttle.Pass()), nit: you could now use std::move for these https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:91: request_->CancelWithError(net::ERR_ACCESS_DENIED); Help me understand: for this case we don't need/want to call CancelRequest() ? https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:100: DCHECK(!defer); !*defer ? https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:151: DCHECK_EQ(0, bytes_read); nit: feels a bit verbose to me https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:203: void AsyncRevalidationDriver::CompleteResponseStarted() { Where is this used? https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:48: return lhs.url_key < rhs.url_key; nit: I think now you could do return std::tie(lhs.xxx, lhs.yyy, lhs.zzz) < std::tie(rhs.xxx, rhs.yyy, rhs.zzz) https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:28: // process. It also implements the creation of a new net::URLRequest and nit: process -> renderer process? https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:30: // stale-while-revalidate algorithm applied to it. It'd be probably easier to understand its lifetime & ownership semantics if it's also noted that it's owned by RDH https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:93: AsyncRevalidationKey::LessThan>; I think we could use ScopedPtrMap here https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2110: // Do nothing if it is enabled already. nit: not very informative, maybe just remove this comment
https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:28: // process. It also implements the creation of a new net::URLRequest and On 2015/11/19 07:00:29, kinuko wrote: > nit: process -> renderer process? oops, sorry please ignore this
Haven't taken a closer look in the tests but this is looking good to me. Most of the complexity's in the manager, and from what we discussed it's agreed that we have minimal hooks in RDH to let the manager handle requests via resource scheduler... https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:25: const int kReadTimeoutSeconds = 5 * 60; How is this timeout value chosen? https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:132: async_revalidation = new AsyncRevalidationDriver( Hmm it's slightly hard to follow that this is actually inserting the value here... maybe move the lines 98-99 before the lines?
https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:7: #include "base/location.h" // For FROM_HERE On 2015/11/19 07:00:28, kinuko wrote: > nit: I see you added these comments per review feedback, while I feel this is > not very common and could be probably omitted. Done. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:11: #include "base/thread_task_runner_handle.h" // from ThreadTaskRunnerHandle. On 2015/11/19 07:00:28, kinuko wrote: > These seem obvious from header names. What's not obvious is that the SingleThreadTaskRunner include is required to call PostTask on the return value from ThreadTaskRunnerHandle::Get(). I don't know how to fix that without adding a lot of verbosity. For now, I have removed these comments since they don't seem to help. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:25: const int kReadTimeoutSeconds = 5 * 60; On 2015/11/19 08:38:34, kinuko wrote: > How is this timeout value chosen? I don't remember. The most likely place is kDefaultDetachableCancelDelayMs, but that would mean I was off by a factor of 10. Also, kDefaultDetachableCancelDelayMs is the total time for the request after it is detached, whereas this is the time between reads, so it is not directly comparable. I have changed it to 30 seconds. Even on a really slow connection, if you receive nothing for 30 seconds it is probably time to give up. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:41: throttle_(throttle.Pass()), On 2015/11/19 07:00:28, kinuko wrote: > nit: you could now use std::move for these Done. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:91: request_->CancelWithError(net::ERR_ACCESS_DENIED); On 2015/11/19 07:00:28, kinuko wrote: > Help me understand: for this case we don't need/want to call CancelRequest() ? CancelRequest() would use ERR_ABORTED, which is very generic and makes life more difficult for people trying to track down the cause of fluctuations in the ERR_ABORTED rate. Also, CancelRequestInternal() does some checking whether the request is pending which is completely unneeded in this case (we know it's pending). Can you think of anything I could do to make this clearer to someone reading the code? https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:100: DCHECK(!defer); On 2015/11/19 07:00:29, kinuko wrote: > !*defer ? Oh. The tests in https://codereview.chromium.org/1308203003 would have caught this, but I haven't rebased them in 2 months. I probably should. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:151: DCHECK_EQ(0, bytes_read); On 2015/11/19 07:00:28, kinuko wrote: > nit: feels a bit verbose to me I have made it less verbose. PTAL. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:203: void AsyncRevalidationDriver::CompleteResponseStarted() { On 2015/11/19 07:00:28, kinuko wrote: > Where is this used? It's not. Thank you for checking. I moved the implementation of this into OnResponseStarted() but it seems I forgot to delete the method. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:48: return lhs.url_key < rhs.url_key; On 2015/11/19 07:00:29, kinuko wrote: > nit: I think now you could do > > return std::tie(lhs.xxx, lhs.yyy, lhs.zzz) < std::tie(rhs.xxx, rhs.yyy, rhs.zzz) Done. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:132: async_revalidation = new AsyncRevalidationDriver( On 2015/11/19 08:38:34, kinuko wrote: > Hmm it's slightly hard to follow that this is actually inserting the value > here... maybe move the lines 98-99 before the lines? Done. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:30: // stale-while-revalidate algorithm applied to it. On 2015/11/19 07:00:29, kinuko wrote: > It'd be probably easier to understand its lifetime & ownership semantics if it's > also noted that it's owned by RDH Comment updated. PTAL. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:93: AsyncRevalidationKey::LessThan>; On 2015/11/19 07:00:29, kinuko wrote: > I think we could use ScopedPtrMap here It's deprecated. But I can use scoped_ptr<AsyncRevalidationDriver> instead. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2110: // Do nothing if it is enabled already. On 2015/11/19 07:00:29, kinuko wrote: > nit: not very informative, maybe just remove this comment Done.
https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:7: #include "base/location.h" // For FROM_HERE On 2015/11/19 21:13:52, Adam Rice wrote: > On 2015/11/19 07:00:28, kinuko wrote: > > nit: I see you added these comments per review feedback, while I feel this is > > not very common and could be probably omitted. > > Done. I genuinely couldn't find what these headers were there for, so I meant to simply ask. I did not mean to imply that it should be clarified in a comment. I'm very sorry for my ambiguous feedback. https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:11: #include "base/thread_task_runner_handle.h" // from ThreadTaskRunnerHandle. On 2015/11/19 21:13:52, Adam Rice wrote: > On 2015/11/19 07:00:28, kinuko wrote: > > These seem obvious from header names. > > What's not obvious is that the SingleThreadTaskRunner include is required to > call PostTask on the return value from ThreadTaskRunnerHandle::Get(). I don't > know how to fix that without adding a lot of verbosity. > > For now, I have removed these comments since they don't seem to help. Ditto.
https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:7: #include "base/location.h" // For FROM_HERE On 2015/11/20 13:18:35, Bence wrote: > On 2015/11/19 21:13:52, Adam Rice wrote: > > On 2015/11/19 07:00:28, kinuko wrote: > > > nit: I see you added these comments per review feedback, while I feel this > is > > > not very common and could be probably omitted. > > > > Done. > > I genuinely couldn't find what these headers were there for, so I meant to > simply ask. I did not mean to imply that it should be clarified in a comment. > I'm very sorry for my ambiguous feedback. Absolutely no problem. I follow the philosophy that all feedback is an opportunity to improve my code. Sometimes this works out better than others.
On 2015/11/20 13:23:52, Adam Rice wrote: > https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... > File content/browser/loader/async_revalidation_driver.cc (right): > > https://codereview.chromium.org/1041993004/diff/750001/content/browser/loader... > content/browser/loader/async_revalidation_driver.cc:7: #include > "base/location.h" // For FROM_HERE > On 2015/11/20 13:18:35, Bence wrote: > > On 2015/11/19 21:13:52, Adam Rice wrote: > > > On 2015/11/19 07:00:28, kinuko wrote: > > > > nit: I see you added these comments per review feedback, while I feel this > > is > > > > not very common and could be probably omitted. > > > > > > Done. > > > > I genuinely couldn't find what these headers were there for, so I meant to > > simply ask. I did not mean to imply that it should be clarified in a comment. > > > I'm very sorry for my ambiguous feedback. > > Absolutely no problem. I follow the philosophy that all feedback is an > opportunity > to improve my code. Sometimes this works out better than others. No problem at all, feeling sorry if I made you feel sorry! Let me articulate that what you both did were very reasonable and understandable.
davidben, could you review the RDH changes?
Thanks! This is much better. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:12: #include "base/single_thread_task_runner.h" Unused? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:17: #include "net/cert/cert_status_flags.h" Unused? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:18: #include "net/ssl/ssl_info.h" Unused? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:19: #include "net/url_request/url_request_context.h" Unused? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:30: // cache entry will be truncated for no good reason. I don't believe this comment is right. If we time out before the server has even sent a response (or before the ResourceScheduler even schedules us, or before the cache entry is even open), the cache entry will not be truncated. (See also why none of these provisions for when we're "allowed to not revalidate" are meaningless. The fallback must always be the limited cache reuse flag.) https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:55: AsyncRevalidationDriver::~AsyncRevalidationDriver() { But for releasing the completion callback, this is already what the destruction order will do. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:80: net::URLRequest* unused, Nit: Match the header file's variable names. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:121: if (!response_info.response_time.is_null() && response_info.was_cached) { What is the response_time check for? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:131: if (request_->status().is_success()) { You've already checked this. The ResourceLoader code has to check again because the request may have been cancelled, but you don't even implemented the Cancel hooks. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:243: // make it stop. Is this still happening? In the previous version, you missed that cancelling would signal a failing OnResponseStarted, so denote the end of Start(). If you were to call Read() after that, I could believe that's why OnReadCompleted got signaled. Otherwise this doesn't make sense. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:248: completion_callback.Run(); base::ResetAndReturn(&completion_callback_).Run(); https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:248: completion_callback.Run(); Add: // |this| may be deleted after this point. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_driver.h (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:42: void CancelRequest(); What need is there to make CancelRequest() a distinct API from the destructor? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:37: static std::vector<std::string> test_authorities() { You needn't bother with this field. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:49: this, cert_request_info)); This is really obnoxious (we need to unrefcount this thing), but this needs to be WeakPtr'd and you need to override Kill() to InvalidateWeakPtrs(). See https://codereview.chromium.org/1395643003 https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:125: void set_defer_request_on_will_start_using_network( Unused https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:132: void set_defer_request_on_will_process_response( Unused https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:138: bool will_redirect_request_called() const { Unused https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:261: // Check that SelectClientCertificate wasn't called and the request aborted. Huh? You aren't checking that SelectClientCertificate wasn't called... https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:316: TEST_F(AsyncRevalidationDriverTest, DeferOnStart) { I don't think you ever test that resuming a revalidation works. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:368: : public net::URLRequestJobFactory::ProtocolHandler { This isn't in the anonymous namespace. Though you could also just wrap the entire file in the anonymous namespace which is probably simpler. I'd previously thought the convention was: namespace foo { namespace { // fixtures and helpers } // namespace // tests } // namespace foo But apparently other folks on //net prefer to write: namespace foo { namespace { // fixtures and helpers, tests, whatever } // namespace } // namespace foo So I probably am just misremembering what people told me the convention was. :-) Anyway, do what you like. FromCacheProtocolHandler probably shouldn't leak out of this compilation unit. Beyond that, it's probably less obnoxious if you're allowed to interleave them, so feel free to pick the latter one. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:62: // resource, it is okay not to perform an async revalidation. This comment is inaccurate. If the request turned into a download, we still consumed the body. This also will fail to do stale-while-revalidate for PlzNavigate, so you need at minimum a TODO here. Probably a bug as well. (This also underscores why the HttpCache logic is a hard blocker for the non-experimental implementation. So long as you are interacting with the RDH, you cannot be making this kinds of impossible-to-ensure assumptions to justify why we don't do an async revalidation. The implementation MUST be resilient to that.) https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:67: int route_id = info->GetRouteID(); This also will not work with PlzNavigate. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:70: // passed but only cares about the resource_type field. Change the type of the method to only take the resource_type. Otherwise this will break in the future without you noticing. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:79: // //chrome/browser/profiles/profile_io_data.h. //content referencing //chrome is a clear layering violation, and this comment will easily become inaccurate without anyone noticing. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:91: // don't need another one. This comment is inaccurate. If there is a Vary header, things won't behave correctly. This is fine because the real implementation will require the extra HttpCache provision, but the comment should be written accordingly. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:96: headers.AddHeadersFromString(info->original_headers()); What are the situations where this differs from URLRequest::extra_request_headers()? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:116: DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0); Don't blacklist load flags. This will easily break when new ones are added. Set the ones you need and whitelist what you carry over. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:122: auto& async_revalidation = insert_result.first->second; No non-const references. Also please spell out the type here. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:128: // callback. This is pretty verbose. How about: // AsyncRevalidationDriver does not outlive its entry in |in_progress_|, // so the iterator and |this| may be passed to base::Bind directly. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:149: in_progress_.erase(prev_it); All this can be replaced with: it->second->CancelRequest(); it = in_progress_.erase(it); https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:163: // A GET request should not have a body, but don't leave it to chance. "but don't leave it to chance" is weird to put in the comment. I'd just take it out. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:497: // have callbacks referencing it. What are "this object" and "it" here? RDH? ARM? I also don't understand what the concern is here. Surely at this point, CancelAsyncRevalidationsForRequestContext would have happened. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:602: // requests, but do it before the checks just to be on the safe side. checks -> CHECKs and "just to be on the safe side" -> "to ensure this does not happen". It took me several tries to understand what this comment was about. "just to be on the safe side" could just as easily mean "so we don't crash if this happens". https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:856: if (request->response_info().async_revalidation_required && !is_shutdown_) { In what situation could is_shutdown_ be true here? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:905: if (request->response_info().async_revalidation_required && !is_shutdown_) { In what situation can is_shutdown_ be true here? https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2146: true, // is_async BUG: You switched two parameters here. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2147: std::string()); // original_headers Please leave at least a TODO, preferably a bug, that this doesn't work with PlzNavigate. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:28: #include "content/browser/loader/async_revalidation_manager.h" Forward-decl
https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:12: #include "base/single_thread_task_runner.h" On 2015/11/23 23:40:41, davidben (slow) wrote: > Unused? Used. I've rewritten the code to make the use explicit (see the calls to PostTask). https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:17: #include "net/cert/cert_status_flags.h" On 2015/11/23 23:40:40, davidben (slow) wrote: > Unused? Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:18: #include "net/ssl/ssl_info.h" On 2015/11/23 23:40:40, davidben (slow) wrote: > Unused? Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:19: #include "net/url_request/url_request_context.h" On 2015/11/23 23:40:40, davidben (slow) wrote: > Unused? Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:30: // cache entry will be truncated for no good reason. On 2015/11/23 23:40:40, davidben (slow) wrote: > I don't believe this comment is right. If we time out before the server has even > sent a response (or before the ResourceScheduler even schedules us, or before > the cache entry is even open), the cache entry will not be truncated. This value isn't used for the response timeout. In fact, I realised that I have no timeout implemented at all for the period between when the connect succeeds and the response headers are received. This is pretty bad even for an experiment. So I added one using as little extra code as possible. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:55: AsyncRevalidationDriver::~AsyncRevalidationDriver() { On 2015/11/23 23:40:40, davidben (slow) wrote: > But for releasing the completion callback, this is already what the destruction > order will do. Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:80: net::URLRequest* unused, On 2015/11/23 23:40:40, davidben (slow) wrote: > Nit: Match the header file's variable names. Done. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:121: if (!response_info.response_time.is_null() && response_info.was_cached) { On 2015/11/23 23:40:40, davidben (slow) wrote: > What is the response_time check for? From the comment on the was_cached member in http_response_info.h: "The following is only defined if the request_time member is set.". Maybe this comment is intended as a simple statement of fact, but I interpreted it as an interface contract. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:131: if (request_->status().is_success()) { On 2015/11/23 23:40:40, davidben (slow) wrote: > You've already checked this. The ResourceLoader code has to check again because > the request may have been cancelled, but you don't even implemented the Cancel > hooks. Okay, that makes sense. Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:243: // make it stop. On 2015/11/23 23:40:40, davidben (slow) wrote: > Is this still happening? In the previous version, you missed that cancelling > would signal a failing OnResponseStarted, so denote the end of Start(). If you > were to call Read() after that, I could believe that's why OnReadCompleted got > signaled. Otherwise this doesn't make sense. Yes, still happening. It might just be a feature of URLRequestTestJob. I haven't checked whether the same thing happens with a real server. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:248: completion_callback.Run(); On 2015/11/23 23:40:40, davidben (slow) wrote: > base::ResetAndReturn(&completion_callback_).Run(); Thank you! I knew that existed but I couldn't remember what it was called. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:248: completion_callback.Run(); On 2015/11/23 23:40:40, davidben (slow) wrote: > Add: > // |this| may be deleted after this point. Done. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:37: static std::vector<std::string> test_authorities() { On 2015/11/23 23:40:41, davidben (slow) wrote: > You needn't bother with this field. Removed, thanks. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:49: this, cert_request_info)); On 2015/11/23 23:40:42, davidben (slow) wrote: > This is really obnoxious (we need to unrefcount this thing), but this needs to > be WeakPtr'd and you need to override Kill() to InvalidateWeakPtrs(). See > https://codereview.chromium.org/1395643003 I now have a hierarchy with three layers of WeakPtrFactorys. I feel weak. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:125: void set_defer_request_on_will_start_using_network( On 2015/11/23 23:40:41, davidben (slow) wrote: > Unused Removed, thanks. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:132: void set_defer_request_on_will_process_response( On 2015/11/23 23:40:41, davidben (slow) wrote: > Unused Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:138: bool will_redirect_request_called() const { On 2015/11/23 23:40:41, davidben (slow) wrote: > Unused Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:261: // Check that SelectClientCertificate wasn't called and the request aborted. On 2015/11/23 23:40:41, davidben (slow) wrote: > Huh? You aren't checking that SelectClientCertificate wasn't called... Good point. Fixed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:316: TEST_F(AsyncRevalidationDriverTest, DeferOnStart) { On 2015/11/23 23:40:41, davidben (slow) wrote: > I don't think you ever test that resuming a revalidation works. You're right. I have added one. And it found a bug. I wasn't called LogUnblocked() on the request. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:368: : public net::URLRequestJobFactory::ProtocolHandler { On 2015/11/23 23:40:41, davidben (slow) wrote: > This isn't in the anonymous namespace. Though you could also just wrap the > entire file in the anonymous namespace which is probably simpler. > > I'd previously thought the convention was: > > namespace foo { > namespace { > > // fixtures and helpers > > } // namespace > > // tests > > } // namespace foo > > But apparently other folks on //net prefer to write: > > namespace foo { > namespace { > > // fixtures and helpers, tests, whatever > > } // namespace > } // namespace foo > > So I probably am just misremembering what people told me the convention was. :-) > Anyway, do what you like. FromCacheProtocolHandler probably shouldn't leak out > of this compilation unit. Beyond that, it's probably less obnoxious if you're > allowed to interleave them, so feel free to pick the latter one. Both conventions are used, and both sides claim they are correct. I try to follow the style in the surrounding code to avoid the discussion. However, I do like to follow the "definition close to use" rule when it comes to tests, so I have put everything in the anonymous namespace like you said. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:62: // resource, it is okay not to perform an async revalidation. On 2015/11/23 23:40:42, davidben (slow) wrote: > This comment is inaccurate. If the request turned into a download, we still > consumed the body. This also will fail to do stale-while-revalidate for > PlzNavigate, so you need at minimum a TODO here. Probably a bug as well. > > (This also underscores why the HttpCache logic is a hard blocker for the > non-experimental implementation. So long as you are interacting with the RDH, > you cannot be making this kinds of impossible-to-ensure assumptions to justify > why we don't do an async revalidation. The implementation MUST be resilient to > that.) I added a TODO with a bug. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:67: int route_id = info->GetRouteID(); On 2015/11/23 23:40:42, davidben (slow) wrote: > This also will not work with PlzNavigate. I filed crbug.com/561610 to track the incompatibility, and modified the RDH constructor to never enable s-w-r when the PlzNavigate flag is set. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:70: // passed but only cares about the resource_type field. On 2015/11/23 23:40:42, davidben (slow) wrote: > Change the type of the method to only take the resource_type. Otherwise this > will break in the future without you noticing. Done in http://crrev.com/1481583002 https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:79: // //chrome/browser/profiles/profile_io_data.h. On 2015/11/23 23:40:42, davidben (slow) wrote: > //content referencing //chrome is a clear layering violation, and this comment > will easily become inaccurate without anyone noticing. Comment changed. PTAL. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:91: // don't need another one. On 2015/11/23 23:40:42, davidben (slow) wrote: > This comment is inaccurate. If there is a Vary header, things won't behave > correctly. This is fine because the real implementation will require the extra > HttpCache provision, but the comment should be written accordingly. I added an explanation of the Vary issue. PTAL. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:96: headers.AddHeadersFromString(info->original_headers()); On 2015/11/23 23:40:42, davidben (slow) wrote: > What are the situations where this differs from > URLRequest::extra_request_headers()? URLRequest-internal changes appear to only happen on redirect (after calling the delegate), so we don't have to worry about those. External changes that I have found: ChromeNetworkDelegate adds DNT: 1 PolicyHeaderIOHelper adds Chrome-Policy-Posture: <blob> signin_header_helper.cc adds an X-Chrome-Connected header sometimes (maybe redirects only?) ChromeResourceDispatcherHostDelegate adds X-Chrome-UMA-Enabled and X-Client-Data Those are the ones I've found with codesearch. They look manageable, but I'm not confident I've found everything and more are likely to appear without warning. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:116: DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0); On 2015/11/23 23:40:42, davidben (slow) wrote: > Don't blacklist load flags. This will easily break when new ones are added. Set > the ones you need and whitelist what you carry over. Done. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:122: auto& async_revalidation = insert_result.first->second; On 2015/11/23 23:40:42, davidben (slow) wrote: > No non-const references. Also please spell out the type here. Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:128: // callback. On 2015/11/23 23:40:42, davidben (slow) wrote: > This is pretty verbose. How about: > > // AsyncRevalidationDriver does not outlive its entry in |in_progress_|, > // so the iterator and |this| may be passed to base::Bind directly. Done. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:149: in_progress_.erase(prev_it); On 2015/11/23 23:40:42, davidben (slow) wrote: > All this can be replaced with: > > it->second->CancelRequest(); > it = in_progress_.erase(it); That's much better, thank you. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:163: // A GET request should not have a body, but don't leave it to chance. On 2015/11/23 23:40:42, davidben (slow) wrote: > "but don't leave it to chance" is weird to put in the comment. I'd just take it > out. Done. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:497: // have callbacks referencing it. On 2015/11/23 23:40:43, davidben (slow) wrote: > What are "this object" and "it" here? RDH? ARM? > > I also don't understand what the concern is here. Surely at this point, > CancelAsyncRevalidationsForRequestContext would have happened. Sorry, the comment was out-of-date, and there's no need to explicitly destroy async_revalidation_manager_ any more. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:602: // requests, but do it before the checks just to be on the safe side. On 2015/11/23 23:40:42, davidben (slow) wrote: > checks -> CHECKs and "just to be on the safe side" -> "to ensure this does not > happen". > > It took me several tries to understand what this comment was about. "just to be > on the safe side" could just as easily mean "so we don't crash if this happens". Sorry about that. It should be clearer now. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:856: if (request->response_info().async_revalidation_required && !is_shutdown_) { On 2015/11/23 23:40:43, davidben (slow) wrote: > In what situation could is_shutdown_ be true here? It can't. Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:905: if (request->response_info().async_revalidation_required && !is_shutdown_) { On 2015/11/23 23:40:42, davidben (slow) wrote: > In what situation can is_shutdown_ be true here? Removed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2146: true, // is_async On 2015/11/23 23:40:42, davidben (slow) wrote: > BUG: You switched two parameters here. Fixed. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2147: std::string()); // original_headers On 2015/11/23 23:40:42, davidben (slow) wrote: > Please leave at least a TODO, preferably a bug, that this doesn't work with > PlzNavigate. Done. https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:28: #include "content/browser/loader/async_revalidation_manager.h" On 2015/11/23 23:40:43, davidben (slow) wrote: > Forward-decl Done.
(Did lightweight review only) https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:41: // destroyed. nit: this comment seems obsolete now https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:110: // doing so. nit: maybe record UMA to see how often this could happen? https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:158: // contiguous in the map (ie. it must form the first part of the key). nit: feels a bit verbose, would something like following work? |resource_context| is the first part of the key, so elements to be cancelled are contiguous in the map. https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:40: void BeginAsyncRevalidation(net::URLRequest* for_request, if it's just to be copie-from could for_request be const ptr (or const ref)?
Thank you for the extra review. https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:41: // destroyed. On 2015/11/27 08:50:01, kinuko wrote: > nit: this comment seems obsolete now Thanks. Removed. https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:110: // doing so. On 2015/11/27 08:50:01, kinuko wrote: > nit: maybe record UMA to see how often this could happen? I just checked the code in net::HttpCache::Transaction and actually we never do an asynchronous revalidation on a Vary mismatch. This makes sense because when there is a Vary mismatch we have to revalidate regardless of whether the response is stale or not. I should have checked it before I wrote the long comment. I have rewritten the comment to be much shorter. Sorry for the trouble. https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:158: // contiguous in the map (ie. it must form the first part of the key). On 2015/11/27 08:50:01, kinuko wrote: > nit: feels a bit verbose, would something like following work? > > |resource_context| is the first part of the key, so elements to be cancelled are > contiguous in the map. That is much clearer, thanks! https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/830001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:40: void BeginAsyncRevalidation(net::URLRequest* for_request, On 2015/11/27 08:50:01, kinuko wrote: > if it's just to be copie-from could for_request be const ptr (or const ref)? Good point. I made it a const reference so it looks like an ordinary input parameter (which it is).
Some more review. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:237: scoped_ptr<ClientCertificateDelegate> delegate) override { This seems to require including client_certificate_delegate.h https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:310: // Verify that a cancelled request calls |completion_callback|. nit: Verify -> Verifies https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:63: // with. Could we make this comment a bit more specific? Just saying "this is a problem" makes me feel why we have this code now then. E.g. "The child has gone away and we can no longer get ResourceContext and URLRequestContext to perform async revalidation. This should be okay for most cases but not for downloads and PlzNavigate, where we might be returning stale body. TODO(ricea)..." (Feel free to rephrase with better English) https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:69: int route_id = info->GetRouteID(); Are these now only used on line 125 (for ScheduleRequest)? Please move these down there then. Please also add a short comment to note that "this part doesn't work with PlzNavigate" here too with a link to crbug.com/561610 https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:73: // fields. This comment looks stale now. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:83: base::Callback<net::URLRequestJob*(net::URLRequest*, nit: using/typedef's at the beginning of declarations per style guide https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:178: delete msg; nit: receive msg as scoped_ptr and let it delete, let's avoid using 'delete' https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:256: void HandleScheme(const std::string& scheme) { If we only test "http" (and it looks like the case) this probably doesn't need to be a method but TestURLRequestJobFactory can statically return true for "http" in IsHandle{Protocol,URL} https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:284: policy->RegisterWebSafeScheme(scheme); Feels a bit overkill for testing where 'scheme' is nearly constant... or am I missing something? https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:287: net::TestNetworkDelegate* network_delegate() { not used? https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:444: bool IsEmpty() { return requests_.empty(); } nit: could be a const method https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2126: // and wouldn't work properly with stale-while-revalidate. crbug.com/561610 This comment isn't feels a bit unclear when readers don't have context for s-w-r and doesn't really look like a TODO. "The original_headers field is for stale-while-revalidate but the feature doesn't work with PlzNavigate, so it's just a placeholder here. Make the feature work with stale-while-revalidate and cleanup this." or something? Also as in line 2138 or 2148 I think the comment should be placed right before the parameter? https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2157: std::string()); // original_headers We don't seem to be really aligning comments in this way, do we? (E.g. line 2153 and line 2154 look misaligned)
btw when your patch has compile errors reviewers tend to just defer reviewing, would be nice to check try results...
https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:237: scoped_ptr<ClientCertificateDelegate> delegate) override { On 2015/12/03 07:51:41, kinuko wrote: > This seems to require including client_certificate_delegate.h Sorry I missed that. Fixed. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:310: // Verify that a cancelled request calls |completion_callback|. On 2015/12/03 07:51:41, kinuko wrote: > nit: Verify -> Verifies Done. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:63: // with. On 2015/12/03 07:51:41, kinuko wrote: > Could we make this comment a bit more specific? Just saying "this is a problem" > makes me feel why we have this code now then. > > E.g. "The child has gone away and we can no longer get ResourceContext and > URLRequestContext to perform async revalidation. This should be okay for most > cases but not for downloads and PlzNavigate, where we might be returning stale > body. TODO(ricea)..." > > (Feel free to rephrase with better English) I investigated the various cases and updated the comment. Apart from the known issue with PlzNavigate, it is not as bad as I thought. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:69: int route_id = info->GetRouteID(); On 2015/12/03 07:51:41, kinuko wrote: > Are these now only used on line 125 (for ScheduleRequest)? Please move these > down there then. Please also add a short comment to note that "this part doesn't > work with PlzNavigate" here too with a link to crbug.com/561610 Done. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:73: // fields. On 2015/12/03 07:51:41, kinuko wrote: > This comment looks stale now. Sorry. Removed. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:83: base::Callback<net::URLRequestJob*(net::URLRequest*, On 2015/12/03 07:51:41, kinuko wrote: > nit: using/typedef's at the beginning of declarations per style guide Done. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:178: delete msg; On 2015/12/03 07:51:41, kinuko wrote: > nit: receive msg as scoped_ptr and let it delete, let's avoid using 'delete' Done. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:256: void HandleScheme(const std::string& scheme) { On 2015/12/03 07:51:41, kinuko wrote: > If we only test "http" (and it looks like the case) this probably doesn't need > to be a method but TestURLRequestJobFactory can statically return true for > "http" in IsHandle{Protocol,URL} Thanks. This saves a lot of code. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:284: policy->RegisterWebSafeScheme(scheme); On 2015/12/03 07:51:41, kinuko wrote: > Feels a bit overkill for testing where 'scheme' is nearly constant... or am I > missing something? No, you are correct. Removed. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:287: net::TestNetworkDelegate* network_delegate() { On 2015/12/03 07:51:41, kinuko wrote: > not used? Removed. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:444: bool IsEmpty() { return requests_.empty(); } On 2015/12/03 07:51:41, kinuko wrote: > nit: could be a const method Done. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2126: // and wouldn't work properly with stale-while-revalidate. crbug.com/561610 On 2015/12/03 07:51:41, kinuko wrote: > This comment isn't feels a bit unclear when readers don't have context for s-w-r > and doesn't really look like a TODO. > > "The original_headers field is for stale-while-revalidate but the feature > doesn't work with PlzNavigate, so it's just a placeholder here. Make the > feature work with stale-while-revalidate and cleanup this." or something? > > Also as in line 2138 or 2148 I think the comment should be placed right before > the parameter? Done. https://codereview.chromium.org/1041993004/diff/870001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2157: std::string()); // original_headers On 2015/12/03 07:51:41, kinuko wrote: > We don't seem to be really aligning comments in this way, do we? > > (E.g. line 2153 and line 2154 look misaligned) Fixed.
On 2015/12/03 07:52:47, kinuko wrote: > btw when your patch has compile errors reviewers tend to just defer reviewing, > would be nice to check try results... Sorry, I was over-confident that the try bots would pass.
Ok let me give lgtm for this one. You'd probably want to explicitly ask davidben or someone in net-dev to do final review after addressing my comments. https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:299: TEST_F(AsyncRevalidationDriverTest, ResumeCancelledRequest) { Can the tests use the same base class (e.g. AsyncRevalidationDriverTest) placed together so that a series of tests are not interleaved with class definitions for other series of tests? Currently the way how class defs and tests are ordered looks a bit random. Why MockSSLErrorJobProtocolHandler is defined far from AsyncRevalidationDriverSSLErrorTest, while some other helper classes like FromCacheURLRequestJob are placed below next to where it's used? I think popular way to order classes + tests is either: 1) define all helper classes and test classes at the beginning of test files and then all tests follow, or 2) put helper classes for one series of tests + the corresponding tests always together, but not in a intermixed way (e.g. AsyncRevalidationDriverTest classes + tests first, then AsyncRevalidationDriverSSLErrorTest classes + tests second and so on). https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:65: // 1. PlzNavigate (but it won't be enabled; see crbug.com/561609) nit: won't be enabled -> is not enabled yet ? https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:337: class AsyncRevalidationRequiredURLRequestTestJob The usage of anon namespace in this file feels inconsistent... some helper classes are inside an anon namespace while others (like this) are not. Also it's inconsistent with the other unittest. Can they be ordered / placed in a single understandable way? https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:495: // enabled. nit: the line 493-495 could probably fit in two lines? https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:497: // navigation becomes the default. Or make them work together. nit: I'd probably write this the other way around; make them work together, or disable if we couldn't.
https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:299: TEST_F(AsyncRevalidationDriverTest, ResumeCancelledRequest) { On 2015/12/05 07:21:37, kinuko wrote: > Can the tests use the same base class (e.g. AsyncRevalidationDriverTest) placed > together so that a series of tests are not interleaved with class definitions > for other series of tests? Currently the way how class defs and tests are > ordered looks a bit random. Why MockSSLErrorJobProtocolHandler is defined far > from AsyncRevalidationDriverSSLErrorTest, while some other helper classes like > FromCacheURLRequestJob are placed below next to where it's used? > > I think popular way to order classes + tests is either: 1) define all helper > classes and test classes at the beginning of test files and then all tests > follow, or 2) put helper classes for one series of tests + the corresponding > tests always together, but not in a intermixed way (e.g. > AsyncRevalidationDriverTest classes + tests first, then > AsyncRevalidationDriverSSLErrorTest classes + tests second and so on). I chose option 2. https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:65: // 1. PlzNavigate (but it won't be enabled; see crbug.com/561609) On 2015/12/05 07:21:37, kinuko wrote: > nit: won't be enabled -> is not enabled yet ? I meant that it won't be enabled if we are executing this code. I have clarified that stale-while-revalidate is disabled when PlzNavigate is turned on. https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:337: class AsyncRevalidationRequiredURLRequestTestJob On 2015/12/05 07:21:37, kinuko wrote: > The usage of anon namespace in this file feels inconsistent... some helper > classes are inside an anon namespace while others (like this) are not. Also > it's inconsistent with the other unittest. Can they be ordered / placed in a > single understandable way? I just put the whole file inside the anonymous namespace. https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:495: // enabled. On 2015/12/05 07:21:37, kinuko wrote: > nit: the line 493-495 could probably fit in two lines? Done. https://codereview.chromium.org/1041993004/diff/930001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:497: // navigation becomes the default. Or make them work together. On 2015/12/05 07:21:37, kinuko wrote: > nit: I'd probably write this the other way around; make them work together, or > disable if we couldn't. Done.
Sorry, I really should have gotten to this last week. This should be the last round-trip. Really the only non-trivial thing is the comment about whether you need CancelRequest to be distinct from destruction. (It is distinct in URLRequest and causes us no end of trouble. I think mmenke and I would both agree that was a mistake.) https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/810001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:243: // make it stop. On 2015/11/25 19:39:39, Adam Rice wrote: > On 2015/11/23 23:40:40, davidben (slow) wrote: > > Is this still happening? In the previous version, you missed that cancelling > > would signal a failing OnResponseStarted, so denote the end of Start(). If you > > were to call Read() after that, I could believe that's why OnReadCompleted got > > signaled. Otherwise this doesn't make sense. > > Yes, still happening. It might just be a feature of URLRequestTestJob. I haven't > checked whether the same thing happens with a real server. And confirmed. Sigh. https://crbug.com/564820. (Mind sticking that in the comment? I'll do a fix asynchronously.) Edit: Apparently I'm slow enough at complex reviews that that's already been fixed. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:201: base::ThreadTaskRunnerHandle::Get(); Nit: Why the change? The old one seems shorter. (base::TTRH::Get()->PostTask(...)) https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:222: base::ThreadTaskRunnerHandle::Get(); Ditto. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:250: void AsyncRevalidationDriver::OnTimeout(TimeoutType type) { Since you allow for the object to be in a canceled but not yet destroyed state, OnTimeout may trigger after you call CancelRequest, which seems poor. But it doesn't seem like you actually need that state. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_driver.h (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:42: void CancelRequest(); What need is there to make CancelRequest() a distinct API from the destructor? When you can avoid it, just using the destructor makes a lot of questions simpler as you no longer have distinct states for things. It's especially confusing because CancelRequest will call completion_callback, but only if you don't destroy the driver first. (Same comment from previous iteration. Might have missed it.) https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:90: } [ Haha. I suppose that's one way to do it. :-) ] https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:163: // Verifies that resuming a cancelled request does not start it again. If you make cancel the same as destruction, this state can't occur. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:104: // the request headers differ. Huh? What does this comment even mean? I would, like the in the header file, leave a TODO about the missing HttpCache logic. (Or maybe just remove these couple of sentences in favor of the header talking about the problem.) https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:134: net::LOAD_MAYBE_USER_GESTURE | net::LOAD_DO_NOT_USE_EMBEDDED_IDENTITY); Carrying over LOAD_PREFETCH seems wrong. That turns on various interesting cache behaviors. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:61: // performing additional async revalidations will not improve matters. Nit: This comment assumes net::HttpCache won't eventually become smarter about Vary. But it's fine. The real fix here is to implement the behavior where a s-w-r'd cache entry's lifetime is bounded which will avoid all issues. I would probably just have written: // Request headers are intentionally not included in the key for simplicity, as they usually don't affect caching. // // TODO(ricea): Limit reuse of stale cache entries so that, if Vary is used, the skipped asynchronous revalidation is harmless. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:71: net::NetworkDelegate*)>; Since you only ever create two job types and one of their create functions depends on the other anyway, why not instead have your TestURLRequestJobFactory::MaybeCreate... method do the string check and have your tests either include or not include "redirect" in the URL as-needed? That would save you all this callback machinery and a somewhat odd RedirectAndRevalidate...::Create method. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:204: request.first_party_for_cookies = url; // bypass third-party cookie blocking Nit: Capitalize and end with period. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:432: } Might be worth also listening for destruction and doing some kind of appropriate DCHECK. Maybe something like... after any URLRequest is destroyed, you're not allowed to call NextRequest or IsEmpty anymore? Since after that it gets pretty screwy. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:500: (stale_while_revalidate_trial_group == "Enabled" || I think typically field trials check a prefix so we have some flexibility in extending this later. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:879: // TODO(ricea): Fix this before launching the feature. (Or you could do the HttpCache thing, which is already a prerequisite for correctness...) https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1380: (!is_sync_load && async_revalidation_manager_ && Why does is_sync_load matter? https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1381: AsyncRevalidationManager::QualifiesForAsyncRevalidation(request_data)); Nit: unnecessary parens
https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:201: base::ThreadTaskRunnerHandle::Get(); On 2015/12/07 23:56:03, davidben (behind on reviews) wrote: > Nit: Why the change? The old one seems shorter. > (base::TTRH::Get()->PostTask(...)) I got tired of people asking me why I need the single_thread_task_runner.h include file. So I made the usage explicit. I don't have any better ideas. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver.cc:250: void AsyncRevalidationDriver::OnTimeout(TimeoutType type) { On 2015/12/07 23:56:03, davidben (behind on reviews) wrote: > Since you allow for the object to be in a canceled but not yet destroyed state, > OnTimeout may trigger after you call CancelRequest, which seems poor. But it > doesn't seem like you actually need that state. The object can still be in a zombie state if it calls completion_callback_.Run() and isn't immediately deleted. I modified the tests so they always delete it immediately, so now this never happens in practice. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_driver.h (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver.h:42: void CancelRequest(); On 2015/12/07 23:56:03, davidben (behind on reviews) wrote: > What need is there to make CancelRequest() a distinct API from the destructor? > When you can avoid it, just using the destructor makes a lot of questions > simpler as you no longer have distinct states for things. > > It's especially confusing because CancelRequest will call completion_callback, > but only if you don't destroy the driver first. > > (Same comment from previous iteration. Might have missed it.) Sorry I missed this. I have removed CancelRequest(). Many things are now simpler. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_driver_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:90: } On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > [ Haha. I suppose that's one way to do it. :-) ] I considered making the test fixture itself templated, but I decided that would waste everyone's time wondering whether templated test fixtures had any weird interactions with the GTest framework. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_driver_unittest.cc:163: // Verifies that resuming a cancelled request does not start it again. On 2015/12/07 23:56:03, davidben (behind on reviews) wrote: > If you make cancel the same as destruction, this state can't occur. Yes, thank you. The test has gone away. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:104: // the request headers differ. On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Huh? What does this comment even mean? > > I would, like the in the header file, leave a TODO about the missing HttpCache > logic. (Or maybe just remove these couple of sentences in favor of the header > talking about the problem.) Removed in favour of the comment in the header. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:134: net::LOAD_MAYBE_USER_GESTURE | net::LOAD_DO_NOT_USE_EMBEDDED_IDENTITY); On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Carrying over LOAD_PREFETCH seems wrong. That turns on various interesting cache > behaviors. I added LOAD_PREFETCH to the prohibited flags in QualifiesForAsyncRevalidation() too, since otherwise an async revalidation could trick the cache into thinking the prefetched resource had been used. Since prefetch is already an asynchronous operation, it doesn't seem useful to try to make it more asynchronous, so an argument can be made that this is the "correct" behaviour. Though I suppose it will also have to be explicitly standardised. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:61: // performing additional async revalidations will not improve matters. On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Nit: This comment assumes net::HttpCache won't eventually become smarter about > Vary. But it's fine. The real fix here is to implement the behavior where a > s-w-r'd cache entry's lifetime is bounded which will avoid all issues. I would > probably just have written: > > // Request headers are intentionally not included in the key for simplicity, as > they usually don't affect caching. > // > // TODO(ricea): Limit reuse of stale cache entries so that, if Vary is used, the > skipped asynchronous revalidation is harmless. I filed a bug for it and added a link. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:71: net::NetworkDelegate*)>; On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Since you only ever create two job types and one of their create functions > depends on the other anyway, why not instead have your > TestURLRequestJobFactory::MaybeCreate... method do the string check and have > your tests either include or not include "redirect" in the URL as-needed? That > would save you all this callback machinery and a somewhat odd > RedirectAndRevalidate...::Create method. Okay, done. That's saved quite a bit of code. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:204: request.first_party_for_cookies = url; // bypass third-party cookie blocking On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Nit: Capitalize and end with period. Done. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:432: } On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Might be worth also listening for destruction and doing some kind of appropriate > DCHECK. Maybe something like... after any URLRequest is destroyed, you're not > allowed to call NextRequest or IsEmpty anymore? Since after that it gets pretty > screwy. I made it so that it removes dangling pointers when the URLRequest is destroyed, but you can still check that the URLRequest actually existed. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:500: (stale_while_revalidate_trial_group == "Enabled" || On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > I think typically field trials check a prefix so we have some flexibility in > extending this later. Done. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:879: // TODO(ricea): Fix this before launching the feature. On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > (Or you could do the HttpCache thing, which is already a prerequisite for > correctness...) Acknowledged. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1380: (!is_sync_load && async_revalidation_manager_ && On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Why does is_sync_load matter? It changes caching behaviour. And just generally breaks stuff. In early versions of this CL I didn't support it because it required a different code path, but now I'm mostly just being superstitious. Friends don't let friends use sync XHR. https://codereview.chromium.org/1041993004/diff/950001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1381: AsyncRevalidationManager::QualifiesForAsyncRevalidation(request_data)); On 2015/12/07 23:56:04, davidben (behind on reviews) wrote: > Nit: unnecessary parens Removed.
lgtm https://codereview.chromium.org/1041993004/diff/970001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/970001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:561: TEST_F(AsyncRevalidationManagerRecordingTest, NoSWRAfterFirstRedirectLeg) { It looks like this test tests strictly more things than the one above.
https://codereview.chromium.org/1041993004/diff/970001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/1041993004/diff/970001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:561: TEST_F(AsyncRevalidationManagerRecordingTest, NoSWRAfterFirstRedirectLeg) { On 2015/12/08 21:35:48, davidben (behind on reviews) wrote: > It looks like this test tests strictly more things than the one above. Yes, the one above looks redundant now. I removed it.
ricea@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for actions.xml. All actions are recorded in content/browser/loader/async_revalidation_driver.cc.
https://codereview.chromium.org/1041993004/diff/1010001/content/browser/loade... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/1010001/content/browser/loade... content/browser/loader/async_revalidation_driver.cc:261: RecordAction(base::UserMetricsAction("AsyncRevalidationReadTimeout")); Given these are not user actions in the semantical sense (i.e. it's not what a user did), I suggest using a histogram for these. In fact, it will also be more efficient in terms of memory use, UMA upload size, etc. I suggest defining a single enum histogram with enums for the different events.
https://codereview.chromium.org/1041993004/diff/1010001/content/browser/loade... File content/browser/loader/async_revalidation_driver.cc (right): https://codereview.chromium.org/1041993004/diff/1010001/content/browser/loade... content/browser/loader/async_revalidation_driver.cc:261: RecordAction(base::UserMetricsAction("AsyncRevalidationReadTimeout")); On 2015/12/09 18:07:04, Alexei Svitkine (slow) wrote: > Given these are not user actions in the semantical sense (i.e. it's not what a > user did), I suggest using a histogram for these. In fact, it will also be more > efficient in terms of memory use, UMA upload size, etc. I suddenly understand the significance of the word "user" in "user actions". Thank you for opening my eyes. > I suggest defining a single enum histogram with enums for the different events. Done. I added histograms for net errors at the same time. PTAL.
lgtm
The CQ bit was checked by ricea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1041993004/#ps1030001 (title: "Use histograms instead of user actions.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1041993004/1030001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1041993004/1030001
Message was sent while issue was closed.
Description was changed from ========== content::ResourceDispatcherHostImpl changes for stale-while-revalidate Inform //net that we support async revalidation using the net::LOAD_SUPPORT_ASYNC_REVALIDATION flag. Issue async revalidations when response info had the async_revalidation_required flag. See design doc at https://docs.google.com/a/chromium.org/document/d/1nBhr25nSJgoyAh4S1-U5h2sH70... BUG=348877 TEST=content_unittests ========== to ========== content::ResourceDispatcherHostImpl changes for stale-while-revalidate Inform //net that we support async revalidation using the net::LOAD_SUPPORT_ASYNC_REVALIDATION flag. Issue async revalidations when response info had the async_revalidation_required flag. See design doc at https://docs.google.com/a/chromium.org/document/d/1nBhr25nSJgoyAh4S1-U5h2sH70... BUG=348877 TEST=content_unittests ==========
Message was sent while issue was closed.
Committed patchset #52 (id:1030001)
Message was sent while issue was closed.
Description was changed from ========== content::ResourceDispatcherHostImpl changes for stale-while-revalidate Inform //net that we support async revalidation using the net::LOAD_SUPPORT_ASYNC_REVALIDATION flag. Issue async revalidations when response info had the async_revalidation_required flag. See design doc at https://docs.google.com/a/chromium.org/document/d/1nBhr25nSJgoyAh4S1-U5h2sH70... BUG=348877 TEST=content_unittests ========== to ========== content::ResourceDispatcherHostImpl changes for stale-while-revalidate Inform //net that we support async revalidation using the net::LOAD_SUPPORT_ASYNC_REVALIDATION flag. Issue async revalidations when response info had the async_revalidation_required flag. See design doc at https://docs.google.com/a/chromium.org/document/d/1nBhr25nSJgoyAh4S1-U5h2sH70... BUG=348877 TEST=content_unittests Committed: https://crrev.com/96e3ce421fbe8b8133bf9852b7d7cf16e8521026 Cr-Commit-Position: refs/heads/master@{#364253} ==========
Message was sent while issue was closed.
Patchset 52 (id:??) landed as https://crrev.com/96e3ce421fbe8b8133bf9852b7d7cf16e8521026 Cr-Commit-Position: refs/heads/master@{#364253} |
