Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929153002:
Use URLRequestContextBuilderMojo to create the SystemURLRequestContext. (Closed)
DescriptionUse URLRequestContextBuilderMojo to create the SystemURLRequestContext.
Also adds a DhcpProxyScriptFetcherFactoryChromeos, to inject into
the URLRequestContextBuilder on ChromeOS (The actual fetcher must be
created after the URLRequestContext, and the factory avoids the whole
chicken/egg thing).
BUG=715695
Review-Url: https://codereview.chromium.org/2929153002
Cr-Commit-Position: refs/heads/master@{#479854}
Committed: https://chromium.googlesource.com/chromium/src/+/14085ad54d50252bcadd1b1d7dc4a0bca2c9c5b3
Patch Set 1 #Patch Set 2 : Remove ability to disable DhcpFetcherFactories, as it's only used in tests #Patch Set 3 : Fixes #Patch Set 4 : Forgot the ProxyConfigService.... #Patch Set 5 : Oops #Patch Set 6 : Erm... Oops (and merge). #
Total comments: 2
Patch Set 7 : Merge #
Total comments: 19
Dependent Patchsets: Messages
Total messages: 72 (47 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...
Patchset #3 (id:40001) has been deleted
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Description was changed from ========== Use URLRequestContextBuilderMojo to create the SystemURLRequestContext. BUG=715695 ========== to ========== Use URLRequestContextBuilderMojo to create the SystemURLRequestContext. BUG=715695 ==========
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use URLRequestContextBuilderMojo to create the SystemURLRequestContext. BUG=715695 ========== to ========== Use URLRequestContextBuilderMojo to create the SystemURLRequestContext. Also adds a DhcpProxyScriptFetcherFactoryChromeos, to inject into the URLRequestContextBuilder on ChromeOS (The actual fetcher must be created after the URLRequestContext, and the factory avoids the whole chicken/egg thing). BUG=715695 ==========
The CQ bit was unchecked by mmenke@chromium.org
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...
Patchset #3 (id:80001) has been deleted
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
Wasn't quite expecting it to take me a month just to get this far, with barely touching ProfileIOData. https://codereview.chromium.org/2929153002/diff/200001/net/url_request/url_re... File net/url_request/url_request_context_builder_mojo.h (right): https://codereview.chromium.org/2929153002/diff/200001/net/url_request/url_re... net/url_request/url_request_context_builder_mojo.h:37: dhcp_fetcher_factory_ = std::move(dhcp_fetcher_factory); Definitely is a question on how much to unit test this stuff, as opposed to relying on integration tests. For IOThread, we also don't have many integration tests, either. Only two tests even test the IOThread with PAC scripts (Both of which I wrote in the last month).
https://codereview.chromium.org/2929153002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2929153002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.cc:887: builder.set_proxy_config_service(std::move(system_proxy_config_service_)); These two blocks are basically inlining ProxyServiceFactory::CreateProxyService (Though the builder here actually creates the service itself)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Randy, this goes without saying, but I'll say it anyways, please do feel free to call out anything you think I should be adding unit tests for, or suggest I split up CLs if you think it would be easier to review things in smaller chunks. This CL does glom a couple changes together, but I think that everything but the IOThread change is simple enough that splitting it up isn't worth the effort.
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...
On 2017/06/13 14:08:44, mmenke wrote: > Randy, this goes without saying, but I'll say it anyways, please do feel free to > call out anything you think I should be adding unit tests for, or suggest I > split up CLs if you think it would be easier to review things in smaller chunks. > This CL does glom a couple changes together, but I think that everything but > the IOThread change is simple enough that splitting it up isn't worth the > effort. It's worth saying, at least around the CL splitting--I tend to just plow through the CLs I'm given. I expect I'll both ask for unit tests if I want them, and that your standards in that space are higher than mine, so no worries there :-}.
QQ: What's the story with the dependent CLs? They don't look like they're intended to land.
On 2017/06/13 15:23:55, Randy Smith (Not in Mondays) wrote: > QQ: What's the story with the dependent CLs? They don't look like they're > intended to land. Oh, never mind; I was confusing "dependent" and "depends on".
On 2017/06/13 15:23:55, Randy Smith (Not in Mondays) wrote: > QQ: What's the story with the dependent CLs? They don't look like they're > intended to land. It's not yet ready to land (And probably not actually depedent on this one), but I expect I'll send it out to you later today. I expect 3-6 more smallish CLs before I get ProfileIOData and OffTheRecordProfileIOData over to using URLRequestContextBuilder. Then it should be fairly easy to merge IOThread's and ProfileIOData's URLRequestContextBuilder configuration logic (Since the OTR main URLRequestContext looks a lot like the system one). I have triage duty this week, but hope to get all of that out by end of next week, baring any unforseen complexities.
On 2017/06/13 15:24:24, Randy Smith (Not in Mondays) wrote: > On 2017/06/13 15:23:55, Randy Smith (Not in Mondays) wrote: > > QQ: What's the story with the dependent CLs? They don't look like they're > > intended to land. > > Oh, never mind; I was confusing "dependent" and "depends on". Ahh, yea, I've made that mistake before.
Request that's parallel to and separate from this CL: URLRequestContextBuilder doesn't really appear to have any detailed documentation on what it's doing and what it's relationship to URLRequestContextStorage is. I'd love to see that added at some point. (I'm working it out on my own, since even if you add it I'd like to be in a position to review that CL, but if I'm having to work it out on my own, that suggests some comments would be useful.)
On 2017/06/13 15:35:08, Randy Smith (Not in Mondays) wrote: > Request that's parallel to and separate from this CL: URLRequestContextBuilder > doesn't really appear to have any detailed documentation on what it's doing and > what it's relationship to URLRequestContextStorage is. I'd love to see that > added at some point. > > (I'm working it out on my own, since even if you add it I'd like to be in a > position to review that CL, but if I'm having to work it out on my own, that > suggests some comments would be useful.) URLRequestContextBuilder doesn't publicly expose its URLRequestContextStorage, so API-level docs shouldn't mention it, though certainly happy to improve docs locally. URLRequestContextStorage basically mirrors the fields of a URLRequestContext, but it owns them all. Putting a unique_ptr into URLrequestContextStorage also sets the corresponding raw pointer in the associated URLRequestContext. URLRequestContextBuilder creates self-contained URLRequestContexts (Save a small number of methods that take raw pointers to things the URLRequestContext does not own). So to manage lifetime of all of the URLRequestContext's objects, it has its own URLRequestContext subclass which also owns a URLRequestContextStorage. When the URLRequestContext is destroyed, it safely (hopefully...) tears down objects in the URLRequestContextStorage it owns. Also, objects in URLRequestContextStorage should mostly be ordered so they can be town down in order, modulo having to cancel in-progress URLRequests before tearing it down.
On 2017/06/13 15:39:57, mmenke wrote: > On 2017/06/13 15:35:08, Randy Smith (Not in Mondays) wrote: > > Request that's parallel to and separate from this CL: URLRequestContextBuilder > > doesn't really appear to have any detailed documentation on what it's doing > and > > what it's relationship to URLRequestContextStorage is. I'd love to see that > > added at some point. > > > > (I'm working it out on my own, since even if you add it I'd like to be in a > > position to review that CL, but if I'm having to work it out on my own, that > > suggests some comments would be useful.) > > URLRequestContextBuilder doesn't publicly expose its URLRequestContextStorage, > so API-level docs shouldn't mention it, though certainly happy to improve docs > locally. > > URLRequestContextStorage basically mirrors the fields of a URLRequestContext, > but it owns them all. Putting a unique_ptr into URLrequestContextStorage also > sets the corresponding raw pointer in the associated URLRequestContext. > > URLRequestContextBuilder creates self-contained URLRequestContexts (Save a small > number of methods that take raw pointers to things the URLRequestContext does > not own). So to manage lifetime of all of the URLRequestContext's objects, it > has its own URLRequestContext subclass which also owns a > URLRequestContextStorage. When the URLRequestContext is destroyed, it safely > (hopefully...) tears down objects in the URLRequestContextStorage it owns. > Also, objects in URLRequestContextStorage should mostly be ordered so they can > be town down in order, modulo having to cancel in-progress URLRequests before > tearing it down. Yep. I'd argue that that doc should be split between the top of url_request_context_builder.h and url_request_context_builder.cc. There's just nothing there when you look at url_request_context_builder.h, so you don't even know that the usage pattern is "Construct the object, call whatever setters you want, call Build(), and then you've got a self-contained URLRequestContext that (mostly) acts like it owns everything it depends on." I agree that the relationship with the storage isn't part of the declaration, but I do think it's useful in the .cc file if you're trying to work out how all these classes that aren't URLRequestContext are related to each other.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Initial comments. If you could respond with the location of the code from which the lines starting with TODO(eroman) were derived from, I can keep reviewing. https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (left): https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:907: base::MakeUnique<net::DataProtocolHandler>()); nit, not sure if it should be in this CL: The code that is now being executed for this, in URLRequestContextBuilder, uses "data" rather than kDataScheme, which seems a step backwards in terms of modularity. Is there a way to fix that? (Also applies to file and ftp.) https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:913: base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}))); So the conversion to using URLRequestContextBuilder is a step backwards in that (URLRCB) uses base::Thread rather than CreateTaskRunnerWithTraits(). Reasonable to convert that, either in this CL or a separate one? https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:829: CreateDefaultAuthHandlerFactory(host_resolver.get())); nit, random thought, not necessary for this CL: Would it be reasonable for URLRequestContextBuilder to have this behavior by default? https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:858: // http://crbug.com/474654 Sorry, I poked around a bit but eventually decided I'd rather just ask: Where does this code come from/map to in the current tree? https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder.h:219: void set_http_network_session_params( Not for this CL, just commenting in case it's relevant at some future time: This interface combined with all of the interfaces below that set member variables on http_network_session_params_ strikes me a poorly designed--if the consumer doesn't realize that calling this after any of the below interfaces will blow away the effect of those interfaces, they could get a rude surprise that they don't notice (depending on testing) for a while. https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder_mojo.h (left): https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder_mojo.h:37: dhcp_fetcher_factory = std::move(dhcp_fetcher_factory_); Snort. That's cute. Is there a (presumably new) test that would catch this?
Thanks for the thoughtful review! https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (left): https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:907: base::MakeUnique<net::DataProtocolHandler>()); On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > nit, not sure if it should be in this CL: The code that is now being executed > for this, in URLRequestContextBuilder, uses "data" rather than kDataScheme, > which seems a step backwards in terms of modularity. Is there a way to fix > that? > > (Also applies to file and ftp.) Sure, I'll update it in another CL. net can certainly depend on URL. https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:913: base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}))); On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > So the conversion to using URLRequestContextBuilder is a step backwards in that > (URLRCB) uses base::Thread rather than CreateTaskRunnerWithTraits(). Reasonable > to convert that, either in this CL or a separate one? Good point, completely forgot about that. I'll update builder in another CL (Since that could break Cronet if task runners aren't fully set up there - I suspect they are, but best to be careful). https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:829: CreateDefaultAuthHandlerFactory(host_resolver.get())); On 2017/06/13 16:42:14, Randy Smith (Not in Mondays) wrote: > nit, random thought, not necessary for this CL: Would it be reasonable for > URLRequestContextBuilder to have this behavior by default? This is an IOThread method, not a net one, and depends on a number of prefs. I'm not sure if we update the factory when they change - we may. Regardless, think this is complicated enough that it's premature to worry about it. https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:858: // http://crbug.com/474654 On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > Sorry, I poked around a bit but eventually decided I'd rather just ask: Where > does this code come from/map to in the current tree? There was a comment about that in the previous patch set, but then I merged for the dependent patch set, making the comment harder to find. :( These two blocks are basically inlining ProxyServiceFactory::CreateProxyService (Though the URLRequestContextBuilderMojo here is what actually creates the service here, we just pass it the requisite parameters) https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder.h:219: void set_http_network_session_params( On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > Not for this CL, just commenting in case it's relevant at some future time: This > interface combined with all of the interfaces below that set member variables on > http_network_session_params_ strikes me a poorly designed--if the consumer > doesn't realize that calling this after any of the below interfaces will blow > away the effect of those interfaces, they could get a rude surprise that they > don't notice (depending on testing) for a while. I agree - it's on my imaginary TODO list to fix. https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder_mojo.h (left): https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder_mojo.h:37: dhcp_fetcher_factory = std::move(dhcp_fetcher_factory_); On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > Snort. That's cute. Is there a (presumably new) test that would catch this? > This is caught by integration tests...If you look at set 4, a rather large number number of ChromeOS browser tests failed, as well as a couple IOThread unittests. They all fail on a DCHECK in CreateProxyServiceUsingMojoFactory (https://cs.chromium.org/chromium/src/net/proxy/proxy_service_mojo.cc?type=cs&...). I'm not sure if we have any test that actually depends on a functional DhcpFetcherFactory being set (Either a real one or a mock one). That having been said, there are no URLRequestContextBuilderMojo unit tests that catch this, and I'm not sure we really want unit tests that check every individual setter in URLRequestContextBuilder[Mojo]. That having been said, I'd love it if we switched to using custom URLRequestContextBulders in net unittests instead of TestURLRequestContextBuilders, to better exercise this code. And this fix was the thing that inspired my comment on unit tests.
https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (left): https://codereview.chromium.org/2929153002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:913: base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}))); On 2017/06/13 17:02:22, mmenke wrote: > On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > > So the conversion to using URLRequestContextBuilder is a step backwards in > that > > (URLRCB) uses base::Thread rather than CreateTaskRunnerWithTraits(). > Reasonable > > to convert that, either in this CL or a separate one? > > Good point, completely forgot about that. I'll update builder in another CL > (Since that could break Cronet if task runners aren't fully set up there - I > suspect they are, but best to be careful). Actually, I'm going to set the task runner via the URLRequestContextBuilder API, and do it in this CL. Using a TaskRunnerWithTraits in the builder potentially changes behavior in Cronet when shutting down a URLRequestContext. Current code blocks on shutting down the file thread, new code will not. It may be what we want there, but I'm not sure it is.
I'm good with this CL as written, so LGTM (though there are a couple of things I explore in the comments below). One request for a separate CL (or this one if you want--don't care): Can you document the inheritance relationship of URLRequestContextBuilder to its subclasses in the docs in that header file? https://codereview.chromium.org/2929153002/diff/220001/chromeos/network/dhcp_... File chromeos/network/dhcp_proxy_script_fetcher_factory_chromeos.h (right): https://codereview.chromium.org/2929153002/diff/220001/chromeos/network/dhcp_... chromeos/network/dhcp_proxy_script_fetcher_factory_chromeos.h:23: // Figure out a way forward there. This isn't a problem with the Win-specific proxy script fetcher? https://codereview.chromium.org/2929153002/diff/220001/net/proxy/dhcp_proxy_s... File net/proxy/dhcp_proxy_script_fetcher_factory.cc (left): https://codereview.chromium.org/2929153002/diff/220001/net/proxy/dhcp_proxy_s... net/proxy/dhcp_proxy_script_fetcher_factory.cc:48: bool DhcpProxyScriptFetcherFactory::IsSupported() { I won't say "Never has so many characters been expended to so little purpose", but wow. Yay for deleting all this. https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder_mojo.h (left): https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder_mojo.h:37: dhcp_fetcher_factory = std::move(dhcp_fetcher_factory_); On 2017/06/13 17:02:22, mmenke wrote: > On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > > Snort. That's cute. Is there a (presumably new) test that would catch this? > > > > This is caught by integration tests...If you look at set 4, a rather large > number number of ChromeOS browser tests failed, as well as a couple IOThread > unittests. They all fail on a DCHECK in CreateProxyServiceUsingMojoFactory > (https://cs.chromium.org/chromium/src/net/proxy/proxy_service_mojo.cc?type=cs&...). > I'm not sure if we have any test that actually depends on a functional > DhcpFetcherFactory being set (Either a real one or a mock one). > > That having been said, there are no URLRequestContextBuilderMojo unit tests that > catch this, and I'm not sure we really want unit tests that check every > individual setter in URLRequestContextBuilder[Mojo]. I'm torn here, but I sorta think that if we're going to expose an API we should test it. But it's at "todo" level; I'm certainly not going to ask you to do it for this CL. > That having been said, I'd love it if we switched to using custom > URLRequestContextBulders in net unittests instead of > TestURLRequestContextBuilders, to better exercise this code. And this fix was > the thing that inspired my comment on unit tests. How would you feel (honest question; I'm not clear this isn't excessive and I'm not asking for it, just musing) about making sure there are unit tests for everything that you exercise from io_thread.cc?
On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > I'm good with this CL as written, so LGTM (though there are a couple of things I > explore in the comments below). > > One request for a separate CL (or this one if you want--don't care): Can you > document the inheritance relationship of URLRequestContextBuilder to its > subclasses in the docs in that header file? In URLRequestContextBuilder's header file, or the subclass's? https://codereview.chromium.org/2929153002/diff/220001/chromeos/network/dhcp_... File chromeos/network/dhcp_proxy_script_fetcher_factory_chromeos.h (right): https://codereview.chromium.org/2929153002/diff/220001/chromeos/network/dhcp_... chromeos/network/dhcp_proxy_script_fetcher_factory_chromeos.h:23: // Figure out a way forward there. On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > This isn't a problem with the Win-specific proxy script fetcher? On Windows, the classes are entirely within net/, so they're not a problem. The issue is that the ChromeOS fetcher is dependent on a bunch of ChromeOS network-related classes that live in chrome/ and are all interdependent and have global state. https://codereview.chromium.org/2929153002/diff/220001/net/proxy/dhcp_proxy_s... File net/proxy/dhcp_proxy_script_fetcher_factory.cc (left): https://codereview.chromium.org/2929153002/diff/220001/net/proxy/dhcp_proxy_s... net/proxy/dhcp_proxy_script_fetcher_factory.cc:48: bool DhcpProxyScriptFetcherFactory::IsSupported() { On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > I won't say "Never has so many characters been expended to so little purpose", > but wow. Yay for deleting all this. Acknowledged. https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder_mojo.h (left): https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder_mojo.h:37: dhcp_fetcher_factory = std::move(dhcp_fetcher_factory_); On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > On 2017/06/13 17:02:22, mmenke wrote: > > On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > > > Snort. That's cute. Is there a (presumably new) test that would catch > this? > > > > > > > This is caught by integration tests...If you look at set 4, a rather large > > number number of ChromeOS browser tests failed, as well as a couple IOThread > > unittests. They all fail on a DCHECK in CreateProxyServiceUsingMojoFactory > > > (https://cs.chromium.org/chromium/src/net/proxy/proxy_service_mojo.cc?type=cs&...). > > I'm not sure if we have any test that actually depends on a functional > > DhcpFetcherFactory being set (Either a real one or a mock one). > > > > That having been said, there are no URLRequestContextBuilderMojo unit tests > that > > catch this, and I'm not sure we really want unit tests that check every > > individual setter in URLRequestContextBuilder[Mojo]. > > I'm torn here, but I sorta think that if we're going to expose an API we should > test it. But it's at "todo" level; I'm certainly not going to ask you to do it > for this CL. I really don't want to have 5 sets of tests for everything (unit tests, URLRequest-layer tests, URLRequestContextBuilder layer tests, NetworkService tests, and chrome/content level integration tests) that just make sure configuring each thing works. Seven sets if you throw in both incognito and IOThread configuration. > > That having been said, I'd love it if we switched to using custom > > URLRequestContextBulders in net unittests instead of > > TestURLRequestContextBuilders, to better exercise this code. And this fix was > > the thing that inspired my comment on unit tests. > > How would you feel (honest question; I'm not clear this isn't excessive and I'm > not asking for it, just musing) about making sure there are unit tests for > everything that you exercise from io_thread.cc? I'd really like to have tests at the NetworkService layer instead, which what consumers should hopefully be using (And I don't want NetworkService and URLRequestContextBuilder tests for everything they can set). I'm all for tests, but at some point, have to actually write some production code. :) Testing at this layer also requires a lot of set up - DHCP scripts, injecting ProxyConfigs, injecting ProxyConfigServices, mocking out DhcpScriptFetcherFactories (And DhcpScriptFetchers), etc. It's not just do the same thing a bunch of times with minor variations. That's not to say I don't think we should have any tests - I did add one for the basic mojo resolver path, just not for the DhcpProxyScriptFetcher logic.
On 2017/06/15 19:33:24, mmenke wrote: > On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > > I'm good with this CL as written, so LGTM (though there are a couple of things > I > > explore in the comments below). > > > > One request for a separate CL (or this one if you want--don't care): Can you > > document the inheritance relationship of URLRequestContextBuilder to its > > subclasses in the docs in that header file? > > In URLRequestContextBuilder's header file, or the subclass's? URLRequestContextBuilder. I want something that if someone were to say "I want to specialize what URLRequestContextBuilder does in a subclass; how would I do that?" it would tell them or at least get them started/let them know they were SOL. > > https://codereview.chromium.org/2929153002/diff/220001/chromeos/network/dhcp_... > File chromeos/network/dhcp_proxy_script_fetcher_factory_chromeos.h (right): > > https://codereview.chromium.org/2929153002/diff/220001/chromeos/network/dhcp_... > chromeos/network/dhcp_proxy_script_fetcher_factory_chromeos.h:23: // Figure out > a way forward there. > On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > > This isn't a problem with the Win-specific proxy script fetcher? > > On Windows, the classes are entirely within net/, so they're not a problem. The > issue is that the ChromeOS fetcher is dependent on a bunch of ChromeOS > network-related classes that live in chrome/ and are all interdependent and have > global state. > > https://codereview.chromium.org/2929153002/diff/220001/net/proxy/dhcp_proxy_s... > File net/proxy/dhcp_proxy_script_fetcher_factory.cc (left): > > https://codereview.chromium.org/2929153002/diff/220001/net/proxy/dhcp_proxy_s... > net/proxy/dhcp_proxy_script_fetcher_factory.cc:48: bool > DhcpProxyScriptFetcherFactory::IsSupported() { > On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > > I won't say "Never has so many characters been expended to so little purpose", > > but wow. Yay for deleting all this. > > Acknowledged. > > https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... > File net/url_request/url_request_context_builder_mojo.h (left): > > https://codereview.chromium.org/2929153002/diff/220001/net/url_request/url_re... > net/url_request/url_request_context_builder_mojo.h:37: dhcp_fetcher_factory = > std::move(dhcp_fetcher_factory_); > On 2017/06/15 19:13:30, Randy Smith (Not in Mondays) wrote: > > On 2017/06/13 17:02:22, mmenke wrote: > > > On 2017/06/13 16:42:15, Randy Smith (Not in Mondays) wrote: > > > > Snort. That's cute. Is there a (presumably new) test that would catch > > this? > > > > > > > > > > This is caught by integration tests...If you look at set 4, a rather large > > > number number of ChromeOS browser tests failed, as well as a couple IOThread > > > unittests. They all fail on a DCHECK in CreateProxyServiceUsingMojoFactory > > > > > > (https://cs.chromium.org/chromium/src/net/proxy/proxy_service_mojo.cc?type=cs&...). > > > I'm not sure if we have any test that actually depends on a functional > > > DhcpFetcherFactory being set (Either a real one or a mock one). > > > > > > That having been said, there are no URLRequestContextBuilderMojo unit tests > > that > > > catch this, and I'm not sure we really want unit tests that check every > > > individual setter in URLRequestContextBuilder[Mojo]. > > > > I'm torn here, but I sorta think that if we're going to expose an API we > should > > test it. But it's at "todo" level; I'm certainly not going to ask you to do > it > > for this CL. > > I really don't want to have 5 sets of tests for everything (unit tests, > URLRequest-layer tests, URLRequestContextBuilder layer tests, NetworkService > tests, and chrome/content level integration tests) that just make sure > configuring each thing works. Seven sets if you throw in both incognito and > IOThread configuration. > > > > That having been said, I'd love it if we switched to using custom > > > URLRequestContextBulders in net unittests instead of > > > TestURLRequestContextBuilders, to better exercise this code. And this fix > was > > > the thing that inspired my comment on unit tests. > > > > How would you feel (honest question; I'm not clear this isn't excessive and > I'm > > not asking for it, just musing) about making sure there are unit tests for > > everything that you exercise from io_thread.cc? > > I'd really like to have tests at the NetworkService layer instead, which what > consumers should hopefully be using (And I don't want NetworkService and > URLRequestContextBuilder tests for everything they can set). I'm all for tests, > but at some point, have to actually write some production code. :) > > Testing at this layer also requires a lot of set up - DHCP scripts, injecting > ProxyConfigs, injecting ProxyConfigServices, mocking out > DhcpScriptFetcherFactories (And DhcpScriptFetchers), etc. It's not just do the > same thing a bunch of times with minor variations. > > That's not to say I don't think we should have any tests - I did add one for the > basic mojo resolver path, just not for the DhcpProxyScriptFetcher logic.
I'll think a bit more about testing the builder (Though making a TestURLRequestContextBuilder and using it instead of TestURLRequestContext appeals more to me than builder-specific unittests), but going to go ahead and land this as-is.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mmenke@chromium.org changed reviewers: + stevenjb@chromium.org
[+stevenjb]: Please review chromeos/. Thanks!
FWIW, it seems like the io_thread.cc refactoring should be separated out, but the chromeos/ changes lgtm.
On 2017/06/15 20:21:37, stevenjb wrote: > FWIW, it seems like the io_thread.cc refactoring should be separated out, but > the chromeos/ changes lgtm. None of the other changes actually do anything, and independently, don't even really affect the build. And, admittedly, I got tired of incremental changes. This is the end of a chain of 10+ CLs. I'll land together, and take my chances.
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": 220001, "attempt_start_ts": 1497558376144240,
"parent_rev": "05e4e6cc84b3bf4ce79bf69c885fa29833f73551", "commit_rev":
"14085ad54d50252bcadd1b1d7dc4a0bca2c9c5b3"}
Message was sent while issue was closed.
Description was changed from ========== Use URLRequestContextBuilderMojo to create the SystemURLRequestContext. Also adds a DhcpProxyScriptFetcherFactoryChromeos, to inject into the URLRequestContextBuilder on ChromeOS (The actual fetcher must be created after the URLRequestContext, and the factory avoids the whole chicken/egg thing). BUG=715695 ========== to ========== Use URLRequestContextBuilderMojo to create the SystemURLRequestContext. Also adds a DhcpProxyScriptFetcherFactoryChromeos, to inject into the URLRequestContextBuilder on ChromeOS (The actual fetcher must be created after the URLRequestContext, and the factory avoids the whole chicken/egg thing). BUG=715695 Review-Url: https://codereview.chromium.org/2929153002 Cr-Commit-Position: refs/heads/master@{#479854} Committed: https://chromium.googlesource.com/chromium/src/+/14085ad54d50252bcadd1b1d7dc4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://chromium.googlesource.com/chromium/src/+/14085ad54d50252bcadd1b1d7dc4... |
