New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 752028 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Task

Blocking:
issue 775402



Sign in to add a comment

Migrate content::ResourceFetcher over mojom::URLLoader

Project Member Reported by kinuko@chromium.org, Aug 3 2017

Issue description

To add a bit of background, there're two content::ResourceFetcher's:

1. ResourceFetcher(Impl) -- for relatively 'raw' resource loading, looks to be somewhat similar to net::URLFetcher in browser process

2. AssociatedResourceFetcher(Impl) -- backed by DocumentThreadableLoader and also supports CORS / CSP etc.

Eventually both could probably migrate over URLLoader (if issue 736308 is done), but this issue is more about the former one.

A brief plan is to make this one (or consumers of this) work directly on top of mojom::URLLoader:

- It'll make Onion Soup a simple file moving once mojom::URLLoader moves into /services (where Blink can depend on)

- We might be able to extend the approach to also turn net::URLFetcher consumers work on top of mojom::URLLoader (for S13N)
 
Cc: tyoshino@chromium.org
Components: Blink>Network
Description: Show this description
Labels: -Type-Bug OS-All Type-Task
Owner: toyoshim@chromium.org
Status: Assigned (was: Available)
I will take this.
> A brief plan is to make this one (or consumers of this) work directly on top of mojom::URLLoader:

Slight point but I wanted to say offline today was the following:

ResourceFetcher's functionality are:
(1) a helper to easily set up WebURLRequest using std::* values
(2) consolidate all the WebURLLoaderClient callbacks

Since (1) is making the class messed with DCHECKs without providing any real value, so tried to remove them in https://chromium-review.googlesource.com/c/527953/ but suspended because of inlining (1) would increase the code at the users.

IIUC, switching it to mojom::URLLoader based would eliminate (1) and unblocks the CL. That's nice.

#5- I see, thanks for giving the detail. Yeah I think making it work with mojom::URLLoader will make it closer to what you tried to do.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5aab5c2b9d9e85ffccbdb3571e9f7d218e95601

commit e5aab5c2b9d9e85ffccbdb3571e9f7d218e95601
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Fri Aug 25 04:21:57 2017

Relocate resource_fetcher_browsertest.cc into fetchers/ directory

Since the test target is in fetchers/ directory, move the test
file to the same directory.

Some EXPECT_EQ arguments are swapped because the first argument
should be the expectation so that failed error messages are readable.
'git cl format' is also applied to make the presubmit check happy.

In addition to mechanical changes, this patch also adds one additional
test case that an existing caller actually relies on. This will help
to avoid regression in the following refactoring.

This patch is the first step to remove blink::WebURLLoader calls
from content::ResourceFetcher. Following changes will actually
replace it with mojom::URLLoader, and modify the content interface
to be simple enough.

Bug:  752028 
Change-Id: Ic59857c5bb5a84476547016d066db8e46e9474bb
Reviewed-on: https://chromium-review.googlesource.com/628396
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497322}
[rename] https://crrev.com/e5aab5c2b9d9e85ffccbdb3571e9f7d218e95601/content/renderer/fetchers/resource_fetcher_browsertest.cc
[modify] https://crrev.com/e5aab5c2b9d9e85ffccbdb3571e9f7d218e95601/content/test/BUILD.gn

Status: Started (was: Assigned)
Note for the next step.

1. Submit https://chromium-review.googlesource.com/c/chromium/src/+/635703 that replaces content::ResourceFetcher internal implementation to use mojom::URLLoader.

2. Move NetworkTrafficAnnotationTag outside content::ResourceFethcer so that each call can set it correctly, and the interface to be more similar to SimpleURLLoader.

3. Change content-friendly callers to use SimpleURLLoader instead of content::ResourceFetcher.

4. Change blink-friendly callers to use platform/loader/fetch infra, or introduce a thin-wrapper class that allows to call SimpleURLLoader with blink-friendly types.

While I'm doing this migration, I will ping owners of each caller to discuss where each caller code will go, and to understand what each caller actually does, (and to have a right NetworkTrafficAnnotationTag).
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ece8de8da5c2986d4f99979b1f090fd1ed8e6c1

commit 6ece8de8da5c2986d4f99979b1f090fd1ed8e6c1
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Wed Aug 30 11:36:48 2017

ResourceFethcerImpl: use mojom::URLLoader instead of blink::WebURLLoader

This patch reimplements ResourceFetchImpl with mojom::URLLoader
instead of blink::WebURLLoader.

Bug:  752028 
Change-Id: I57373386a3762ae867519a95314e8099b091b57d
Reviewed-on: https://chromium-review.googlesource.com/635703
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498421}
[modify] https://crrev.com/6ece8de8da5c2986d4f99979b1f090fd1ed8e6c1/content/child/web_url_request_util.cc
[modify] https://crrev.com/6ece8de8da5c2986d4f99979b1f090fd1ed8e6c1/content/child/web_url_request_util.h
[modify] https://crrev.com/6ece8de8da5c2986d4f99979b1f090fd1ed8e6c1/content/renderer/fetchers/resource_fetcher_impl.cc
[modify] https://crrev.com/6ece8de8da5c2986d4f99979b1f090fd1ed8e6c1/content/renderer/fetchers/resource_fetcher_impl.h

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b75ab96a17598c6159609d6a7cdc36f7849df48

commit 4b75ab96a17598c6159609d6a7cdc36f7849df48
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Wed Aug 30 16:49:55 2017

make ChildURLLoaderFactoryGetter be in content/public

mojom::URLLoaderFactory is going to be a standard way to make a network
request, but there is no way to obtain mojom::URLLoaderFactory instance
in chrome/{child,renderer} today.

This patch moves ChildURLLoaderFactoryGetter into content/public/child,
and promotes RenderFrameImpl::GetDefaultURLLoaderFactoryGetter interface
to public interface of RenderFrame.

Bug:  752028 
Change-Id: I7abecd06a534e8aad0d402647471c0556583abb2
Reviewed-on: https://chromium-review.googlesource.com/641071
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498500}
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/BUILD.gn
[delete] https://crrev.com/ed7971c95c05a5dbe6514e464643b08e3b3c2341/content/child/child_url_loader_factory_getter.h
[rename] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/child_url_loader_factory_getter_impl.cc
[add] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/child_url_loader_factory_getter_impl.h
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/service_worker/service_worker_network_provider.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/service_worker/service_worker_provider_context.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/service_worker/service_worker_provider_context.h
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/service_worker/service_worker_provider_context_unittest.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/child/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/public/child/BUILD.gn
[add] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/public/child/child_url_loader_factory_getter.h
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/public/renderer/render_frame.h
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/fetchers/resource_fetcher_impl.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/render_frame_impl.h
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/4b75ab96a17598c6159609d6a7cdc36f7849df48/content/renderer/shared_worker/embedded_shared_worker_stub.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6448dc95030b7b0687fa67aa51bd8825073042bf

commit 6448dc95030b7b0687fa67aa51bd8825073042bf
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Thu Aug 31 07:20:48 2017

Add alternative ResourceFetcher::Start that will replace the old one

This is a transitional change to have old and new versions of
ResourceFetcher::Start() that take different arguments.

New version takes mojom::URLLoaderFactory,
net::NetworkTrafficAnnotationTag, and maximum data size.

There are two callers for the old ResourceFetcher::Start(), and I will
make two separate changes to migrate each caller to use the new one.

Bug:  752028 
Change-Id: Ibf14f258617fd35a78e82b10007a6a5d5062aab5
Reviewed-on: https://chromium-review.googlesource.com/641177
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498774}
[modify] https://crrev.com/6448dc95030b7b0687fa67aa51bd8825073042bf/content/public/renderer/resource_fetcher.h
[modify] https://crrev.com/6448dc95030b7b0687fa67aa51bd8825073042bf/content/renderer/fetchers/resource_fetcher_impl.cc
[modify] https://crrev.com/6448dc95030b7b0687fa67aa51bd8825073042bf/content/renderer/fetchers/resource_fetcher_impl.h

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2796e7e3119ec75d1a556394bcafa05ba6fa40bc

commit 2796e7e3119ec75d1a556394bcafa05ba6fa40bc
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Thu Aug 31 08:13:01 2017

MojoContextState: use new ResourceFetcher::Start interface

Use new ResourceFetcher::Start interface to specify
mojom::URLLoaderFactory and net::NetworkTrafficAnnotationTag
correctly.

Bug:  752028 
Change-Id: I9e4eef2a0af22c75264e826f797ef4a6784e5c04
Reviewed-on: https://chromium-review.googlesource.com/643027
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yuzhu Shen <yzshen@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498780}
[modify] https://crrev.com/2796e7e3119ec75d1a556394bcafa05ba6fa40bc/content/renderer/mojo_context_state.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Sep 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e8074275acad6f5ed333a44a9b9fe09def7631a

commit 3e8074275acad6f5ed333a44a9b9fe09def7631a
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Tue Sep 05 09:47:11 2017

NetErrorHandler: use new ResourceFetcher::Start interface

Use new ResourceFetcher::Start interface to specify
mojom::URLLoaderFactory and net::NetworkTrafficAnnotationTag
correctly.

After this change, the old Start interface will be removed shortly.

Bug:  752028 
Change-Id: Ic7892cfad6f4c16da88bec73ee6d960248c27c5a
Reviewed-on: https://chromium-review.googlesource.com/644652
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499592}
[modify] https://crrev.com/3e8074275acad6f5ed333a44a9b9fe09def7631a/chrome/renderer/net/net_error_helper.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Sep 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/935fae24e75ed434f7dce7cf9c92abbfece99844

commit 935fae24e75ed434f7dce7cf9c92abbfece99844
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Wed Sep 06 06:58:33 2017

content::ResourceFetcher: deprecate old Start interface

Now that all users for the old interface are migrated to use the new
one, let's remove the old interface to enforce setting a right traffic
annotation for each caller.

Bug:  752028 
Change-Id: I7cd0bad8dea1509255bac499fecbb94426fd8eb4
Reviewed-on: https://chromium-review.googlesource.com/645410
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499895}
[modify] https://crrev.com/935fae24e75ed434f7dce7cf9c92abbfece99844/content/public/renderer/resource_fetcher.h
[modify] https://crrev.com/935fae24e75ed434f7dce7cf9c92abbfece99844/content/renderer/fetchers/resource_fetcher_browsertest.cc
[modify] https://crrev.com/935fae24e75ed434f7dce7cf9c92abbfece99844/content/renderer/fetchers/resource_fetcher_impl.cc
[modify] https://crrev.com/935fae24e75ed434f7dce7cf9c92abbfece99844/content/renderer/fetchers/resource_fetcher_impl.h

Oops, the last cleanup was submitted with a wrong bug id.
You can find the commit log at https://bugs.chromium.org/p/chromium/issues/detail?id=760581&desc=2#c6

We could replace its internal implementation with SimpleURLLoader, but it could be done later separately.

I would close this as Fixed.
Status: Fixed (was: Started)
Blocking: 775402

Sign in to add a comment