Issue metadata
Sign in to add a comment
|
Make a URLFetcher replacement that wraps a URLLoader |
||||||||||||||||||||||
Issue descriptionUsing a content::mojom::URLLoader requires a lot of Mojo magic (Particularly around the stream for the body), and is easy to get wrong - managing two sources of input, both of which can fail independently of another, for instance, is easy to get wrong, and it's not likely that all ~180 URLFetcher consumers will unit test every possible ordering of events, or handle them correctly. So we should build a URLFetcher replacement that handles all the Mojo stuff for the URLLoader API. It can expose the Request/Response objects, so we don't need nearly as many setters/getters as URLFetcher itself uses. I think we should also use this as a chance to discourage people from using unbounded in-memory receive buffers (Something that URLFetcher does by default), and making it easier to handle errors (By default, don't read/return response bodies on 4xx/5xx errors, but make an option to do so).
,
Jul 25 2017
Averse to adding another unit test binary, rather.
,
Aug 2 2017
,
Aug 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d5132345b80cc749867319d473dadfe14bd8da8a commit d5132345b80cc749867319d473dadfe14bd8da8a Author: Matt Menke <mmenke@chromium.org> Date: Tue Aug 22 10:45:53 2017 Add SimpleURLLoader to make using URLLoader simpler. The class allows downloading a URL to a string using a URLLoader, abstracting out most of the complexities of the URLLoader API. Bug: 746977 Change-Id: Iec5d52a1028d81b803e6fe2aac978a8303a6fd2a Reviewed-on: https://chromium-review.googlesource.com/600630 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#496275} [modify] https://crrev.com/d5132345b80cc749867319d473dadfe14bd8da8a/content/public/common/BUILD.gn [modify] https://crrev.com/d5132345b80cc749867319d473dadfe14bd8da8a/content/public/common/DEPS [add] https://crrev.com/d5132345b80cc749867319d473dadfe14bd8da8a/content/public/common/simple_url_loader.cc [add] https://crrev.com/d5132345b80cc749867319d473dadfe14bd8da8a/content/public/common/simple_url_loader.h [add] https://crrev.com/d5132345b80cc749867319d473dadfe14bd8da8a/content/public/common/simple_url_loader_unittest.cc [modify] https://crrev.com/d5132345b80cc749867319d473dadfe14bd8da8a/content/test/BUILD.gn
,
Sep 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd4f2f7b594a0cc0d5ff897ef5fd789f806fb6f3 commit dd4f2f7b594a0cc0d5ff897ef5fd789f806fb6f3 Author: Matt Menke <mmenke@chromium.org> Date: Mon Sep 25 19:35:13 2017 SimpleURLLoader: Split out reading from body pipe to a separate class. This CL splits out SimpleURLLoader's code to read from the body pipe out of SaveToStringBodyHandler and into a separate class. This will allow the body to be read off-thread when downloading a URL to disk. This CL also does a couple other things in preparation to support saving URLs to disk: It adds a SequenceChecker to SimpleURLLoaderImpl, and slightly modifies the API. Bug: 746977 Change-Id: Ic76e9da1d70b6951797fca293d69c270ddf989f1 Reviewed-on: https://chromium-review.googlesource.com/669360 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#504130} [modify] https://crrev.com/dd4f2f7b594a0cc0d5ff897ef5fd789f806fb6f3/content/public/common/simple_url_loader.cc [modify] https://crrev.com/dd4f2f7b594a0cc0d5ff897ef5fd789f806fb6f3/content/public/common/simple_url_loader.h
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/82271d290f29adb4d00e9b233fa2a4ebc8803a29 commit 82271d290f29adb4d00e9b233fa2a4ebc8803a29 Author: Matt Menke <mmenke@chromium.org> Date: Thu Sep 28 03:11:27 2017 SimpleURLLoader: Add download to file support. The file is written to on another thread, and the data pipe is also read from off the consumer's thread, so this should be save to use on named threads without causing significant jank. Bug: 746977 Change-Id: I8e38945a569e4cd97ff4381ecf43acc73c731cea Reviewed-on: https://chromium-review.googlesource.com/673043 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#504880} [modify] https://crrev.com/82271d290f29adb4d00e9b233fa2a4ebc8803a29/content/public/common/simple_url_loader.cc [modify] https://crrev.com/82271d290f29adb4d00e9b233fa2a4ebc8803a29/content/public/common/simple_url_loader.h [modify] https://crrev.com/82271d290f29adb4d00e9b233fa2a4ebc8803a29/content/public/common/simple_url_loader_unittest.cc
,
Oct 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0b753000b9037734303d22155df483228a401545 commit 0b753000b9037734303d22155df483228a401545 Author: Matt Menke <mmenke@chromium.org> Date: Wed Oct 18 01:14:15 2017 SimpleURLLoader: Add retry support on 5xx and network errors. These are the cases in which URLFetcher supports retries. Unlike URLFetcher, there's a single method to enable both types of retries, and a single counter. Unlike URLFetcher, this does not support backoff via the ThrottleManager (Something only used in the service process). We may want to implement something simpler, or just let the service process consumers themselves deal with backoff. Bug: 746977 Change-Id: I31c81652176856231289ad88d082713bad6df61d Reviewed-on: https://chromium-review.googlesource.com/701215 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#509642} [modify] https://crrev.com/0b753000b9037734303d22155df483228a401545/content/public/common/simple_url_loader.cc [modify] https://crrev.com/0b753000b9037734303d22155df483228a401545/content/public/common/simple_url_loader.h [modify] https://crrev.com/0b753000b9037734303d22155df483228a401545/content/public/common/simple_url_loader_unittest.cc
,
Oct 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cfd3cf322964d3f39df4b9513ac43f24635e4f5 commit 4cfd3cf322964d3f39df4b9513ac43f24635e4f5 Author: Matt Menke <mmenke@chromium.org> Date: Wed Oct 18 03:02:34 2017 Add download to temp file support to SimpleURLLoader. Bug: 746977 Change-Id: Ib0439337359486f1523e85963d8549fbae58ee08 Reviewed-on: https://chromium-review.googlesource.com/705695 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#509669} [modify] https://crrev.com/4cfd3cf322964d3f39df4b9513ac43f24635e4f5/content/public/common/simple_url_loader.cc [modify] https://crrev.com/4cfd3cf322964d3f39df4b9513ac43f24635e4f5/content/public/common/simple_url_loader.h [modify] https://crrev.com/4cfd3cf322964d3f39df4b9513ac43f24635e4f5/content/public/common/simple_url_loader_unittest.cc
,
Oct 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b8ace9e069420b505e2090d936c5dffdc30620e commit 6b8ace9e069420b505e2090d936c5dffdc30620e Author: Matt Menke <mmenke@chromium.org> Date: Wed Oct 18 15:45:00 2017 Add redirect interception support to SimpleURLLoader. This lets consumers cancel a request (by deleting the SimpleURLLoader) on redirects. Bug: 746977 Change-Id: I712f83fec2e81922785d7cde9c3fff686f63e3a4 Reviewed-on: https://chromium-review.googlesource.com/721979 Reviewed-by: Randy Smith <rdsmith@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#509777} [modify] https://crrev.com/6b8ace9e069420b505e2090d936c5dffdc30620e/content/public/common/simple_url_loader.cc [modify] https://crrev.com/6b8ace9e069420b505e2090d936c5dffdc30620e/content/public/common/simple_url_loader.h [modify] https://crrev.com/6b8ace9e069420b505e2090d936c5dffdc30620e/content/public/common/simple_url_loader_unittest.cc
,
Oct 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/507fbef7137560c7ab07594246d3c627c7f1beda commit 507fbef7137560c7ab07594246d3c627c7f1beda Author: Matt Menke <mmenke@chromium.org> Date: Wed Oct 18 16:53:28 2017 SimpleURLLoader: Add tests for when using disconnected URLLoaderFactory Tests cover both the disconnected before initial load and disconnected before retry cases. These cases shouldn't require any extra code to handle, but it's good to have test coverage for them (And testing these cases unearthed a Mojo bug). Also refactor the tests using MockURLLoaderFactory so that the MockURLLoaderFactory doesn't re-entrantly call into the calling SimpleURLLoader. This may not actually be needed for the new tests to work, but it makes for healthier tests. Bug: 746977 Change-Id: I747ae586ebd0b6bae867ec912d6f32f3002420f4 Reviewed-on: https://chromium-review.googlesource.com/723167 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#509794} [modify] https://crrev.com/507fbef7137560c7ab07594246d3c627c7f1beda/content/public/common/simple_url_loader_unittest.cc
,
Nov 7 2017
Apologies, applied the wrong component in bulk.
,
Nov 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75ef3e360ead4ff096fc11722d5950d53518c34a commit 75ef3e360ead4ff096fc11722d5950d53518c34a Author: Matt Menke <mmenke@chromium.org> Date: Tue Nov 21 23:38:32 2017 Refactor SimpleURLLoader API. Make it take in a unique_ptr<content::ResourceRequest>, to avoid having to copy it on retry, or when making a delayed request. Also make the constructor take the ResourceRequest and NetworkAnnotation as parameters, to simplify things a little. TBR=jdufault@chromium.org Bug: 746977 Change-Id: Ic0e2a573c8b8601e2b1ab9a4b58430006b8679dc Reviewed-on: https://chromium-review.googlesource.com/777587 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#518451} [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/chrome/browser/apps/drive/drive_app_converter.cc [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/chrome/browser/chromeos/login/screens/terms_of_service_screen.cc [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/chrome/browser/net/network_context_configuration_browsertest.cc [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/chrome/browser/net/profile_network_context_service_browsertest.cc [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/content/browser/browsing_data/browsing_data_remover_impl_browsertest.cc [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/content/browser/network_service_restart_browsertest.cc [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/content/public/common/simple_url_loader.cc [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/content/public/common/simple_url_loader.h [modify] https://crrev.com/75ef3e360ead4ff096fc11722d5950d53518c34a/content/public/common/simple_url_loader_unittest.cc
,
Nov 29 2017
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db988cff8a4d8625ad65ada8611682728485a629 commit db988cff8a4d8625ad65ada8611682728485a629 Author: Matt Menke <mmenke@chromium.org> Date: Tue Dec 12 19:25:36 2017 Add file upload support to SimpleURLLoader. This CL passes in the path for the network service to open. It's an intermediary step towards opening the file in-process and passing the file handle to the service instead. Bug: 746977 Change-Id: Ic432cd2d78027a83e9f63e2bc2a66c0eea51b937 Reviewed-on: https://chromium-review.googlesource.com/811887 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#523505} [modify] https://crrev.com/db988cff8a4d8625ad65ada8611682728485a629/content/public/common/simple_url_loader.cc [modify] https://crrev.com/db988cff8a4d8625ad65ada8611682728485a629/content/public/common/simple_url_loader.h [modify] https://crrev.com/db988cff8a4d8625ad65ada8611682728485a629/content/public/common/simple_url_loader_unittest.cc
,
Jan 17 2018
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/82465c10ada82a0751102bdfe02aac27578c9897 commit 82465c10ada82a0751102bdfe02aac27578c9897 Author: Matt Menke <mmenke@chromium.org> Date: Tue Jan 30 17:43:59 2018 Fix SimpleURLLoader test fixture. MockURLLoader::RunTest had a case statement that was accidetnally falling through to the next case. Bug: 177475 , 746977 Change-Id: Ifce64d1afaaaff6b33ef8057517627e694389641 Reviewed-on: https://chromium-review.googlesource.com/891739 Reviewed-by: Randy Smith <rdsmith@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#532933} [modify] https://crrev.com/82465c10ada82a0751102bdfe02aac27578c9897/content/public/common/simple_url_loader_unittest.cc
,
Feb 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6ed13d615a9a818baf0988b1ae51ed17a568326 commit a6ed13d615a9a818baf0988b1ae51ed17a568326 Author: Matt Menke <mmenke@chromium.org> Date: Sun Feb 04 22:39:06 2018 SimpleURLLoader: Add streaming support. This allows a SimpleURLLoader consumer to process data as it comes in, instead of writing it to a file or appending it to a single (potentially quite long) string. Bug: 746977 Change-Id: I34c3f9dba1973d49d46e08d8c9947a57d7185992 Reviewed-on: https://chromium-review.googlesource.com/883665 Reviewed-by: Randy Smith <rdsmith@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Commit-Queue: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#534303} [modify] https://crrev.com/a6ed13d615a9a818baf0988b1ae51ed17a568326/content/public/common/BUILD.gn [modify] https://crrev.com/a6ed13d615a9a818baf0988b1ae51ed17a568326/content/public/common/simple_url_loader.cc [modify] https://crrev.com/a6ed13d615a9a818baf0988b1ae51ed17a568326/content/public/common/simple_url_loader.h [add] https://crrev.com/a6ed13d615a9a818baf0988b1ae51ed17a568326/content/public/common/simple_url_loader_stream_consumer.h [modify] https://crrev.com/a6ed13d615a9a818baf0988b1ae51ed17a568326/content/public/common/simple_url_loader_unittest.cc
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a8406df19ea46ff766118416186b87940c45ff88 commit a8406df19ea46ff766118416186b87940c45ff88 Author: Matt Menke <mmenke@chromium.org> Date: Wed Mar 07 21:46:58 2018 Create a TYPE_CHUNKED_DATA_PIPE DataElement. And add support for it to services/network. This CL introduces network::mojom::ChunkedDataPipeGetters which is similar to DataPipeGettter, but is used for chunked uploads, which allow consumers to start uploading a request body before its size is known. Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I7d7dfad4b6e33329cf56f7c9d1666944a7b25475 Bug: 746977 Reviewed-on: https://chromium-review.googlesource.com/908828 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Helen Li <xunjieli@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#541594} [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/content/browser/loader/upload_data_stream_builder.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/content/common/page_state_serialization.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/content/common/service_worker/service_worker_loader_helpers.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/content/renderer/loader/web_url_request_util.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/BUILD.gn [add] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/chunked_data_pipe_upload_data_stream.cc [add] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/chunked_data_pipe_upload_data_stream.h [add] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/chunked_data_pipe_upload_data_stream_unittest.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/data_pipe_element_reader.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/data_pipe_element_reader.h [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/cpp/BUILD.gn [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/cpp/data_element.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/cpp/data_element.h [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/cpp/network_param_ipc_traits.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/cpp/resource_request_body.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/cpp/resource_request_body.h [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/mojom/BUILD.gn [add] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/mojom/chunked_data_pipe_getter.mojom [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/public/mojom/data_pipe_getter.mojom [add] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/test_chunked_data_pipe_getter.cc [add] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/test_chunked_data_pipe_getter.h [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/url_loader.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/services/network/url_loader_unittest.cc [modify] https://crrev.com/a8406df19ea46ff766118416186b87940c45ff88/storage/browser/blob/blob_data_builder.cc
,
Mar 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f520a8495ee33d9e4be72be42adab8efd0b96dd commit 7f520a8495ee33d9e4be72be42adab8efd0b96dd Author: Matt Menke <mmenke@chromium.org> Date: Wed Mar 28 21:38:37 2018 Add a note to URLFetcher that it shouldn't be use in content embedders. Also add a presubmit warning to trigger on new/modified uses of URLFetcher. Bug: 746977 , 598073 Change-Id: I962a9f9bdb0c8c533f5cc4e7ca5a919f917a89ca Reviewed-on: https://chromium-review.googlesource.com/984512 Reviewed-by: Helen Li <xunjieli@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#546605} [modify] https://crrev.com/7f520a8495ee33d9e4be72be42adab8efd0b96dd/PRESUBMIT.py [modify] https://crrev.com/7f520a8495ee33d9e4be72be42adab8efd0b96dd/net/url_request/url_fetcher.h
,
Mar 30 2018
Do URLFetcher users have to pause their current work and convert to network::SimpleURLLoader before continuing? You may want to send a chromium-dev PSA, so developers won't be caught by surprise.
,
Mar 30 2018
No, they don't - people shouldn't be adding new uses of it, though. I was asked for a presubmit check when I commented about this on a a CL. Really, this applies to all consumers of any net object moving out of process, though mojo wrappers aren't available to all of them yet.
,
Mar 30 2018
This got on my radar when I made some changes [1] and hit the presubmit warning. I've bypassed it on upload since [1] hasn't violated the spirit of the presubmit. [1] https://chromium-review.googlesource.com/c/chromium/src/+/977562/6/chrome/browser/printing/cloud_print/privet_url_fetcher.cc
,
Mar 30 2018
Bypassing the presubmit check to update current consumers is indeed entirely reasonable - if I could have made the check only trigger on new consumers, I would have done that (Well, without including a list of the 100+ current consumers in the presubmit check, at least).
,
Apr 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59716d015d9d4a7870990a83419d7b5c3806a213 commit 59716d015d9d4a7870990a83419d7b5c3806a213 Author: Matt Menke <mmenke@chromium.org> Date: Thu Apr 05 12:45:53 2018 Make the net::URLFetcher presubmit check not require a presubmit bypass I misunderstood the boolean parameter. This check is intended to be advisory, rather than something that blocks landing CLs. Bug: 746977 , 598073 Change-Id: Ib7695f7bea15f1613a4f9e73e0258fa231340161 Reviewed-on: https://chromium-review.googlesource.com/992393 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#548394} [modify] https://crrev.com/59716d015d9d4a7870990a83419d7b5c3806a213/PRESUBMIT.py
,
May 17 2018
Marking this as fixed, though may still need to add a feature or two. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by mmenke@chromium.org
, Jul 25 2017Status: Available (was: Started)