Skip to content

Commit

Permalink
Don't use ResourceType for URLLoaderThrottleProvider
Browse files Browse the repository at this point in the history
ResourceType in (Web)URLRequest is now deprecated.

Use network::mojom::RequestDestination instead when it's available,
or use specific flag (e.g. is_main_frame) it's enough.
https://fetch.spec.whatwg.org/#concept-request-destination

Bug: 960143, 1045925
Change-Id: I9129b19a8a0039cdc9d939509a2b5132325ca5ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2041982
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739641}
  • Loading branch information
kinu authored and Commit Bot committed Feb 8, 2020
1 parent 0510a18 commit 881484e
Show file tree
Hide file tree
Showing 22 changed files with 69 additions and 75 deletions.
6 changes: 3 additions & 3 deletions android_webview/renderer/aw_url_loader_throttle_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ AwURLLoaderThrottleProvider::~AwURLLoaderThrottleProvider() {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
AwURLLoaderThrottleProvider::CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) {
const blink::WebURLRequest& request) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;

// Some throttles have already been added in the browser for frame resources.
// Don't add them for frame requests.
bool is_frame_resource = blink::IsResourceTypeFrame(resource_type);
bool is_frame_resource =
blink::IsRequestDestinationFrame(request.GetRequestDestination());

DCHECK(!is_frame_resource ||
type_ == content::URLLoaderThrottleProviderType::kFrame);
Expand Down
3 changes: 1 addition & 2 deletions android_webview/renderer/aw_url_loader_throttle_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ class AwURLLoaderThrottleProvider : public content::URLLoaderThrottleProvider {
std::unique_ptr<content::URLLoaderThrottleProvider> Clone() override;
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) override;
const blink::WebURLRequest& request) override;
void SetOnline(bool is_online) override;

private:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
#include "net/base/escape.h"
#include "net/base/load_flags.h"
#include "net/http/http_status_code.h"
#include "services/network/public/mojom/fetch_api.mojom-shared.h"
#include "services/network/public/mojom/url_response_head.mojom.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/mojom/loader/resource_load_info.mojom-shared.h"
#include "third_party/blink/public/platform/web_url.h"
#include "third_party/blink/public/platform/web_url_request.h"

Expand All @@ -28,10 +28,10 @@ namespace subresource_redirect {
std::unique_ptr<SubresourceRedirectURLLoaderThrottle>
SubresourceRedirectURLLoaderThrottle::MaybeCreateThrottle(
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type,
int render_frame_id) {
if (base::FeatureList::IsEnabled(blink::features::kSubresourceRedirect) &&
resource_type == blink::mojom::ResourceType::kImage &&
request.GetRequestDestination() ==
network::mojom::RequestDestination::kImage &&
(request.GetPreviewsState() &
blink::WebURLRequest::kSubresourceRedirectOn) &&
request.Url().ProtocolIs(url::kHttpsScheme)) {
Expand All @@ -54,8 +54,7 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
network::ResourceRequest* request,
bool* defer) {
DCHECK(base::FeatureList::IsEnabled(blink::features::kSubresourceRedirect));
DCHECK_EQ(request->resource_type,
static_cast<int>(blink::mojom::ResourceType::kImage));
DCHECK_EQ(request->destination, network::mojom::RequestDestination::kImage);
DCHECK(request->previews_state &
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON);
DCHECK(request->url.SchemeIs(url::kHttpsScheme));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "base/macros.h"
#include "third_party/blink/public/common/loader/url_loader_throttle.h"
#include "third_party/blink/public/mojom/loader/resource_load_info.mojom-shared.h"

namespace blink {
class WebURLRequest;
Expand All @@ -23,7 +22,6 @@ class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
public:
static std::unique_ptr<SubresourceRedirectURLLoaderThrottle>
MaybeCreateThrottle(const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type,
int render_frame_id);

~SubresourceRedirectURLLoaderThrottle() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
#include "chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h"
#include "chrome/renderer/subresource_redirect/subresource_redirect_util.h"
#include "content/public/common/previews_state.h"
#include "services/network/public/mojom/fetch_api.mojom-shared.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/mojom/loader/resource_load_info.mojom-shared.h"
#include "third_party/blink/public/platform/web_url.h"
#include "third_party/blink/public/platform/web_url_request.h"

Expand Down Expand Up @@ -41,14 +41,15 @@ namespace {
std::unique_ptr<SubresourceRedirectURLLoaderThrottle>
CreateSubresourceRedirectURLLoaderThrottle(
const GURL& url,
blink::mojom::ResourceType resource_type,
network::mojom::RequestDestination request_destination,
int previews_state,
const std::vector<std::string>& public_image_urls) {
blink::WebURLRequest request;
request.SetUrl(url);
request.SetPreviewsState(previews_state);
request.SetRequestDestination(request_destination);
DCHECK(SubresourceRedirectURLLoaderThrottle::MaybeCreateThrottle(
request, resource_type, kRenderFrameID)
request, kRenderFrameID)
.get() != nullptr);

return std::make_unique<TestSubresourceRedirectURLLoaderThrottle>(
Expand All @@ -58,32 +59,32 @@ CreateSubresourceRedirectURLLoaderThrottle(
TEST(SubresourceRedirectURLLoaderThrottleTest, TestMaybeCreateThrottle) {
struct TestCase {
bool is_subresource_redirect_feature_enabled;
blink::mojom::ResourceType resource_type;
network::mojom::RequestDestination destination;
int previews_state;
const std::string url;
bool expected_is_throttle_created;
};

const TestCase kTestCases[]{
{true, blink::mojom::ResourceType::kImage,
{true, network::mojom::RequestDestination::kImage,
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
"https://www.test.com/test.jpg", true},
{true, blink::mojom::ResourceType::kImage,
{true, network::mojom::RequestDestination::kImage,
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON |
content::PreviewsTypes::LAZY_IMAGE_AUTO_RELOAD,
"https://www.test.com/test.jpg", true},

// Failure cases
{false, blink::mojom::ResourceType::kImage,
{false, network::mojom::RequestDestination::kImage,
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
"https://www.test.com/test.jpg", false},
{true, blink::mojom::ResourceType::kScript,
{true, network::mojom::RequestDestination::kScript,
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
"https://www.test.com/test.jpg", false},
{true, blink::mojom::ResourceType::kImage,
{true, network::mojom::RequestDestination::kImage,
content::PreviewsTypes::LAZY_IMAGE_AUTO_RELOAD,
"https://www.test.com/test.jpg", false},
{true, blink::mojom::ResourceType::kImage,
{true, network::mojom::RequestDestination::kImage,
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
"http://www.test.com/test.jpg", false},
};
Expand All @@ -103,9 +104,10 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, TestMaybeCreateThrottle) {
blink::WebURLRequest request;
request.SetPreviewsState(test_case.previews_state);
request.SetUrl(GURL(test_case.url));
request.SetRequestDestination(test_case.destination);
EXPECT_EQ(test_case.expected_is_throttle_created,
SubresourceRedirectURLLoaderThrottle::MaybeCreateThrottle(
request, test_case.resource_type, kRenderFrameID) != nullptr);
request, kRenderFrameID) != nullptr);
}
}

Expand Down Expand Up @@ -151,16 +153,15 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, TestGetSubresourceURL) {

for (const TestCase& test_case : kTestCases) {
auto throttle = CreateSubresourceRedirectURLLoaderThrottle(
test_case.original_url, blink::mojom::ResourceType::kImage,
test_case.original_url, network::mojom::RequestDestination::kImage,
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
{"https://www.test.com/public_img.jpg",
"https://www.test.com/public_img.jpg#anchor",
"https://www.test.com/public_img.jpg?public_arg1=bar&public_arg2"});
network::ResourceRequest request;
request.url = test_case.original_url;
request.resource_type =
static_cast<int>(blink::mojom::ResourceType::kImage);
request.previews_state = content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON;
request.destination = network::mojom::RequestDestination::kImage;
bool defer = true;
throttle->WillStartRequest(&request, &defer);

Expand All @@ -181,12 +182,13 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, DeferOverridenToFalse) {
{});

auto throttle = CreateSubresourceRedirectURLLoaderThrottle(
GURL("https://www.test.com/test.jpg"), blink::mojom::ResourceType::kImage,
GURL("https://www.test.com/test.jpg"),
network::mojom::RequestDestination::kImage,
content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON,
{"https://www.test.com/test.jpg"});
network::ResourceRequest request;
request.url = GURL("https://www.test.com/test.jpg");
request.resource_type = static_cast<int>(blink::mojom::ResourceType::kImage);
request.destination = network::mojom::RequestDestination::kImage;
request.previews_state = content::PreviewsTypes::SUBRESOURCE_REDIRECT_ON;
bool defer = true;

Expand Down
13 changes: 8 additions & 5 deletions chrome/renderer/url_loader_throttle_provider_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -134,15 +134,18 @@ URLLoaderThrottleProviderImpl::Clone() {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
URLLoaderThrottleProviderImpl::CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) {
const blink::WebURLRequest& request) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;

const network::mojom::RequestDestination request_destination =
request.GetRequestDestination();

// Some throttles have already been added in the browser for frame resources.
// Don't add them for frame requests.
bool is_frame_resource = blink::IsResourceTypeFrame(resource_type);
bool is_frame_resource =
blink::IsRequestDestinationFrame(request_destination);

DCHECK(!is_frame_resource ||
type_ == content::URLLoaderThrottleProviderType::kFrame);
Expand Down Expand Up @@ -180,7 +183,7 @@ URLLoaderThrottleProviderImpl::CreateThrottles(

#if BUILDFLAG(ENABLE_EXTENSIONS)
if (type_ == content::URLLoaderThrottleProviderType::kFrame &&
resource_type == blink::mojom::ResourceType::kObject) {
request_destination == network::mojom::RequestDestination::kObject) {
content::RenderFrame* render_frame =
content::RenderFrame::FromRoutingID(render_frame_id);
auto mime_handlers =
Expand Down Expand Up @@ -222,7 +225,7 @@ URLLoaderThrottleProviderImpl::CreateThrottles(
#endif // defined(OS_CHROMEOS)

auto throttle = subresource_redirect::SubresourceRedirectURLLoaderThrottle::
MaybeCreateThrottle(request, resource_type, render_frame_id);
MaybeCreateThrottle(request, render_frame_id);
if (throttle)
throttles.push_back(std::move(throttle));

Expand Down
3 changes: 1 addition & 2 deletions chrome/renderer/url_loader_throttle_provider_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ class URLLoaderThrottleProviderImpl
std::unique_ptr<content::URLLoaderThrottleProvider> Clone() override;
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) override;
const blink::WebURLRequest& request) override;
void SetOnline(bool is_online) override;

private:
Expand Down
3 changes: 1 addition & 2 deletions chromecast/renderer/cast_url_loader_throttle_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ CastURLLoaderThrottleProvider::Clone() {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
CastURLLoaderThrottleProvider::CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) {
const blink::WebURLRequest& request) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;
Expand Down
3 changes: 1 addition & 2 deletions chromecast/renderer/cast_url_loader_throttle_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ class CastURLLoaderThrottleProvider
std::unique_ptr<content::URLLoaderThrottleProvider> Clone() override;
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) override;
const blink::WebURLRequest& request) override;
void SetOnline(bool is_online) override;

private:
Expand Down
4 changes: 1 addition & 3 deletions content/public/renderer/url_loader_throttle_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ class CONTENT_EXPORT URLLoaderThrottleProvider {
// requests from shared or service workers, this is called on the worker
// thread and |render_frame_id| will be set to MSG_ROUTING_NONE.
virtual std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
CreateThrottles(int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) = 0;
CreateThrottles(int render_frame_id, const blink::WebURLRequest& request) = 0;

// Set the network status online state as specified in |is_online|.
virtual void SetOnline(bool is_online) = 0;
Expand Down
4 changes: 2 additions & 2 deletions content/renderer/loader/web_worker_fetch_context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,8 @@ void WebWorkerFetchContextImpl::WillSendRequest(blink::WebURLRequest& request) {
extra_data->set_render_frame_id(ancestor_frame_id_);
extra_data->set_frame_request_blocker(frame_request_blocker_);
if (throttle_provider_) {
extra_data->set_url_loader_throttles(throttle_provider_->CreateThrottles(
ancestor_frame_id_, request, WebURLRequestToResourceType(request)));
extra_data->set_url_loader_throttles(
throttle_provider_->CreateThrottles(ancestor_frame_id_, request));
}
if (response_override_) {
using RequestContextType = blink::mojom::RequestContextType;
Expand Down
18 changes: 7 additions & 11 deletions content/renderer/render_frame_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4814,13 +4814,13 @@ void RenderFrameImpl::OnMainFrameDocumentIntersectionChanged(
void RenderFrameImpl::WillSendRequest(blink::WebURLRequest& request) {
// This method is called for subresources, while transition type is
// a navigation concept. We pass ui::PAGE_TRANSITION_LINK as default one.
WillSendRequestInternal(request, WebURLRequestToResourceType(request),
WillSendRequestInternal(request, /*for_main_frame=*/false,
ui::PAGE_TRANSITION_LINK);
}

void RenderFrameImpl::WillSendRequestInternal(
blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type,
bool for_main_frame,
ui::PageTransition transition_type) {
if (render_view_->renderer_preferences_.enable_do_not_track) {
request.SetHttpHeaderField(blink::WebString::FromUTF8(kDoNotTrackHeader),
Expand Down Expand Up @@ -4879,17 +4879,16 @@ void RenderFrameImpl::WillSendRequestInternal(
extra_data->set_allow_cross_origin_auth_prompt(
render_view_->renderer_preferences().allow_cross_origin_auth_prompt);

request.SetDownloadToNetworkCacheOnly(
is_for_no_state_prefetch &&
resource_type != blink::mojom::ResourceType::kMainFrame);
request.SetDownloadToNetworkCacheOnly(is_for_no_state_prefetch &&
!for_main_frame);

// The RenderThreadImpl or its URLLoaderThrottleProvider member may not be
// valid in some tests.
RenderThreadImpl* render_thread = RenderThreadImpl::current();
if (render_thread && render_thread->url_loader_throttle_provider()) {
extra_data->set_url_loader_throttles(
render_thread->url_loader_throttle_provider()->CreateThrottles(
routing_id_, request, resource_type));
routing_id_, request));
}

// This is an instance where we embed a copy of the routing id
Expand Down Expand Up @@ -6341,11 +6340,8 @@ void RenderFrameImpl::BeginNavigationInternal(
// TODO(clamy): Apply devtools override.
// TODO(clamy): Make sure that navigation requests are not modified somewhere
// else in blink.
WillSendRequestInternal(request,
frame_->Parent()
? blink::mojom::ResourceType::kSubFrame
: blink::mojom::ResourceType::kMainFrame,
transition_type);
bool for_main_frame = !frame_->Parent();
WillSendRequestInternal(request, for_main_frame, transition_type);

if (!info->url_request.GetExtraData())
info->url_request.SetExtraData(base::MakeRefCounted<RequestExtraData>());
Expand Down
2 changes: 1 addition & 1 deletion content/renderer/render_frame_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1175,7 +1175,7 @@ class CONTENT_EXPORT RenderFrameImpl

// |transition_type| corresponds to the document which triggered this request.
void WillSendRequestInternal(blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type,
bool for_main_frame,
ui::PageTransition transition_type);

// Returns the URL being loaded by the |frame_|'s request.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ void ServiceWorkerFetchContextImpl::WillSendRequest(
// worker scripts.
script_url_to_skip_throttling_ = GURL();
} else if (throttle_provider_) {
extra_data->set_url_loader_throttles(throttle_provider_->CreateThrottles(
MSG_ROUTING_NONE, request, WebURLRequestToResourceType(request)));
extra_data->set_url_loader_throttles(
throttle_provider_->CreateThrottles(MSG_ROUTING_NONE, request));
}

request.SetExtraData(std::move(extra_data));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ class ServiceWorkerFetchContextImplTest : public testing::Test {

std::vector<std::unique_ptr<blink::URLLoaderThrottle>> CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) override {
const blink::WebURLRequest& request) override {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;
throttles.emplace_back(std::make_unique<FakeURLLoaderThrottle>());
return throttles;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ WebEngineURLLoaderThrottleProvider::Clone() {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
WebEngineURLLoaderThrottleProvider::CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) {
const blink::WebURLRequest& request) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ class WebEngineURLLoaderThrottleProvider
std::unique_ptr<content::URLLoaderThrottleProvider> Clone() override;
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> CreateThrottles(
int render_frame_id,
const blink::WebURLRequest& request,
blink::mojom::ResourceType resource_type) override;
const blink::WebURLRequest& request) override;
void SetOnline(bool is_online) override;

private:
Expand Down
6 changes: 6 additions & 0 deletions third_party/blink/common/loader/resource_type_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,10 @@ bool IsResourceTypeFrame(blink::mojom::ResourceType type) {
type == blink::mojom::ResourceType::kSubFrame;
}

bool IsRequestDestinationFrame(network::mojom::RequestDestination destination) {
return destination == network::mojom::RequestDestination::kDocument ||
destination == network::mojom::RequestDestination::kFrame ||
destination == network::mojom::RequestDestination::kIframe;
}

} // namespace blink
Loading

0 comments on commit 881484e

Please sign in to comment.