Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951813002:
Make URLRequestContextBuilder use base/task_scheduler/ (Closed)
|
Created:
3 years, 6 months ago by mmenke Modified:
3 years, 5 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake URLRequestContextBuilder use base/task_scheduler/
when no file thread is specified.
When no thread was input, it used to create a single thread itself
and run most file operations on that. Now it creates
[Sequenced]TaskRunners for the TransportSecurityPersister and to
handle file URLs. It also creates a SingleThreadedTaskRunner for
the cache, if enabled. This makes it more friendly to use in
Chrome, without causing extra threads to be created.
If this behavior is not desired, the consumer can still pass in a
SingleThreadedTaskRunner, and that will be used instead.
The properties it uses do not exactly match what ProfileIOData is
using - file URLs now use a USER_BLOCKING TaskRunner instead of
USER_VISIBLE.
This is a followup to https://codereview.chromium.org/2929153002/.
BUG=715695
Review-Url: https://codereview.chromium.org/2951813002
Cr-Commit-Position: refs/heads/master@{#482746}
Committed: https://chromium.googlesource.com/chromium/src/+/8d890ab33ea80c5a4b903f7717c77043525088ab
Patch Set 1 #Patch Set 2 : Fix #
Total comments: 13
Patch Set 3 : Response to comments #
Total comments: 3
Patch Set 4 : Switch to block shutdown #Patch Set 5 : Merge #
Total comments: 4
Messages
Total messages: 50 (34 generated)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
This fixes the other "Regression" from the IOThread CL, though you pointed out this on during review. Now working on a more complicated CrOS proxy config regression, which was caused by a CL you didn't review.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
On 2017/06/20 23:14:26, mmenke wrote: > This fixes the other "Regression" from the IOThread CL, though you pointed out > this on during review. Now working on a more complicated CrOS proxy config > regression, which was caused by a CL you didn't review. Also, note that Cronet uses SetFileTaskRunner on both Android and iOS, so this won't add bonus threads on either platform. I want to investigate if the cache can be switched to a SequencedTaskRunner, but I don't think we care about the proxy config using a SingleThreadedTaskRunner.
On 2017/06/20 23:18:08, mmenke wrote: > On 2017/06/20 23:14:26, mmenke wrote: > > This fixes the other "Regression" from the IOThread CL, though you pointed out > > this on during review. Now working on a more complicated CrOS proxy config > > regression, which was caused by a CL you didn't review. > > Also, note that Cronet uses SetFileTaskRunner on both Android and iOS, so this > won't add bonus threads on either platform. I want to investigate if the cache > can be switched to a SequencedTaskRunner, but I don't think we care about the > proxy config using a SingleThreadedTaskRunner. Quick top level questions/comments, not blocking my review: * nit: Change "without cause extra" to "without causing extra" in the CL description. * I don't understanding the reasoning behind having more USER_BLOCKING rather than USER_VISIBLE traits in the builder as compared to ProfileIOData; is this just preserving behavior for SystemURLRequestContext for the moment, or is it a behavior change? * I'm also not sure why we don't care about the proxy config using a SingleThreadedTaskRunner; an extra thread is an extra thread (and hence just as objectionable in the proxy context as in other contexts), right?
First round of comments. https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:419: GetSingleThreadTaskRunner( When I first looked at this, I thought "Oh, of course it needs it's own thread", but then I realized I was confusing it with the pac resolver. What's the rationale for the ProxyService having it's own thread? https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:421: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})); Why CONTINUE_ON_SHUTDOWN for the proxy service? https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:488: base::TaskShutdownBehavior::BLOCK_SHUTDOWN})); It's not doubt due to my mis-reading, but where is this variable used? It looks like it's constructed and then thrown away. https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:494: base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}))); Hmmm. Everything else was USER_BLOCKING, which made sense to me because it was in the user request resolution path. But this is as well, but it's only USER_VISIBLE. What's the rationale? https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.h:366: // traits. nit, suggestion: It bothers me a little that we're being specific about using a file related thingy above (in the setter) but the naming of these methods doesn't mention that it's for file operations. WDYT about including "File" in the names? https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.h:366: // traits. I also find myself wondering if names that captured more of the goals of these routines for URLRequestContextBuilder (i.e. slightly higher level) might be better. I'll mention that now in case it sparks ideas in your mind, but feel free to ignore the comment--if I still feel that way after I understand the purposes better I'll make concrete suggestions.
Description was changed from ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. It creates task runners with the appropriate traits as needed, instead of creating its own thread and running them all on that. This makes it more friendly to use in Chrome, without cause extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - more operations are considered USER_BLOCKING instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 ========== to ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. It creates task runners with the appropriate traits as needed, instead of creating its own thread and running them all on that. This makes it more friendly to use in Chrome, without causing extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - more operations are considered USER_BLOCKING instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 ==========
On 2017/06/21 12:23:29, Randy Smith (Not in Mondays) wrote: > On 2017/06/20 23:18:08, mmenke wrote: > > On 2017/06/20 23:14:26, mmenke wrote: > > > This fixes the other "Regression" from the IOThread CL, though you pointed > out > > > this on during review. Now working on a more complicated CrOS proxy config > > > regression, which was caused by a CL you didn't review. > > > > Also, note that Cronet uses SetFileTaskRunner on both Android and iOS, so this > > won't add bonus threads on either platform. I want to investigate if the > cache > > can be switched to a SequencedTaskRunner, but I don't think we care about the > > proxy config using a SingleThreadedTaskRunner. > > Quick top level questions/comments, not blocking my review: > * nit: Change "without cause extra" to "without causing extra" in the CL > description. > > * I don't understanding the reasoning behind having more USER_BLOCKING rather > than USER_VISIBLE traits in the builder as compared to ProfileIOData; is this > just preserving behavior for SystemURLRequestContext for the moment, or is it a > behavior change? It's based on the descriptions of USER_VISIBLE and USER_BLOCKING - all of these potentially block loading the foreground page, which seems USER_BLOCKING to me, per description. > * I'm also not sure why we don't care about the proxy config using a > SingleThreadedTaskRunner; an extra thread is an extra thread (and hence just as > objectionable in the proxy context as in other contexts), right? Note that the task scheduler can still share threads between SingleThreadedTaskRunner, it just has to commit to doing so up front. My general feeling is we should stick to the public API docs of TaskScheduler. If that results in too many threads, that's a TaskScheduler bug (Either a bug in the docs, which should advise on what we should be doing, or a bug in the implementation).
https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:419: GetSingleThreadTaskRunner( On 2017/06/21 12:56:26, Randy Smith (Not in Mondays) wrote: > When I first looked at this, I thought "Oh, of course it needs it's own thread", > but then I realized I was confusing it with the pac resolver. What's the > rationale for the ProxyService having it's own thread? ProxyService::CreateSystemProxyConfigService requires a SingleThreadTaskRunner is the only rationale. Turns out this argument is only used on OS_LINUX, anyways (Which the above ifdef avoids running this code on), so I think we can just use nullptr. Problem solved. :) (Note that this code doesn't seem to be used anywhere, anyways - since it doesn't work on Linux and Android, and so anyone who wants proxy support on those platforms has to set up their own ProxyConfigService). https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:421: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})); On 2017/06/21 12:56:26, Randy Smith (Not in Mondays) wrote: > Why CONTINUE_ON_SHUTDOWN for the proxy service? Because we aren't writing anything to disk, so no reason to block shutdown on getting the proxy configuration. Moot now anyways. https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:488: base::TaskShutdownBehavior::BLOCK_SHUTDOWN})); On 2017/06/21 12:56:26, Randy Smith (Not in Mondays) wrote: > It's not doubt due to my mis-reading, but where is this variable used? It looks > like it's constructed and then thrown away. Oops...Copy-paste error. https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:494: base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}))); On 2017/06/21 12:56:26, Randy Smith (Not in Mondays) wrote: > Hmmm. Everything else was USER_BLOCKING, which made sense to me because it was > in the user request resolution path. But this is as well, but it's only > USER_VISIBLE. What's the rationale? So, actually, despite the CL description, this was copying the existing behavior. Using the description, though, USER_BLOCKING does seem accurate here. Copying is also why this is SKIP_ON_SHUTDOWN - I'd tend to think CONTINUE_ON_SHUTDOWN (Which means we don't block shutdown on completing pending tasks) would be more appropriate, but leaving as-is for now. https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.h:366: // traits. On 2017/06/21 12:56:26, Randy Smith (Not in Mondays) wrote: > I also find myself wondering if names that captured more of the goals of these > routines for URLRequestContextBuilder (i.e. slightly higher level) might be > better. I'll mention that now in case it sparks ideas in your mind, but feel > free to ignore the comment--if I still feel that way after I understand the > purposes better I'll make concrete suggestions. I'm open to other ideas. Largely I just wanted reusable methods to use either the task schedule or passed in TaskRunner. Each method is only used once currently, but we'll need to at least add the CookieStore, and I think having methods is less regression prone than inlinine the logic everywhere. https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.h:366: // traits. On 2017/06/21 12:56:26, Randy Smith (Not in Mondays) wrote: > nit, suggestion: It bothers me a little that we're being specific about using a > file related thingy above (in the setter) but the naming of these methods > doesn't mention that it's for file operations. WDYT about including "File" in > the names? Done. I didn't do this in the first place mostly because I didn't like either GetFile*TaskRunner as sounding awkward or Get*FileTaskRunner as interrupting class names, and adding more works (ForFileIO or whatever) starts making these method names absurdly long.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I don't know if you were listening in to my conversation, but I think we're going to need to take in the cache thread as input.... I'm rethinking my view on creating SingleThreadedTaskRunners through the interface, mostly because of all the app requests contexts. Admittedly, they don't currently use URLRequestContextBuilder, and I'm not planning on switching them over anytime soon, but I suspect that will become a problem. Anyhow, not going to worry about it in this CL, but may add a way just to set the cache thread before switching ProfileIOData over.
Sorry this is taking so many rounds; trying to do them quickly. More high level questions: * I mention below that I'd like the CL description to be clear about in which ways this CL is expected to change behavior and which ways it isn't. This is probably a subset of that request but confirming: The mention of the difference with ProfileIOData isn't a warning of a behavior change, just a future possible incompatibility? * You said in a comment response that you were copying behavior. Where were you copying it from? Was it your interpretation of what the created file thread would do, or was it from one of the consumer specified STTRs? If so, which one? https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2951813002/diff/80001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:494: base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}))); On 2017/06/21 15:13:55, mmenke wrote: > On 2017/06/21 12:56:26, Randy Smith (Not in Mondays) wrote: > > Hmmm. Everything else was USER_BLOCKING, which made sense to me because it > was > > in the user request resolution path. But this is as well, but it's only > > USER_VISIBLE. What's the rationale? > > So, actually, despite the CL description, this was copying the existing > behavior. Using the description, though, USER_BLOCKING does seem accurate here. Ah, I missed that. I'd suggest being pretty clear in the CL description as to what behaviors are expected to change and what not as a result of this CL--the regression investigator you'll be helping is unlikely to be yourself, but might appreciate it anyway :-}. > Copying is also why this is SKIP_ON_SHUTDOWN - I'd tend to think > CONTINUE_ON_SHUTDOWN (Which means we don't block shutdown on completing pending > tasks) would be more appropriate, but leaving as-is for now. Agreed now that I've mapped in the constant definitions, but if you want to keep it to match existing behavior, that's also ok by me. https://codereview.chromium.org/2951813002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2951813002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:456: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}))); How confident are you that the cache is resilient against posted tasks being killed on shutdown? I could imagine consistency issues. If you're matching existing behavior, I won't block, but I'd be inclined to check with Gavin/Maks--I could imagine this being relevant to stuff they're working on. ETA: Ok, I went digging to see if I could confirm that this is existing behavior, and I failed. If the builder created its own file thread, that would have been done with default Options::joinable, which is true, which I think is BLOCK_SHUTDOWN. What am I missing?
On 2017/06/21 15:45:55, mmenke wrote: > I don't know if you were listening in to my conversation, but I think we're > going to need to take in the cache thread as input.... I'm rethinking my view > on creating SingleThreadedTaskRunners through the interface, mostly because of > all the app requests contexts. Admittedly, they don't currently use > URLRequestContextBuilder, and I'm not planning on switching them over anytime > soon, but I suspect that will become a problem. Anyhow, not going to worry > about it in this CL, but may add a way just to set the cache thread before > switching ProfileIOData over. Acknowledged, but I wasn't listening, so that information wasn't incorporated in the comments I just made.
https://codereview.chromium.org/2951813002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2951813002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:456: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}))); In case of simple cache, it's "resilient" in the sense it might lose the index and have to recover it. Don't know about block cache, but probably worse, that one would have to be confirmed by Gavin (and might have been had I not butted in into Matt's and his conversation..) Though it's curious that this is passing in FileTaskRunner and not the browser CACHE thread (which they are also trying to remove, but which is what most clients of disk_cache::CreateCacheBackend use.
Description was changed from ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. It creates task runners with the appropriate traits as needed, instead of creating its own thread and running them all on that. This makes it more friendly to use in Chrome, without causing extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - more operations are considered USER_BLOCKING instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 ========== to ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. It creates task runners with the appropriate traits as needed, instead of creating its own thread and running them all on that. This makes it more friendly to use in Chrome, without causing extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - file URLs now use a USER_BLOCKING TaskRunner instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 ==========
On 2017/06/21 15:48:52, Randy Smith (Not in Mondays) wrote: > Sorry this is taking so many rounds; trying to do them quickly. Not a problem - I don't think there's a huge rush on this one, anyways. I should be the one apologizing for the initial state of this CL. I'm a bit frazzled by a P-1 ChromeOS ProxyConfig stable/beta blocker, but not much of an excuse. > More high level questions: > * I mention below that I'd like the CL description to be clear about in which > ways this CL is expected to change behavior and which ways it isn't. This is > probably a subset of that request but confirming: The mention of the difference > with ProfileIOData isn't a warning of a behavior change, just a future possible > incompatibility? I think IOThread used the same priorities as ProfileIOData, so it is a change for that as well (Of course, it's a change from what we were doing before I switched over the URLRequestContextBuilder, and a different change from after I switched over, which I think gets into too much nuance for a CL description, hence the mention of ProfileIOData). Note that the system URLRequestContext didn't even support file URLs until I made it to PAC fetching itself instead of using a Proxy URLRequestContext, so this really only applies to a tiny number of requests made by a tiny number of users for PAC files using file URLs. Anyhow, I've updated the description. > * You said in a comment response that you were copying behavior. Where were you > copying it from? Was it your interpretation of what the created file thread > would do, or was it from one of the consumer specified STTRs? If so, which one? It was just the file scheme where I copied behavior (From ProfileIOData). I thought I'd be copying cookie storage setup as well, but turns out that URLRequestContextBuilder doesn't currently support setting up an on-disk cookie store. https://codereview.chromium.org/2951813002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2951813002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:456: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}))); On 2017/06/21 15:58:28, Maks Orlovich wrote: > In case of simple cache, it's "resilient" in the sense it might lose the index > and have to recover it. Don't know about block cache, but probably worse, that > one would have to be confirmed by Gavin (and might have been had I not butted in > into Matt's and his conversation..) So I think you're right about this. I can't remember what I was thinking when I made this CONTNIUE_ON_SHUTDOWN (May have been confused by the BLOCKING in USER_BLOCKING or something. Or may not have been thinking at all). > Though it's curious that this is passing in FileTaskRunner and not the browser > CACHE thread (which they are also trying to remove, but which is what most > clients of disk_cache::CreateCacheBackend use. Note that the "file" TaskRunner is just the single task runner passed into the URLRequestContextBuilder for file I/O. It's currently nullptr in Chrome, and we're falling back to using the task scheduler. Of course, this isn't being used at all currently, anyways, since this class is only being used with the SystemURLRequestContext, which doesn't have a cache.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
As we talked about offline, I'd like the CL description to be clear that this is a (admittedly minor) change in the behavior that consumers of URLRequestContextBuilder might notice--it's not just a refactor. I'm inclined to specifically call out SystemURLRequestContext, and to not mention ProfileIOData (since that's not relevant yet), but up to you. Beyond that, LGTM; stuff below is suggestions or random musing. https://codereview.chromium.org/2951813002/diff/140001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/2951813002/diff/140001/net/url_request/url_re... net/url_request/url_request_context_builder.h:366: // traits. Confirming that it's intentional on your part that these are above the 'private' line, and hence that you have a use case in mind for subclasses using them? (If you haven't already queued the CL up, my personal preference would be that these be private methods and they be shifted out to protected when first used. But I'm ok with this if you prefer it.) https://codereview.chromium.org/2951813002/diff/140001/net/url_request/url_re... net/url_request/url_request_context_builder.h:372: const base::TaskTraits& traits); Thought (i.e. a notch underneath suggestion): At this point, each of these routines is only called once, which seems like not much work for the overhead of three additions to the interface. If you hoisted the calls into their calling locations, I think it would be fewer total LOC. Alternatively, combining the three calls might work (though that would seem to require an enum, which is ugly). Anyway, just raising the question as to whether there are other ways you want to structure this.
Description was changed from ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. It creates task runners with the appropriate traits as needed, instead of creating its own thread and running them all on that. This makes it more friendly to use in Chrome, without causing extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - file URLs now use a USER_BLOCKING TaskRunner instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 ========== to ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. When no thread was input, it used to create a single thread itself and run most file operations on that. Now it creates [Sequenced]TaskRunners for the TransportSecurityPersister and to handle file URLs. It also creates a SingleThreadedTaskRunner for the cache, if enabled. This makes it more friendly to use in Chrome, without causing extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - file URLs now use a USER_BLOCKING TaskRunner instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 ==========
Updated description https://codereview.chromium.org/2951813002/diff/140001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/2951813002/diff/140001/net/url_request/url_re... net/url_request/url_request_context_builder.h:366: // traits. On 2017/06/27 21:00:51, Randy Smith (Not in Mondays) wrote: > Confirming that it's intentional on your part that these are above the 'private' > line, and hence that you have a use case in mind for subclasses using them? > > (If you haven't already queued the CL up, my personal preference would be that > these be private methods and they be shifted out to protected when first used. > But I'm ok with this if you prefer it.) Yes, that's deliberate. While I'm not sure subclasses ever will need them, it seems reasonable for them to have access. I generally prefer to err on the side of making things private, but in this case, these just seemed very reasonable to expose from the start. https://codereview.chromium.org/2951813002/diff/140001/net/url_request/url_re... net/url_request/url_request_context_builder.h:372: const base::TaskTraits& traits); On 2017/06/27 21:00:51, Randy Smith (Not in Mondays) wrote: > Thought (i.e. a notch underneath suggestion): At this point, each of these > routines is only called once, which seems like not much work for the overhead of > three additions to the interface. If you hoisted the calls into their calling > locations, I think it would be fewer total LOC. Alternatively, combining the > three calls might work (though that would seem to require an enum, which is > ugly). Anyway, just raising the question as to whether there are other ways you > want to structure this. I'll have to use these again for the cookie store (and presumably the channel ID store), at least, so while they are called only once, I think this is simplest for future changes. Also, I can't merge them, unless I use a template - they each return different types. Once most things are hooked up, I'm thinking I'll eventually do a simplification pass over this class, figure out what's needed, what's not.
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498597832233720,
"parent_rev": "cfbf85b0c37adf3acccead282bf5c9b794ba4009", "commit_rev":
"8d890ab33ea80c5a4b903f7717c77043525088ab"}
Message was sent while issue was closed.
Description was changed from ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. When no thread was input, it used to create a single thread itself and run most file operations on that. Now it creates [Sequenced]TaskRunners for the TransportSecurityPersister and to handle file URLs. It also creates a SingleThreadedTaskRunner for the cache, if enabled. This makes it more friendly to use in Chrome, without causing extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - file URLs now use a USER_BLOCKING TaskRunner instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 ========== to ========== Make URLRequestContextBuilder use base/task_scheduler/ when no file thread is specified. When no thread was input, it used to create a single thread itself and run most file operations on that. Now it creates [Sequenced]TaskRunners for the TransportSecurityPersister and to handle file URLs. It also creates a SingleThreadedTaskRunner for the cache, if enabled. This makes it more friendly to use in Chrome, without causing extra threads to be created. If this behavior is not desired, the consumer can still pass in a SingleThreadedTaskRunner, and that will be used instead. The properties it uses do not exactly match what ProfileIOData is using - file URLs now use a USER_BLOCKING TaskRunner instead of USER_VISIBLE. This is a followup to https://codereview.chromium.org/2929153002/. BUG=715695 Review-Url: https://codereview.chromium.org/2951813002 Cr-Commit-Position: refs/heads/master@{#482746} Committed: https://chromium.googlesource.com/chromium/src/+/8d890ab33ea80c5a4b903f7717c7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/8d890ab33ea80c5a4b903f7717c7...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 482746 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
