ProxyResolutionService threading documentation |
||
Issue description
The ProxyResolutionService expects to receive a SequencedTaskRunner that runs on the IO thread:
ProxyResolutionService::CreateSystemProxyConfigService(
const scoped_refptr<base::SequencedTaskRunner>& io_task_runner);
But Chrome now passes the task runner for the UI thread.
Downstream dependencies like ProxyConfigServiceLinux::SetupAndFetchInitialConfig() have similar documentation.
This change was made here and there was some follow-up work proposed: https://chromium-review.googlesource.com/c/chromium/src/+/760538/25/chrome/browser/net/proxy_service_factory.cc#38
Could we take care of this documentation/variable name changes to keep the code consistent and self-explanatory?
,
Mar 17 2018
[michaelpg]: I'm having trouble finding any documentation that refers to the IO thread, other than that one argument. What am I missing?
,
Mar 17 2018
Oh, I also didn't realize I needed to update the "ProxyResolutionService" methods that don't actually have anything to do with the ProxyResolutionService. Is that it? Just that one ProxyConfigServiceLinux method, and the weird static ProxyResolutionService methods that should probably be ProxyConfigService methods?
,
Mar 17 2018
Yep, looks like that's it. I'm just wondering what to pass to ProxyResolutionService::CreateSystemProxyConfigService() from places like content_shell/app_shell which don't use the NetworkContext stuff.
,
Mar 18 2018
Do it on the threads they're used on (Which is the IO thread, currently). With Chrome, they now send the settings through Mojo, so it's simplest for them to be on the UI thread, where the NetworkContext pipe lives. When we port everything to use the network service, we can move them over to the UI thread.
,
Mar 18 2018
Incidentally, what's the status of the network service? I'm reworking AppShell's networking (it's currently based on content_shell which is optimized for testing) -- should I be using the network service, or is it not ready yet, especially for non-Chrome embedders?
,
Mar 19 2018
Making progress, but there are still a bunch of things that aren't hooked up yet, and we're focusing more on Chrome's needs at the moment, and haven't really been thinking about other content-embedders, for the most part. We have made the URLLoader/SimpleURLLoader APIs available to content/ embedders, however, so shared components can now make network requests in a network-service friendly way. Just grab the URLLoaderFactory (Or NetworkContext) from the StoragePartition, and you're good to go, if the code was using a profile's main URLRequestContext. We don't have a standard answer yet for things that use other URLRequestContexts (Like their own SystemURLRequestContext-equivalents)
,
Mar 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8ee4019ff78d4fea9e9cd82ae86b0adc2de5498 commit d8ee4019ff78d4fea9e9cd82ae86b0adc2de5498 Author: Matt Menke <mmenke@chromium.org> Date: Fri Mar 23 23:10:09 2018 Replace two references to io_task_runner in net's proxy config code. The system ProxyConfigService is now created on Chrome's UI thread, so the references were outdated. Also, net/ really shouldn't know about content/'s threads. Bug: 823063 Change-Id: I399bdc13fa46b89f45111cafb393e2c9cd735bdd Reviewed-on: https://chromium-review.googlesource.com/978386 Commit-Queue: Eric Roman <eroman@chromium.org> Reviewed-by: Eric Roman <eroman@chromium.org> Cr-Commit-Position: refs/heads/master@{#545614} [modify] https://crrev.com/d8ee4019ff78d4fea9e9cd82ae86b0adc2de5498/net/proxy_resolution/proxy_config_service_linux.h [modify] https://crrev.com/d8ee4019ff78d4fea9e9cd82ae86b0adc2de5498/net/proxy_resolution/proxy_resolution_service.cc [modify] https://crrev.com/d8ee4019ff78d4fea9e9cd82ae86b0adc2de5498/net/proxy_resolution/proxy_resolution_service.h
,
Mar 24 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by mmenke@chromium.org
, Mar 17 2018