Commit 0b2b1a92 authored by Arthur Sonzogni's avatar Arthur Sonzogni Committed by Commit Bot

Revert "Report was_cached in SubresourceLoadInfo"

This reverts commit d46ddaea.

Reason for revert: 2 tests are failing with NavigationMojoResponse.
See https://crbug.com/822237
The two failing are disabled with the NetworkService.
I can't fix the reverted CL. It has removed URLLoaderCompletionStatus.exists_in_cache and rely on NavigationURLLoaderNetworkService::OnResponseStarted to memorize if the response is in the cache.
In a lot of cases when NavigationURLLoaderDelegate::OnRequestFailed() is called, NavigationURLLoaderNetworkService::OnResponseStarted() is not called. It means |has_stale_copy_in_cache| will be set to false even if the failed request is in cache.

Original change's description:
> Report was_cached in SubresourceLoadInfo
> 
> This change adds a boolean `was_cached` to the SubresourceLoadInfo
> struct.  This lets consumers know whether or not the response was
> fetched from the network cache.  Notably, this is important for
> recording page load metrics when the Network Service is enabled.
> 
> BUG=816684
> 
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
> Change-Id: I28d60ff7286feca5d333203405ee9c351f43c0ab
> Reviewed-on: https://chromium-review.googlesource.com/938650
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Jay Civelli <jcivelli@chromium.org>
> Commit-Queue: Conley Owens <cco3@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#543276}

TBR=dcheng@chromium.org,jcivelli@chromium.org,jam@chromium.org,cco3@chromium.org

Change-Id: I23b197839b9b35af7d79ed03320a7494635c61bf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 816684
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/964461Reviewed-by: default avatarArthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543375}
parent 4087a4cc
...@@ -327,6 +327,10 @@ void AppCacheURLLoaderJob::NotifyCompleted(int error_code) { ...@@ -327,6 +327,10 @@ void AppCacheURLLoaderJob::NotifyCompleted(int error_code) {
network::URLLoaderCompletionStatus status(error_code); network::URLLoaderCompletionStatus status(error_code);
if (!error_code) { if (!error_code) {
const net::HttpResponseInfo* http_info =
is_range_request() ? range_response_info_.get()
: (info_ ? info_->http_response_info() : nullptr);
status.exists_in_cache = http_info->was_cached;
status.completion_time = base::TimeTicks::Now(); status.completion_time = base::TimeTicks::Now();
status.encoded_body_length = status.encoded_body_length =
is_range_request() ? range_response_info_->headers->GetContentLength() is_range_request() ? range_response_info_->headers->GetContentLength()
......
...@@ -486,6 +486,7 @@ void MojoAsyncResourceHandler::OnResponseCompleted( ...@@ -486,6 +486,7 @@ void MojoAsyncResourceHandler::OnResponseCompleted(
network::URLLoaderCompletionStatus loader_status; network::URLLoaderCompletionStatus loader_status;
loader_status.error_code = error_code; loader_status.error_code = error_code;
loader_status.exists_in_cache = request()->response_info().was_cached;
loader_status.completion_time = base::TimeTicks::Now(); loader_status.completion_time = base::TimeTicks::Now();
loader_status.encoded_data_length = request()->GetTotalReceivedBytes(); loader_status.encoded_data_length = request()->GetTotalReceivedBytes();
loader_status.encoded_body_length = request()->GetRawBodyBytes(); loader_status.encoded_body_length = request()->GetRawBodyBytes();
......
...@@ -972,7 +972,6 @@ NavigationURLLoaderNetworkService::NavigationURLLoaderNetworkService( ...@@ -972,7 +972,6 @@ NavigationURLLoaderNetworkService::NavigationURLLoaderNetworkService(
std::vector<std::unique_ptr<URLLoaderRequestHandler>> initial_handlers) std::vector<std::unique_ptr<URLLoaderRequestHandler>> initial_handlers)
: delegate_(delegate), : delegate_(delegate),
allow_download_(request_info->common_params.allow_download), allow_download_(request_info->common_params.allow_download),
response_was_cached_(false),
weak_factory_(this) { weak_factory_(this) {
DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK_CURRENTLY_ON(BrowserThread::UI);
int frame_tree_node_id = request_info->frame_tree_node_id; int frame_tree_node_id = request_info->frame_tree_node_id;
...@@ -1119,7 +1118,6 @@ void NavigationURLLoaderNetworkService::OnReceiveResponse( ...@@ -1119,7 +1118,6 @@ void NavigationURLLoaderNetworkService::OnReceiveResponse(
net::SSLInfo ssl_info; net::SSLInfo ssl_info;
if (maybe_ssl_info.has_value()) if (maybe_ssl_info.has_value())
ssl_info = maybe_ssl_info.value(); ssl_info = maybe_ssl_info.value();
response_was_cached_ = response->head.was_cached;
delegate_->OnResponseStarted( delegate_->OnResponseStarted(
std::move(response), std::move(url_loader_client_endpoints), nullptr, std::move(response), std::move(url_loader_client_endpoints), nullptr,
...@@ -1144,7 +1142,7 @@ void NavigationURLLoaderNetworkService::OnComplete( ...@@ -1144,7 +1142,7 @@ void NavigationURLLoaderNetworkService::OnComplete(
"&NavigationURLLoaderNetworkService", this, "success", "&NavigationURLLoaderNetworkService", this, "success",
false); false);
delegate_->OnRequestFailed(response_was_cached_, status.error_code, delegate_->OnRequestFailed(status.exists_in_cache, status.error_code,
status.ssl_info); status.ssl_info);
} }
......
...@@ -78,7 +78,6 @@ class CONTENT_EXPORT NavigationURLLoaderNetworkService ...@@ -78,7 +78,6 @@ class CONTENT_EXPORT NavigationURLLoaderNetworkService
std::unique_ptr<URLLoaderRequestController> request_controller_; std::unique_ptr<URLLoaderRequestController> request_controller_;
bool allow_download_; bool allow_download_;
bool response_was_cached_;
// Factories to handle navigation requests for non-network resources. // Factories to handle navigation requests for non-network resources.
ContentBrowserClient::NonNetworkURLLoaderFactoryMap ContentBrowserClient::NonNetworkURLLoaderFactoryMap
......
...@@ -168,6 +168,7 @@ void AbortRequestBeforeItStarts( ...@@ -168,6 +168,7 @@ void AbortRequestBeforeItStarts(
// Tell the renderer that this request was disallowed. // Tell the renderer that this request was disallowed.
network::URLLoaderCompletionStatus status; network::URLLoaderCompletionStatus status;
status.error_code = net::ERR_ABORTED; status.error_code = net::ERR_ABORTED;
status.exists_in_cache = false;
// No security info needed, connection not established. // No security info needed, connection not established.
status.completion_time = base::TimeTicks(); status.completion_time = base::TimeTicks();
status.encoded_data_length = 0; status.encoded_data_length = 0;
......
...@@ -64,7 +64,6 @@ void PopulateResourceResponse( ...@@ -64,7 +64,6 @@ void PopulateResourceResponse(
response->head.headers = request->response_headers(); response->head.headers = request->response_headers();
request->GetCharset(&response->head.charset); request->GetCharset(&response->head.charset);
response->head.content_length = request->GetExpectedContentSize(); response->head.content_length = request->GetExpectedContentSize();
response->head.was_cached = request->was_cached();
request->GetMimeType(&response->head.mime_type); request->GetMimeType(&response->head.mime_type);
net::HttpResponseInfo response_info = request->response_info(); net::HttpResponseInfo response_info = request->response_info();
response->head.was_fetched_via_spdy = response_info.was_fetched_via_spdy; response->head.was_fetched_via_spdy = response_info.was_fetched_via_spdy;
......
...@@ -523,40 +523,6 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, ...@@ -523,40 +523,6 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
EXPECT_TRUE(new_web_contents_observer.RenderViewCreatedCalled()); EXPECT_TRUE(new_web_contents_observer.RenderViewCreatedCalled());
} }
// Observer class to track subresource loads.
class SubresourceLoadObserver : public WebContentsObserver {
public:
explicit SubresourceLoadObserver(Shell* shell)
: WebContentsObserver(shell->web_contents()) {}
void SubresourceResponseStarted(
const mojom::SubresourceLoadInfo& subresource_load_info) override {
last_subresource_load_info_ = subresource_load_info.Clone();
}
mojom::SubresourceLoadInfo* last_subresource_load_info() const {
return last_subresource_load_info_.get();
}
private:
mojom::SubresourceLoadInfoPtr last_subresource_load_info_;
DISALLOW_COPY_AND_ASSIGN(SubresourceLoadObserver);
};
IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
SubresourceLoadInfoReportsWasCached) {
SubresourceLoadObserver observer(shell());
ASSERT_TRUE(embedded_test_server()->Start());
GURL url(
embedded_test_server()->GetURL("/page_with_cached_subresource.html"));
NavigateToURL(shell(), url);
EXPECT_FALSE(observer.last_subresource_load_info()->was_cached);
NavigateToURL(shell(), url);
EXPECT_TRUE(observer.last_subresource_load_info()->was_cached);
}
struct LoadProgressDelegateAndObserver : public WebContentsDelegate, struct LoadProgressDelegateAndObserver : public WebContentsDelegate,
public WebContentsObserver { public WebContentsObserver {
explicit LoadProgressDelegateAndObserver(Shell* shell) explicit LoadProgressDelegateAndObserver(Shell* shell)
......
...@@ -29,7 +29,4 @@ struct SubresourceLoadInfo { ...@@ -29,7 +29,4 @@ struct SubresourceLoadInfo {
// Bitmask of status info of the SSL certificate. // Bitmask of status info of the SSL certificate.
// See net/cert/cert_status_flags.h // See net/cert/cert_status_flags.h
uint32 cert_status; uint32 cert_status;
// True if the response was fetched from the network cache.
bool was_cached;
}; };
...@@ -162,7 +162,6 @@ void ResourceDispatcher::OnReceivedResponse( ...@@ -162,7 +162,6 @@ void ResourceDispatcher::OnReceivedResponse(
} }
} }
subresource_load_info->cert_status = response_head.cert_status; subresource_load_info->cert_status = response_head.cert_status;
subresource_load_info->was_cached = response_head.was_cached;
NotifySubresourceStarted(RenderThreadImpl::DeprecatedGetMainTaskRunner(), NotifySubresourceStarted(RenderThreadImpl::DeprecatedGetMainTaskRunner(),
request_info->render_frame_id, request_info->render_frame_id,
std::move(subresource_load_info)); std::move(subresource_load_info));
......
...@@ -246,6 +246,7 @@ TEST_F(ResourceDispatcherTest, DelegateTest) { ...@@ -246,6 +246,7 @@ TEST_F(ResourceDispatcherTest, DelegateTest) {
// peer at once. // peer at once.
network::URLLoaderCompletionStatus status; network::URLLoaderCompletionStatus status;
status.error_code = net::OK; status.error_code = net::OK;
status.exists_in_cache = false;
status.encoded_data_length = strlen(kTestPageContents); status.encoded_data_length = strlen(kTestPageContents);
client->OnComplete(status); client->OnComplete(status);
...@@ -291,6 +292,7 @@ TEST_F(ResourceDispatcherTest, CancelDuringCallbackWithWrapperPeer) { ...@@ -291,6 +292,7 @@ TEST_F(ResourceDispatcherTest, CancelDuringCallbackWithWrapperPeer) {
// OnCompletedRequest, but it should not lead to crashes.) // OnCompletedRequest, but it should not lead to crashes.)
network::URLLoaderCompletionStatus status; network::URLLoaderCompletionStatus status;
status.error_code = net::OK; status.error_code = net::OK;
status.exists_in_cache = false;
status.encoded_data_length = strlen(kTestPageContents); status.encoded_data_length = strlen(kTestPageContents);
client->OnComplete(status); client->OnComplete(status);
......
...@@ -462,7 +462,6 @@ class WebURLLoaderImpl::Context : public base::RefCounted<Context> { ...@@ -462,7 +462,6 @@ class WebURLLoaderImpl::Context : public base::RefCounted<Context> {
enum DeferState {NOT_DEFERRING, SHOULD_DEFER, DEFERRED_DATA}; enum DeferState {NOT_DEFERRING, SHOULD_DEFER, DEFERRED_DATA};
DeferState defers_loading_; DeferState defers_loading_;
int request_id_; int request_id_;
bool response_was_cached_;
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_; scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
}; };
...@@ -542,7 +541,6 @@ WebURLLoaderImpl::Context::Context( ...@@ -542,7 +541,6 @@ WebURLLoaderImpl::Context::Context(
: nullptr), : nullptr),
defers_loading_(NOT_DEFERRING), defers_loading_(NOT_DEFERRING),
request_id_(-1), request_id_(-1),
response_was_cached_(false),
url_loader_factory_(std::move(url_loader_factory)) { url_loader_factory_(std::move(url_loader_factory)) {
DCHECK(url_loader_factory_ || !resource_dispatcher); DCHECK(url_loader_factory_ || !resource_dispatcher);
} }
...@@ -879,7 +877,6 @@ void WebURLLoaderImpl::Context::OnReceivedResponse( ...@@ -879,7 +877,6 @@ void WebURLLoaderImpl::Context::OnReceivedResponse(
// TODO(yhirano): Support ftp listening and multipart // TODO(yhirano): Support ftp listening and multipart
return; return;
} }
response_was_cached_ = info.was_cached;
client_->DidReceiveResponse(response); client_->DidReceiveResponse(response);
...@@ -966,8 +963,8 @@ void WebURLLoaderImpl::Context::OnCompletedRequest( ...@@ -966,8 +963,8 @@ void WebURLLoaderImpl::Context::OnCompletedRequest(
if (status.error_code != net::OK) { if (status.error_code != net::OK) {
const WebURLError::HasCopyInCache has_copy_in_cache = const WebURLError::HasCopyInCache has_copy_in_cache =
response_was_cached_ ? WebURLError::HasCopyInCache::kTrue status.exists_in_cache ? WebURLError::HasCopyInCache::kTrue
: WebURLError::HasCopyInCache::kFalse; : WebURLError::HasCopyInCache::kFalse;
client_->DidFail( client_->DidFail(
status.cors_error_status status.cors_error_status
? WebURLError(*status.cors_error_status, has_copy_in_cache, url_) ? WebURLError(*status.cors_error_status, has_copy_in_cache, url_)
......
<html>
<head></head>
<body>
<img src="cachetime"/>
</body>
</html>
...@@ -282,6 +282,7 @@ IPC_STRUCT_TRAITS_END() ...@@ -282,6 +282,7 @@ IPC_STRUCT_TRAITS_END()
IPC_STRUCT_TRAITS_BEGIN(network::URLLoaderCompletionStatus) IPC_STRUCT_TRAITS_BEGIN(network::URLLoaderCompletionStatus)
IPC_STRUCT_TRAITS_MEMBER(error_code) IPC_STRUCT_TRAITS_MEMBER(error_code)
IPC_STRUCT_TRAITS_MEMBER(exists_in_cache)
IPC_STRUCT_TRAITS_MEMBER(completion_time) IPC_STRUCT_TRAITS_MEMBER(completion_time)
IPC_STRUCT_TRAITS_MEMBER(encoded_data_length) IPC_STRUCT_TRAITS_MEMBER(encoded_data_length)
IPC_STRUCT_TRAITS_MEMBER(encoded_body_length) IPC_STRUCT_TRAITS_MEMBER(encoded_body_length)
...@@ -358,7 +359,6 @@ IPC_STRUCT_TRAITS_BEGIN(network::ResourceResponseInfo) ...@@ -358,7 +359,6 @@ IPC_STRUCT_TRAITS_BEGIN(network::ResourceResponseInfo)
IPC_STRUCT_TRAITS_MEMBER(ct_policy_compliance) IPC_STRUCT_TRAITS_MEMBER(ct_policy_compliance)
IPC_STRUCT_TRAITS_MEMBER(is_legacy_symantec_cert) IPC_STRUCT_TRAITS_MEMBER(is_legacy_symantec_cert)
IPC_STRUCT_TRAITS_MEMBER(content_length) IPC_STRUCT_TRAITS_MEMBER(content_length)
IPC_STRUCT_TRAITS_MEMBER(was_cached)
IPC_STRUCT_TRAITS_MEMBER(encoded_data_length) IPC_STRUCT_TRAITS_MEMBER(encoded_data_length)
IPC_STRUCT_TRAITS_MEMBER(encoded_body_length) IPC_STRUCT_TRAITS_MEMBER(encoded_body_length)
IPC_STRUCT_TRAITS_MEMBER(appcache_id) IPC_STRUCT_TRAITS_MEMBER(appcache_id)
......
...@@ -21,7 +21,6 @@ scoped_refptr<ResourceResponse> ResourceResponse::DeepCopy() const { ...@@ -21,7 +21,6 @@ scoped_refptr<ResourceResponse> ResourceResponse::DeepCopy() const {
new_response->head.ct_policy_compliance = head.ct_policy_compliance; new_response->head.ct_policy_compliance = head.ct_policy_compliance;
new_response->head.is_legacy_symantec_cert = head.is_legacy_symantec_cert; new_response->head.is_legacy_symantec_cert = head.is_legacy_symantec_cert;
new_response->head.content_length = head.content_length; new_response->head.content_length = head.content_length;
new_response->head.was_cached = head.was_cached;
new_response->head.encoded_data_length = head.encoded_data_length; new_response->head.encoded_data_length = head.encoded_data_length;
new_response->head.encoded_body_length = head.encoded_body_length; new_response->head.encoded_body_length = head.encoded_body_length;
new_response->head.appcache_id = head.appcache_id; new_response->head.appcache_id = head.appcache_id;
......
...@@ -15,7 +15,6 @@ ResourceResponseInfo::ResourceResponseInfo() ...@@ -15,7 +15,6 @@ ResourceResponseInfo::ResourceResponseInfo()
content_length(-1), content_length(-1),
encoded_data_length(-1), encoded_data_length(-1),
encoded_body_length(-1), encoded_body_length(-1),
was_cached(false),
appcache_id(0), appcache_id(0),
was_fetched_via_spdy(false), was_fetched_via_spdy(false),
was_alpn_negotiated(false), was_alpn_negotiated(false),
......
...@@ -69,9 +69,6 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceResponseInfo { ...@@ -69,9 +69,6 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceResponseInfo {
// has been read to the end. // has been read to the end.
int64_t encoded_body_length; int64_t encoded_body_length;
// True if the response was fetched from the network cache.
bool was_cached;
// The appcache this response was loaded from, or kAppCacheNoCacheId. // The appcache this response was loaded from, or kAppCacheNoCacheId.
// TODO(rdsmith): Remove conceptual dependence on appcache. // TODO(rdsmith): Remove conceptual dependence on appcache.
int64_t appcache_id; int64_t appcache_id;
......
...@@ -26,6 +26,7 @@ URLLoaderCompletionStatus::~URLLoaderCompletionStatus() = default; ...@@ -26,6 +26,7 @@ URLLoaderCompletionStatus::~URLLoaderCompletionStatus() = default;
bool URLLoaderCompletionStatus::operator==( bool URLLoaderCompletionStatus::operator==(
const URLLoaderCompletionStatus& rhs) const { const URLLoaderCompletionStatus& rhs) const {
return error_code == rhs.error_code && return error_code == rhs.error_code &&
exists_in_cache == rhs.exists_in_cache &&
completion_time == rhs.completion_time && completion_time == rhs.completion_time &&
encoded_data_length == rhs.encoded_data_length && encoded_data_length == rhs.encoded_data_length &&
encoded_body_length == rhs.encoded_body_length && encoded_body_length == rhs.encoded_body_length &&
......
...@@ -36,6 +36,9 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) URLLoaderCompletionStatus { ...@@ -36,6 +36,9 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) URLLoaderCompletionStatus {
// The error code. ERR_FAILED is set for CORS errors. // The error code. ERR_FAILED is set for CORS errors.
int error_code = 0; int error_code = 0;
// A copy of the data requested exists in the cache.
bool exists_in_cache = false;
// Time the request completed. // Time the request completed.
base::TimeTicks completion_time; base::TimeTicks completion_time;
......
...@@ -47,7 +47,6 @@ void PopulateResourceResponse(net::URLRequest* request, ...@@ -47,7 +47,6 @@ void PopulateResourceResponse(net::URLRequest* request,
response->head.headers = request->response_headers(); response->head.headers = request->response_headers();
request->GetCharset(&response->head.charset); request->GetCharset(&response->head.charset);
response->head.content_length = request->GetExpectedContentSize(); response->head.content_length = request->GetExpectedContentSize();
response->head.was_cached = request->was_cached();
request->GetMimeType(&response->head.mime_type); request->GetMimeType(&response->head.mime_type);
net::HttpResponseInfo response_info = request->response_info(); net::HttpResponseInfo response_info = request->response_info();
response->head.was_fetched_via_spdy = response_info.was_fetched_via_spdy; response->head.was_fetched_via_spdy = response_info.was_fetched_via_spdy;
...@@ -689,6 +688,7 @@ void URLLoader::NotifyCompleted(int error_code) { ...@@ -689,6 +688,7 @@ void URLLoader::NotifyCompleted(int error_code) {
URLLoaderCompletionStatus status; URLLoaderCompletionStatus status;
status.error_code = error_code; status.error_code = error_code;
status.exists_in_cache = url_request_->response_info().was_cached;
status.completion_time = base::TimeTicks::Now(); status.completion_time = base::TimeTicks::Now();
status.encoded_data_length = url_request_->GetTotalReceivedBytes(); status.encoded_data_length = url_request_->GetTotalReceivedBytes();
status.encoded_body_length = url_request_->GetRawBodyBytes(); status.encoded_body_length = url_request_->GetRawBodyBytes();
......
...@@ -89,6 +89,7 @@ void URLLoaderFactory::CreateLoaderAndStart( ...@@ -89,6 +89,7 @@ void URLLoaderFactory::CreateLoaderAndStart(
if (client) { if (client) {
URLLoaderCompletionStatus status; URLLoaderCompletionStatus status;
status.error_code = net::ERR_INSUFFICIENT_RESOURCES; status.error_code = net::ERR_INSUFFICIENT_RESOURCES;
status.exists_in_cache = false;
status.completion_time = base::TimeTicks::Now(); status.completion_time = base::TimeTicks::Now();
client->OnComplete(status); client->OnComplete(status);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment