Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475143002:
Defer DRP start up until after the context getter is created (Closed)
DescriptionDefer DRP start up until after the context getter is created
DRP initialization will be defered until the main request context is
created. Currently, DRP will force the create of the context getter
causing unneccesary early evaluation.
BUG=662488
Committed: https://crrev.com/ff0a4a3f4f165290c3da7902a67d98434a49e7e3
Cr-Commit-Position: refs/heads/master@{#430676}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 23 (12 generated)
The CQ bit was checked by ryansturm@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...)
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
I think this works, one question, though. https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl_io_data.cc:249: db_task_runner); What happens if we start making requests on the IO thread before this is done?
ryansturm@chromium.org changed reviewers: + bengr@chromium.org
I'm adding bengr as a reviewer as well to get his thoughts. https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl_io_data.cc:249: db_task_runner); On 2016/11/04 17:25:10, mmenke wrote: > What happens if we start making requests on the IO thread before this is done? We still wait for config to be set up in DRP, which takes a couple thread hops or a fetch of a new config either way. Delaying this by a slight amount should be handled.
Description was changed from ========== Defer DRP start up until after the context getter is created DRP initialization will be defered until the main request context is created. Currently, DRP will force the create of the context getter causing unneccesary early evaluation. BUG= ========== to ========== Defer DRP start up until after the context getter is created DRP initialization will be defered until the main request context is created. Currently, DRP will force the create of the context getter causing unneccesary early evaluation. BUG=662488 ==========
LGTM https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl_io_data.cc:207: BrowserThread::GetTaskRunnerForThread(BrowserThread::UI))); I'm not objecting, but is there a reason you're moving this?
What motivated this CL? https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl_io_data.cc:207: BrowserThread::GetTaskRunnerForThread(BrowserThread::UI))); On 2016/11/04 19:02:44, mmenke wrote: > I'm not objecting, but is there a reason you're moving this? Likewise, I'm curious why you're moving this.
https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2475143002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl_io_data.cc:207: BrowserThread::GetTaskRunnerForThread(BrowserThread::UI))); On 2016/11/07 18:13:40, bengr wrote: > On 2016/11/04 19:02:44, mmenke wrote: > > I'm not objecting, but is there a reason you're moving this? > > Likewise, I'm curious why you're moving this. I talked to mmenke about this, and he's actually asking a slightly different question then you. He is asking why the previews line moved above DRP, and you are asking why half of DRP initialization moved down. The basic idea is that calling profile_->GetRequestContext() here causes IO thread tasks to potentially start before this method finishes. And the Offline Page URL Request interceptor was sometimes getting a nullptr due to io_data_->previews_io_data_ not being set yet. To answer mmenke's question, there's not a strong reason to move previews above DRP here, but I'd prefer previews be created first here to prevent possible similar problems if DRP code changes again.
lgtm
The CQ bit was checked by ryansturm@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: linux_chromium_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 ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Defer DRP start up until after the context getter is created DRP initialization will be defered until the main request context is created. Currently, DRP will force the create of the context getter causing unneccesary early evaluation. BUG=662488 ========== to ========== Defer DRP start up until after the context getter is created DRP initialization will be defered until the main request context is created. Currently, DRP will force the create of the context getter causing unneccesary early evaluation. BUG=662488 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Defer DRP start up until after the context getter is created DRP initialization will be defered until the main request context is created. Currently, DRP will force the create of the context getter causing unneccesary early evaluation. BUG=662488 ========== to ========== Defer DRP start up until after the context getter is created DRP initialization will be defered until the main request context is created. Currently, DRP will force the create of the context getter causing unneccesary early evaluation. BUG=662488 Committed: https://crrev.com/ff0a4a3f4f165290c3da7902a67d98434a49e7e3 Cr-Commit-Position: refs/heads/master@{#430676} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/ff0a4a3f4f165290c3da7902a67d98434a49e7e3 Cr-Commit-Position: refs/heads/master@{#430676} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
