|
|
Created:
3 years, 6 months ago by mmenke Modified:
3 years, 6 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove more SystemURLRequestContext members into its ContextStorage
This is needed before switching over to URLRequestContextBuilder.
BUG=715695
Review-Url: https://codereview.chromium.org/2910643003
Cr-Commit-Position: refs/heads/master@{#477782}
Committed: https://chromium.googlesource.com/chromium/src/+/c2af5956cc783ccf67f93c3028f892ce489a2f28
Patch Set 1 #Patch Set 2 : Fix #Patch Set 3 : HostResolver, too #Patch Set 4 : Merge #Patch Set 5 : Fix DNS #Patch Set 6 : Merge? #Patch Set 7 : Fix? #Patch Set 8 : Fix Merge #
Total comments: 6
Patch Set 9 : Merge #Patch Set 10 : Response #
Messages
Total messages: 64 (55 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_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
Description was changed from ========== Move IOThread's cert and auth stuff into the system context's storage. This is needed before switching over to URLRequestContextBuilder. We may need to move this stuff back out before switching ProfileIOData's URLRequestContext over to using a URLRequestContextBuilder, but may not. First pass, this is simpler than giving URLRequestContextBuilder some APIs that don't take overship of these objects. BUG=715695 ========== to ========== Move IOThread's cert and auth stuff into the system context's storage. This is needed before switching over to URLRequestContextBuilder. BUG=715695 ==========
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Move IOThread's cert and auth stuff into the system context's storage. This is needed before switching over to URLRequestContextBuilder. BUG=715695 ========== to ========== Move more SystemURLRequestContext members in its ContextStorage This is needed before switching over to URLRequestContextBuilder. BUG=715695 ==========
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_...)
Description was changed from ========== Move more SystemURLRequestContext members in its ContextStorage This is needed before switching over to URLRequestContextBuilder. BUG=715695 ========== to ========== Move more SystemURLRequestContext members into its ContextStorage This is needed before switching over to URLRequestContextBuilder. BUG=715695 ==========
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
I think I'll need to do some more Mojo/V8 proxy logic refactoring, but this is probably the last CL I'll end you before switching over to URLRequestContextBuilder.
On 2017/05/26 20:41:17, mmenke wrote: > I think I'll need to do some more Mojo/V8 proxy logic refactoring, but this is > probably the last CL I'll end you before switching over to > URLRequestContextBuilder. Oops, this one looks to have real test failures. Don't both reviewing this one until I have them resolved (I'll tell you when that happens).
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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 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...
Patchset #8 (id:140001) has been deleted
rdsmith: PTAL (No rush on this - fine if you don't get to it until next Tuesday).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
LGTM--all comments below I'm happy to go with your judgement if it disagrees with the comment. https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:587: RegisterSTHObserver(ct_tree_tracker_.get()); Why not move this with the ct_verifier code move? It looks to be a direct data dependency; why not keep it clustered with that code? https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:851: CreateDefaultAuthHandlerFactory()); Would you be willing to either put in a comment that this call must occur after the creation of the global host resolver above, or make it implicit by giving the host resolver as an argument to the function? https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.h:238: std::unique_ptr<net::HttpAuthHandlerRegistryFactory> Why change to the subclass type instead of just sticking with HttpAuthHandlerFactory? (Don't care, just curious.)
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...
https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:587: RegisterSTHObserver(ct_tree_tracker_.get()); On 2017/06/07 19:28:17, Randy Smith (Not in Mondays) wrote: > Why not move this with the ct_verifier code move? It looks to be a direct data > dependency; why not keep it clustered with that code? Because these aren't owned by (or pointed to by) the URLRequestContext, and I want to share URLRequestContextBuilder code between this and ProfileIOData. If these remain shared, I'll either need special case code there for the first URLRequestContext built, or I'll need to make all shared objects in a separate function during set up (While the system URLRequestContext is created first, once we switch to using a network service, that could change, if we construct them on first use when using the NetworkService). If I use a separate function, it's more likely I'll move shared objects back here, and make this the separate function. That does perhaps argue I should be doing a design document up front to decide some of this stuff, but there's just so much cruft that I've been reluctant to do it, and the path is grungy enough that I'mt not sure how well I'd be able to keep to the plan, anyways. https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:851: CreateDefaultAuthHandlerFactory()); On 2017/06/07 19:28:17, Randy Smith (Not in Mondays) wrote: > Would you be willing to either put in a comment that this call must occur after > the creation of the global host resolver above, or make it implicit by giving > the host resolver as an argument to the function? Did the latter (Was planning to do it in the next CL, anyway, when we won't even have a URLRequestContext to steal things from) https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/2910643003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.h:238: std::unique_ptr<net::HttpAuthHandlerRegistryFactory> On 2017/06/07 19:28:17, Randy Smith (Not in Mondays) wrote: > Why change to the subclass type instead of just sticking with > HttpAuthHandlerFactory? (Don't care, just curious.) Just because an "HttpAuthHandlerRegistryFactory" is not clearly an "HttpAuthHandlerFactory" (It sounds like it creates HttpAuthHandlerRegistries), and I thought this made it a little clearer it was, though it's certainly debatable. Anyhow, don't have strong opinion here, so I changed it.
The CQ bit was unchecked by mmenke@chromium.org
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2910643003/#ps200001 (title: "Response")
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
CQ cannot get SignCLA result. Please try later.
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": 200001, "attempt_start_ts": 1496871215038820, "parent_rev": "466d40a9545e8124e2ce2561146d46aceec23420", "commit_rev": "9667358ddfbcc627fa35bad406bddf3b36404c48"}
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496871215038820, "parent_rev": "7576773367d39acbaa5e8011722803605354d047", "commit_rev": "c2af5956cc783ccf67f93c3028f892ce489a2f28"}
Message was sent while issue was closed.
Description was changed from ========== Move more SystemURLRequestContext members into its ContextStorage This is needed before switching over to URLRequestContextBuilder. BUG=715695 ========== to ========== Move more SystemURLRequestContext members into its ContextStorage This is needed before switching over to URLRequestContextBuilder. BUG=715695 Review-Url: https://codereview.chromium.org/2910643003 Cr-Commit-Position: refs/heads/master@{#477782} Committed: https://chromium.googlesource.com/chromium/src/+/c2af5956cc783ccf67f93c3028f8... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/chromium/src/+/c2af5956cc783ccf67f93c3028f8... |