Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939433004:
Move common main URLRequestContext setup code to ProfileIOData (Closed)
|
Created:
3 years, 6 months ago by mmenke Modified:
3 years, 5 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove common main URLRequestContext setup code to ProfileIOData
from subclasses.
Also move ownership of some ProfileIOData components from globals
to the URLRequestContextStorage of the main URLRequestContext.
BUG=715695
Review-Url: https://codereview.chromium.org/2939433004
Cr-Commit-Position: refs/heads/master@{#482786}
Committed: https://chromium.googlesource.com/chromium/src/+/5167a68813ef5edb1445e205e5489059285e3885
Patch Set 1 #Patch Set 2 : Merge #Patch Set 3 : Fix #Patch Set 4 : Fix, expand CL slightly #Patch Set 5 : Remove inaccurate old comment #Patch Set 6 : Fix #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 39 (27 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_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...
Description was changed from ========== check BUG= ========== to ========== Move common main URLRequestContext setup code to ProfileIOData from subclasses. Also move ownership of some ProfileIOData components from globals to the URLRequestContextStorage of the main URLRequestContext. 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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: 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...
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
Re-running trybots now, but looks like the 4 failures causing me to not send this out on Tuesday were all due to flake. :( https://codereview.chromium.org/2939433004/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (left): https://codereview.chromium.org/2939433004/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:660: if (cert_transparency_verifier_) { All of these null checks were basically checking the same thing - if we'd set up the main_request_context_ (In no case do we set it up but leave any of these objects as NULL, except the ReportingService, which we don't use in incognito, but all we need to do there is delete it, anyways)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Randy: I'm going to be working on a couple regressions, so feel free to punt my refactors for a day or two, but please give my fixes higher priority (I don't think either of the fixes I'll be sending your way will be complicated, anyways, but we'll see).
On 2017/06/20 16:21:25, mmenke wrote: > Randy: I'm going to be working on a couple regressions, so feel free to punt my > refactors for a day or two, but please give my fixes higher priority (I don't > think either of the fixes I'll be sending your way will be complicated, anyways, > but we'll see). Actually, don't bother to review this. The delegate approach is looking uglier to me than I expected. Instead, I think I'll use a single callback that takes a URLRequestContextBuilder, and tell the in-process network service to set up a NetworkContext as normal, but invoke that callback on the URLRequestContextBuilder. Nice thing about the callback approach is it lets us bind arguments (Like the interceptor list), and largely keep current setup logic.
Message was sent while issue was closed.
On 2017/06/22 19:58:03, mmenke wrote: > On 2017/06/20 16:21:25, mmenke wrote: > > Randy: I'm going to be working on a couple regressions, so feel free to punt > my > > refactors for a day or two, but please give my fixes higher priority (I don't > > think either of the fixes I'll be sending your way will be complicated, > anyways, > > but we'll see). > > Actually, don't bother to review this. The delegate approach is looking uglier > to me than I expected. Instead, I think I'll use a single callback that takes a > URLRequestContextBuilder, and tell the in-process network service to set up a > NetworkContext as normal, but invoke that callback on the > URLRequestContextBuilder. Nice thing about the callback approach is it lets us > bind arguments (Like the interceptor list), and largely keep current setup > logic. Proof that if you ignore a problem, sometimes it *does* go away! :-} (Despite that, apologies for taking so long to get back to this--Network service bot rotation + bug triage + other reviews have taken most of my time.)
Message was sent while issue was closed.
On 2017/06/22 20:00:55, Randy Smith (Not in Mondays) wrote: > On 2017/06/22 19:58:03, mmenke wrote: > > On 2017/06/20 16:21:25, mmenke wrote: > > > Randy: I'm going to be working on a couple regressions, so feel free to > punt > > my > > > refactors for a day or two, but please give my fixes higher priority (I > don't > > > think either of the fixes I'll be sending your way will be complicated, > > anyways, > > > but we'll see). > > > > Actually, don't bother to review this. The delegate approach is looking > uglier > > to me than I expected. Instead, I think I'll use a single callback that takes > a > > URLRequestContextBuilder, and tell the in-process network service to set up a > > NetworkContext as normal, but invoke that callback on the > > URLRequestContextBuilder. Nice thing about the callback approach is it lets > us > > bind arguments (Like the interceptor list), and largely keep current setup > > logic. > > Proof that if you ignore a problem, sometimes it *does* go away! :-} > > (Despite that, apologies for taking so long to get back to this--Network service > bot rotation + bug triage + other reviews have taken most of my time.) Not a problem. I've had plenty to do this week.
Message was sent while issue was closed.
Oops...Sorry. Please do review this one, it's the one on top of this that you shouldn't review. Too many CLs (I can't count up to 3)
On 2017/06/22 20:24:39, mmenke wrote: > Oops...Sorry. Please do review this one, it's the one on top of this that you > shouldn't review. Too many CLs (I can't count up to 3) This one is back on my list. I have removed 2934153002 from my queue; let me know if that's not the one you wanted me to nix.
On 2017/06/22 20:31:36, Randy Smith (Not in Mondays) wrote: > On 2017/06/22 20:24:39, mmenke wrote: > > Oops...Sorry. Please do review this one, it's the one on top of this that you > > shouldn't review. Too many CLs (I can't count up to 3) > > This one is back on my list. I have removed 2934153002 from my queue; let me > know if that's not the one you wanted me to nix. That's indeed the one not to review. I also closed it.
LGTM.
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...
On 2017/06/27 21:27:13, Randy Smith (Not in Mondays) wrote: > LGTM. Thanks! Think I'm just one or two CLs from switching over to the URLRequestContextBuilder, though I may put that off until after branch (3 weeks away, but may be a bit short staffed, and I'm thinking I may take a break and work on other things, anyways like getting back to closing sockets on network changes).
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498598877622610,
"parent_rev": "af7a1e8f70ec8345321efd2dc274a2c9a55ad97a", "commit_rev":
"5167a68813ef5edb1445e205e5489059285e3885"}
Message was sent while issue was closed.
Description was changed from ========== Move common main URLRequestContext setup code to ProfileIOData from subclasses. Also move ownership of some ProfileIOData components from globals to the URLRequestContextStorage of the main URLRequestContext. BUG=715695 ========== to ========== Move common main URLRequestContext setup code to ProfileIOData from subclasses. Also move ownership of some ProfileIOData components from globals to the URLRequestContextStorage of the main URLRequestContext. BUG=715695 Review-Url: https://codereview.chromium.org/2939433004 Cr-Commit-Position: refs/heads/master@{#482786} Committed: https://chromium.googlesource.com/chromium/src/+/5167a68813ef5edb1445e205e548... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5167a68813ef5edb1445e205e548... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
