Migrate content::ResourceFetcher over mojom::URLLoader |
|||||||
Issue descriptionTo 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)
,
Aug 3 2017
,
Aug 8 2017
,
Aug 8 2017
I will take this.
,
Aug 8 2017
> 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.
,
Aug 8 2017
#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.
,
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
,
Aug 28 2017
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).
,
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
,
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
,
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
,
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
,
Sep 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3bd1cf5265913bf86b7a8fd74b9cfaa1e2f8dcb commit c3bd1cf5265913bf86b7a8fd74b9cfaa1e2f8dcb Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Tue Sep 05 06:32:47 2017 content::ResourceFetcher use OnceCallback and BindOnce Use OnceCallback for the completion callback. Bug: 752028 Change-Id: I7135cd13ffe0a2bc7b314abb30a2a143296278dd Reviewed-on: https://chromium-review.googlesource.com/646993 Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/heads/master@{#499576} [modify] https://crrev.com/c3bd1cf5265913bf86b7a8fd74b9cfaa1e2f8dcb/chrome/renderer/net/net_error_helper.cc [modify] https://crrev.com/c3bd1cf5265913bf86b7a8fd74b9cfaa1e2f8dcb/content/public/renderer/resource_fetcher.h [modify] https://crrev.com/c3bd1cf5265913bf86b7a8fd74b9cfaa1e2f8dcb/content/renderer/fetchers/resource_fetcher_impl.cc [modify] https://crrev.com/c3bd1cf5265913bf86b7a8fd74b9cfaa1e2f8dcb/content/renderer/fetchers/resource_fetcher_impl.h [modify] https://crrev.com/c3bd1cf5265913bf86b7a8fd74b9cfaa1e2f8dcb/content/renderer/mojo_context_state.cc
,
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
,
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
,
Sep 7 2017
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.
,
Sep 7 2017
,
Oct 17 2017
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by kinuko@chromium.org
, Aug 3 2017