Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1952423002:
JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. (Closed)
|
Created:
4 years, 7 months ago by Zhongyi Shi Modified:
4 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@Job_Controller_1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionJobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. HttpStreamFactoryImpl::JobController will take over scheduling between main job and alternative job.
BUG=605398
Committed: https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5
Cr-Commit-Position: refs/heads/master@{#407846}
Patch Set 1 #
Total comments: 14
Patch Set 2 : sync to JobController1, and move DoWaitForJob to JobController #Patch Set 3 : move DoWaitForJob back, resume Job after delay in JobController #
Total comments: 14
Patch Set 4 : rebase after JobController 1 landed #Patch Set 5 : add unittests #Patch Set 6 : move resume logic up to controller #
Total comments: 22
Patch Set 7 : Remove ResumeAfterDelay #
Total comments: 17
Patch Set 8 : Move resume logic up to controller #Patch Set 9 : More tests with Resume logic #
Total comments: 28
Patch Set 10 : addressing nits #Patch Set 11 : use unmocked Job::Start() and asyncronous Proxy resolution in tests #
Total comments: 3
Patch Set 12 : Remove mocked Start() for TestJob #Patch Set 13 : use async proxy resolver for unittests #Patch Set 14 : rebase with master, fix tests #Patch Set 15 : rebase onto origin/master #Patch Set 16 : fix a include typo #Messages
Total messages: 53 (25 generated)
Description was changed from ========== JobController 2: Remove reference between Jobs. JobController will determine whether or not to resume the other job. BUG=605398 ========== to ========== JobController 2: Remove reference between Jobs. JobController will determine whether or not to resume the other job. BUG=605398 ==========
zhongyi@chromium.org changed reviewers: + rch@chromium.org
This CL removes cross reference between two Jobs. Next step to do is to move MarkAlternativeServiceBroken to JobController. Remove other_job_status_ in Job. New tests might be added in next step or current tests will be edited to enforce checking.
Looking good. Let's get #1 sorted out before going to far on this CL, but a few thoughts... https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.h:66: NON_ALTERNATIVE, How about MAIN? https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:231: job_controller_->OnJobDeletion(this); The controller owns the job, right? That means the controller is deleting the job. So that seems to suggest that the controller already knows that the job is being deleted. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:896: DCHECK((job_type_ == ALTERNATIVE || !job_controller_->racing())); it's not clear to me why the job needs to perform this check since this seems like the responsibility of the controller. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:988: base::TimeDelta delay = base::TimeDelta(); nit: I think you can remove the = ...; https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:115: friend class JobController; Can we get away without this friendship? https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:486: main_job_ = NULL; nullptr is cool now, NULL is *so* 2015 :> https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.h:178: bool racing_; If this is about the alt job blocking the main job, it seems like blocking_ would be a better name than racing_.
Sync to JobController 1. Move some more scheduling in Job::DoWaitForJob() to JobController. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.h:66: NON_ALTERNATIVE, On 2016/05/06 21:33:14, Ryan Hamilton wrote: > How about MAIN? Done. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:231: job_controller_->OnJobDeletion(this); On 2016/05/06 21:33:14, Ryan Hamilton wrote: > The controller owns the job, right? That means the controller is deleting the > job. So that seems to suggest that the controller already knows that the job is > being deleted. The intention was to remove references to the deleted Job. But since we had fewer ref (just bound_job_), yeah, we don't need this anymore. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:896: DCHECK((job_type_ == ALTERNATIVE || !job_controller_->racing())); On 2016/05/06 21:33:14, Ryan Hamilton wrote: > it's not clear to me why the job needs to perform this check since this seems > like the responsibility of the controller. Moved to JobController! https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:988: base::TimeDelta delay = base::TimeDelta(); On 2016/05/06 21:33:14, Ryan Hamilton wrote: > nit: I think you can remove the = ...; Done. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:115: friend class JobController; On 2016/05/06 21:33:14, Ryan Hamilton wrote: > Can we get away without this friendship? Done. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:486: main_job_ = NULL; On 2016/05/06 21:33:14, Ryan Hamilton wrote: > nullptr is cool now, NULL is *so* 2015 :> Acknowledged. https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.h:178: bool racing_; On 2016/05/06 21:33:14, Ryan Hamilton wrote: > If this is about the alt job blocking the main job, it seems like blocking_ > would be a better name than racing_. Done.
zhongyi@chromium.org changed reviewers: + rdsmith@chromium.org
Move DoWaitForJob back to Job.
Just a quick heads up that I won't be able to look at this until later in the
week--I've been attacked by a lot of reviews, and I need to make some progress
on my own CLs. I'm aiming for Thursday; if that's going to be a problem, free
free to take me off the CL and rely purely on Ryan. Sorry :-{.
So excited by this CL! https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:738: job_controller_->MaybeResumeOtherJob(this, base::TimeDelta()); As discussed offline, I *think* we can move the logic of restarting the other job "now" from the various error conditions in the job.cc up to the controller.cc in the various OnWhatever(job) methods (like OnStreamFailed()) https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:827: if (job_controller_->ResumeJobWithDelay(this)) { I'm quite happy with the logic here. Yay! It'd be great if we could find a more obvious name for this method. In particular "WithDelay" is a bit surprising because there is no delay argument passed in. Also, from the name it's not obvious why this method would return true/false. I wonder if a better name might be ShouldWait()? (Also, we might be able to remove "ForJob"/"FOR_JOB" from the names of the states/methods since the fact that this job is waiting for another job is hidden by the controller. https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:830: } else { nit: no need for else since if() returned. https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:932: job_controller_->MaybeResumeOtherJob(this, delay); Looking at this line (and the line below in this same method) i take it this is where we want to un-block the other job. When the other job is blocked, the main job may need to wait, which is why we're passing in the delay here. Do I have this right? I wonder if we actually need to handle the case where rv != ERR_IO_PENDING here. That represents an error, which we *think* will result in the other job resuming via OnStreamFailed. https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1597: MaybeMarkAlternativeServiceBroken(); Can we move all of this logic up to the controller? https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:472: } else if (job != main_job_.get() && job != alternative_job_.get()) { nit: I might move this to a DCHECK() at the top of the method: DCHECK(job == alternative_job_.get() || job == main_job_.get()); https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.h:119: wait_time_ = delay; nit: wait_time_for_main_job_ and set_wait_time_for_main_job() https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.h:213: bool blocking_; Can you expand this comment a bit? Under what circumstances would we have both jobs, but not be blocking? https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.h:214: base::TimeDelta wait_time_; Comment?
What's the current status of this CL? My initial reaction looking through it is that there's still a fair number of references between the jobs (as evidenced by the existence of the MaybeResumeOtherJob method and the job_type_ internal variable). Are there plans to modify it on that basis, or should I review it as currently written? (My ideal would be a Job class that didn't know about the existence of other jobs at all, and provided enough information when it succeeded/failed for its owner to figure out what should be done with other jobs. If you're headed in that direction, I'd like to wait and look at the next PS; if it isn't possible or you'd like suggestions as to how to get there, I should probably review this one. Just let me know.) One other question: What revision is this PS branched from? "git cl patch" failed for me, I presume because my checkout isn't at the right revision. Thanks! https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:738: job_controller_->MaybeResumeOtherJob(this, base::TimeDelta()); On 2016/05/17 22:14:09, Ryan Hamilton wrote: > As discussed offline, I *think* we can move the logic of restarting the other > job "now" from the various error conditions in the job.cc up to the > controller.cc in the various OnWhatever(job) methods (like OnStreamFailed()) +1; one of my reactions to this CL is that we're not really removing the cross-job references if we're calling methods on JobController that refer to the other job.
On 2016/05/25 21:22:22, Randy Smith - Not in Fridays wrote: > What's the current status of this CL? My initial reaction looking through it is > that there's still a fair number of references between the jobs (as evidenced by > the existence of the MaybeResumeOtherJob method and the job_type_ internal > variable). Are there plans to modify it on that basis, or should I review it as > currently written? > > (My ideal would be a Job class that didn't know about the existence of other > jobs at all, and provided enough information when it succeeded/failed for its > owner to figure out what should be done with other jobs. If you're headed in > that direction, I'd like to wait and look at the next PS; if it isn't possible > or you'd like suggestions as to how to get there, I should probably review this > one. Just let me know.) > > One other question: What revision is this PS branched from? "git cl patch" > failed for me, I presume because my checkout isn't at the right revision. > > Thanks! > > https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... > net/http/http_stream_factory_impl_job.cc:738: > job_controller_->MaybeResumeOtherJob(this, base::TimeDelta()); > On 2016/05/17 22:14:09, Ryan Hamilton wrote: > > As discussed offline, I *think* we can move the logic of restarting the other > > job "now" from the various error conditions in the job.cc up to the > > controller.cc in the various OnWhatever(job) methods (like OnStreamFailed()) > > +1; one of my reactions to this CL is that we're not really removing the > cross-job references if we're calling methods on JobController that refer to the > other job. Sorry, I didn't update here. I was working on the unittests for JobController 1. And will wrap up everything for JobController 1 before returning to this CL.
On 2016/05/26 05:01:07, Zhongyi Shi wrote: > On 2016/05/25 21:22:22, Randy Smith - Not in Fridays wrote: > > What's the current status of this CL? My initial reaction looking through it > is > > that there's still a fair number of references between the jobs (as evidenced > by > > the existence of the MaybeResumeOtherJob method and the job_type_ internal > > variable). Are there plans to modify it on that basis, or should I review it > as > > currently written? > > > > (My ideal would be a Job class that didn't know about the existence of other > > jobs at all, and provided enough information when it succeeded/failed for its > > owner to figure out what should be done with other jobs. If you're headed in > > that direction, I'd like to wait and look at the next PS; if it isn't possible > > or you'd like suggestions as to how to get there, I should probably review > this > > one. Just let me know.) > > > > One other question: What revision is this PS branched from? "git cl patch" > > failed for me, I presume because my checkout isn't at the right revision. > > > > Thanks! > > > > > https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... > > File net/http/http_stream_factory_impl_job.cc (right): > > > > > https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... > > net/http/http_stream_factory_impl_job.cc:738: > > job_controller_->MaybeResumeOtherJob(this, base::TimeDelta()); > > On 2016/05/17 22:14:09, Ryan Hamilton wrote: > > > As discussed offline, I *think* we can move the logic of restarting the > other > > > job "now" from the various error conditions in the job.cc up to the > > > controller.cc in the various OnWhatever(job) methods (like OnStreamFailed()) > > > > +1; one of my reactions to this CL is that we're not really removing the > > cross-job references if we're calling methods on JobController that refer to > the > > other job. > > Sorry, I didn't update here. I was working on the unittests for JobController 1. > And will wrap up everything for JobController 1 before returning to this CL. Awesome; I'll wait for your ping.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:60001) has been deleted
Description was changed from ========== JobController 2: Remove reference between Jobs. JobController will determine whether or not to resume the other job. BUG=605398 ========== to ========== JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. HttpStreamFactoryImpl::JobController will take over scheduling between main job and alternative job. BUG=605398 ==========
Patchset #5 (id:140001) has been deleted
Moving the resume logic to JobController unless we get an ERR_IO_PENDING for DoInitConnection(). PTAL! https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:827: if (job_controller_->ResumeJobWithDelay(this)) { On 2016/05/17 22:14:09, Ryan Hamilton wrote: > I'm quite happy with the logic here. Yay! > > It'd be great if we could find a more obvious name for this method. In > particular "WithDelay" is a bit surprising because there is no delay argument > passed in. Also, from the name it's not obvious why this method would return > true/false. I wonder if a better name might be ShouldWait()? > > (Also, we might be able to remove "ForJob"/"FOR_JOB" from the names of the > states/methods since the fact that this job is waiting for another job is hidden > by the controller. Done. https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:830: } else { On 2016/05/17 22:14:09, Ryan Hamilton wrote: > nit: no need for else since if() returned. Done. https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:472: } else if (job != main_job_.get() && job != alternative_job_.get()) { On 2016/05/17 22:14:09, Ryan Hamilton wrote: > nit: I might move this to a DCHECK() at the top of the method: > > DCHECK(job == alternative_job_.get() || job == main_job_.get()); Done. https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.h:119: wait_time_ = delay; On 2016/05/17 22:14:09, Ryan Hamilton wrote: > nit: wait_time_for_main_job_ and set_wait_time_for_main_job() Done.
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:312: ResumeAfterDelay(delegate_->wait_time_for_main_job()); Can the delegate (the controller) simply call Resume() on this job when this job should resume? In other words, can the delay time be moved up? https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:121: virtual void OnInitConnectionNotSuccessful( Discussed offline. How about: virtual void OnConnectionInitialized( Job* job, int rv, const base::TimeDelta& delay) = 0; Does this job know what |delay| should be set to, or can the controller figure that out? https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:127: // resume |job| with appropriate wait time. How about something like: // Returns false if |job| can advance to the next state. Otherwise, |job| will wait for <SomeMethod> to be called before advancing. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:147: virtual void set_wait_time_for_main_job(const base::TimeDelta& delay) = 0; It's not obvious to me why these methods are needed on the job, so I'll keep reading. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:200: void ResumeAfterDelay(const base::TimeDelta& delay); What is the difference between these two methods? https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:452: State state_; nit: as discussed, can we remove this? https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:227: MaybeResumeOtherJob(job, base::TimeDelta()); Looks like the other places this is called there is an empty line following. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:331: MaybeResumeOtherJob(job, base::TimeDelta()); ditto. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:449: main_job_->Resume(delay); I'm confused about when wait_time_ is used to resume the job and when it's not. Perhaps this can be simplified? https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:203: void MaybeResumeOtherJob(Job* job, const base::TimeDelta& delay); I think this can only resume the *main* job, right? So perhaps: MaybeResumeMainJob? (And tweak the comment) https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:246: bool blocking_; nit: main_job_is_blocked_; https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:247: base::TimeDelta wait_time_; nit: main_job_wait_time_? (or some such) Can you comment how this relates to the other bool? Can the main job resume if the bool is true? Does it resume when that becomes false after the TimeDelta? https://codereview.chromium.org/1952423002/diff/200001/net/spdy/spdy_test_uti... File net/spdy/spdy_test_util_common.h (right): https://codereview.chromium.org/1952423002/diff/200001/net/spdy/spdy_test_uti... net/spdy/spdy_test_util_common.h:193: std::unique_ptr<QuicRandom> quic_random; It's a bit of a bummer to have net/spdy/ depend on net/quic/. Can we find some way to avoid this?
Patchset #7 (id:220001) has been deleted
Remove the ResumeAfterDelay mehtod, all the logic lives in Resume() now. Also addressing comments. PTAL! https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:312: ResumeAfterDelay(delegate_->wait_time_for_main_job()); On 2016/06/29 23:12:28, Ryan Hamilton wrote: > Can the delegate (the controller) simply call Resume() on this job when this job > should resume? In other words, can the delay time be moved up? Done. Yay!! https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:121: virtual void OnInitConnectionNotSuccessful( On 2016/06/29 23:12:28, Ryan Hamilton wrote: > Discussed offline. How about: > > virtual void OnConnectionInitialized( > Job* job, > int rv, > const base::TimeDelta& delay) = 0; > > Does this job know what |delay| should be set to, or can the controller figure > that out? Discussed offline. The controller will need to know the wait_time_ for the main job if it is a delayed TCP case. The wait time will be set by the alternative job so that when this method is invoked, controller already knows the delay time. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:127: // resume |job| with appropriate wait time. On 2016/06/29 23:12:28, Ryan Hamilton wrote: > How about something like: > > // Returns false if |job| can advance to the next state. Otherwise, |job| will > wait for <SomeMethod> to be called before advancing. Done. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:147: virtual void set_wait_time_for_main_job(const base::TimeDelta& delay) = 0; On 2016/06/29 23:12:28, Ryan Hamilton wrote: > It's not obvious to me why these methods are needed on the job, so I'll keep > reading. The set method is mainly used for delayed TCP case to tell the controller cache the delay for main job. Also used in the test case in request_unittest. The get method is used in Job::Resume, where the job may want to query the controller for waiting time to ResumeAfterDelay. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:200: void ResumeAfterDelay(const base::TimeDelta& delay); On 2016/06/29 23:12:28, Ryan Hamilton wrote: > What is the difference between these two methods? In the old world, Resume calles ResumeAfterDelay if the job's next_state_ is STATE_DO_WAIT_COMPLETE. However, ResumeAfterDelay is also called in DoWait where we want to tell the MAIN job to wait. At that time, the next_state_ is not set to STATE_DO_WAIT_COMPLETE yet. I have changed to the code in DoWait() which will always set the next_state_ to STATE_DO_WAIT_COMPLETE, then query controller whether it should wait, and call *Resume* if needed, then it will set the next_state_ according to the result from ShouldWait(). So no ResumeAfterDealy any more. All in Resume! https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:452: State state_; On 2016/06/29 23:12:28, Ryan Hamilton wrote: > nit: as discussed, can we remove this? Done. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:227: MaybeResumeOtherJob(job, base::TimeDelta()); On 2016/06/29 23:12:28, Ryan Hamilton wrote: > Looks like the other places this is called there is an empty line following. Done. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:331: MaybeResumeOtherJob(job, base::TimeDelta()); On 2016/06/29 23:12:28, Ryan Hamilton wrote: > ditto. Done. https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:203: void MaybeResumeOtherJob(Job* job, const base::TimeDelta& delay); On 2016/06/29 23:12:28, Ryan Hamilton wrote: > I think this can only resume the *main* job, right? So perhaps: > > MaybeResumeMainJob? > > (And tweak the comment) Urgh, right! it should resume the main job only!
Looking GREAT! https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:289: void HttpStreamFactoryImpl::Job::Resume(const base::TimeDelta& delay) { Discussed offline, let's do this: void HttpStreamFactoryImpl::Job::Resume() { DCHECK_EQ(job_type_, MAIN); DCHECK_EQ(STATE_WAIT_COMPLETE, next_state_); OnIOComplete(OK); } Then it's up to the controller to post a task to call Resume() when the Job should actually resume. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:826: delegate_->set_wait_time_for_main_job(base::TimeDelta()); Why does this happen here? I would have thought this method would only be called by the alternative job when the alternative job finally knows how long the main job should wait for. This reminds me that we should make sure we have test coverage for the case where: alt job starts alt job returns ERR_IO_PENDING from DoResolveProxy main job starts main job gets OK from DoResolveProxy main job Waits I suspect we have this coverage, but if not we should make sure we add it. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:833: // Report to delegate that this job has initialized the connection. nit: I'd just drop this since your method is pretty readable. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:921: delegate_->set_wait_time_for_main_job( Yeah, this is the only place I'd expect this method to be called. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:141: virtual bool main_job_is_blocked() = 0; Do we need this method? I would think this should be private to the controller. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:142: virtual const base::TimeDelta& wait_time_for_main_job() const = 0; ditto https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:143: virtual void set_wait_time_for_main_job(const base::TimeDelta& delay) = 0; This needs to be CamelCase, since it's virtual. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:191: // this is not called, then Request is expected to cancel |this| by The request will not actually cancel the job, right? https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:457: if (rv != OK && rv != ERR_SPDY_SESSION_ALREADY_EXISTS) { I'm surprised to see the SPDY_SESSION_ALREADY_EXISTS error here. Can you say more about this?
Test coverage for resume logic added! PTAL :D https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:289: void HttpStreamFactoryImpl::Job::Resume(const base::TimeDelta& delay) { On 2016/07/01 00:13:30, Ryan Hamilton wrote: > Discussed offline, let's do this: > > void HttpStreamFactoryImpl::Job::Resume() { > DCHECK_EQ(job_type_, MAIN); > DCHECK_EQ(STATE_WAIT_COMPLETE, next_state_); > OnIOComplete(OK); > } > > Then it's up to the controller to post a task to call Resume() when the Job > should actually resume. Done. Yay!!! Much cleaner. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:826: delegate_->set_wait_time_for_main_job(base::TimeDelta()); On 2016/07/01 00:13:30, Ryan Hamilton wrote: > Why does this happen here? I would have thought this method would only be called > by the alternative job when the alternative job finally knows how long the main > job should wait for. > > This reminds me that we should make sure we have test coverage for the case > where: > alt job starts > alt job returns ERR_IO_PENDING from DoResolveProxy > main job starts > main job gets OK from DoResolveProxy > main job Waits > > I suspect we have this coverage, but if not we should make sure we add it. This SetWaitTimeForMainJob is to clear the main_job_wait_time_ in controller, but you're right, we are able to move this up to controller to clear the wait time. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:826: delegate_->set_wait_time_for_main_job(base::TimeDelta()); On 2016/07/01 00:13:30, Ryan Hamilton wrote: > Why does this happen here? I would have thought this method would only be called > by the alternative job when the alternative job finally knows how long the main > job should wait for. > > This reminds me that we should make sure we have test coverage for the case > where: > alt job starts > alt job returns ERR_IO_PENDING from DoResolveProxy > main job starts > main job gets OK from DoResolveProxy > main job Waits > > I suspect we have this coverage, but if not we should make sure we add it. Added DoNotResumeMainJobWhenAltJobPending covers this. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:833: // Report to delegate that this job has initialized the connection. On 2016/07/01 00:13:30, Ryan Hamilton wrote: > nit: I'd just drop this since your method is pretty readable. Done. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:141: virtual bool main_job_is_blocked() = 0; On 2016/07/01 00:13:30, Ryan Hamilton wrote: > Do we need this method? I would think this should be private to the controller. Done. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:142: virtual const base::TimeDelta& wait_time_for_main_job() const = 0; On 2016/07/01 00:13:30, Ryan Hamilton wrote: > ditto Removed. Used in the request_unittests. But I have removed the old DelayMainJob thus no usage for this method any more! https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:143: virtual void set_wait_time_for_main_job(const base::TimeDelta& delay) = 0; On 2016/07/01 00:13:30, Ryan Hamilton wrote: > This needs to be CamelCase, since it's virtual. Done. https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:457: if (rv != OK && rv != ERR_SPDY_SESSION_ALREADY_EXISTS) { On 2016/07/01 00:13:30, Ryan Hamilton wrote: > I'm surprised to see the SPDY_SESSION_ALREADY_EXISTS error here. Can you say > more about this? Sure. This is refactored from the old code. In the old code, we will resume the waiting job in DoInitConnectionComplete(). We will resume the main job if we are using_quic AND the DoInitConnection is NOT OK. In addition, we will also resume if we got any error when creating SPDY session. The check for SPDY_SESSION_ALREADY_EXISTS prevents us to incorrectly resume the job. https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...
Looking great! Just a few cosmetic comments on the real code, and a couple of questions about the tests. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:796: next_state_ = STATE_INIT_CONNECTION; nit: It's cleaner to just remove this. Then we'll go to WAIT_COMPLETE which will send us to INIT_CONNECTION. It's a *tiny* bit of extra CPU, but I think it's more consistent with our general purpose state machinery. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:891: // the waiting job if it's paused. nit: Let's tweak this comment slightly since all the job does is set the wait time (not actually resume the main job) // There is no available QUIC session. Inform the delegate how long to delay the main job. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:123: // will wait for Job::OnIOComplete() to be called before advancing. Resume() not OnIOComplete(), right? https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:469: if (job == alternative_job_.get() && main_job_) { nit: I recommend reversing the polarity of this check (and possibly adding it to the previous if: if (!main_job_is_blocked_ || job != alternative_job_.get() || !main_job_) return; main_job_is_blocked = false; if (!main_job_->is_waiting()) return; ...PostTask....() https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:482: int rv) { can we do DCHECK(main_job_is_blocked_)? https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:483: if (rv != OK && rv != ERR_SPDY_SESSION_ALREADY_EXISTS) { I think perhaps we should move this ERR_SPDY_SESSION_ALREADY_EXISTS check down into the Job because it's really the job which knows about this error code, right? https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:506: } Should this be: if (main_job_is_blocked_) return true; if (main_job_wait_time_.is_zero()) return false; PostTask(); return true; With the code as written, If the main job is blocked, and the wait time is non-zero, we'll post a task. But I don't think we should do that while the main job is blocked. (Admittedly, I don't think we can get into this state, but I think it's cleaner to simply return true and do nothing else if the main job is blocked.) https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:561: main_job_wait_time_ = delay; DCHECK(main_job_is_blocked); https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:141: // i.e., |job| won't call Job::ResumeAfterDelay(). Else true and resume ResumeAfterDelay is now dead, right? Perhaps this should be reworked? https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:246: bool main_job_is_blocked_; Let's comment this a bit more. When main_job_is_blocked_ is true, then the main job must not create a connection. If it is ready to create a connection while it is blocked, then it must wait until is it resumed. Or some such... https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:121: ; nit: extra ; https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:466: EXPECT_CALL(*job_factory_.main_job(), MarkOtherJobComplete(_)).Times(1); Is MarkOtherJobComplete going away in a future CL? https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:517: HttpStreamRequest::HTTP_STREAM); Why do we need to use a Peer here? https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:713: HttpStreamRequest::HTTP_STREAM); Can we avoid the peer by making the PAC resolution asyn (or perhaps host resolution async?)
Addressing nits. Feel free to chat over on JobPeer. Thanks! https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:796: next_state_ = STATE_INIT_CONNECTION; On 2016/07/12 00:21:45, Ryan Hamilton wrote: > nit: It's cleaner to just remove this. Then we'll go to WAIT_COMPLETE which will > send us to INIT_CONNECTION. It's a *tiny* bit of extra CPU, but I think it's > more consistent with our general purpose state machinery. Done. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:891: // the waiting job if it's paused. On 2016/07/12 00:21:44, Ryan Hamilton wrote: > nit: Let's tweak this comment slightly since all the job does is set the wait > time (not actually resume the main job) > > // There is no available QUIC session. Inform the delegate how long to delay the > main job. Done. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:123: // will wait for Job::OnIOComplete() to be called before advancing. On 2016/07/12 00:21:45, Ryan Hamilton wrote: > Resume() not OnIOComplete(), right? Done. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:469: if (job == alternative_job_.get() && main_job_) { On 2016/07/12 00:21:45, Ryan Hamilton wrote: > nit: I recommend reversing the polarity of this check (and possibly adding it to > the previous if: > > if (!main_job_is_blocked_ || job != alternative_job_.get() || !main_job_) > return; > > main_job_is_blocked = false; > if (!main_job_->is_waiting()) > return; > > ...PostTask....() Done. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:482: int rv) { On 2016/07/12 00:21:45, Ryan Hamilton wrote: > can we do DCHECK(main_job_is_blocked_)? No, we couldn't. OnConnectionInitialized is called in Job::DoConnection(). If it's called by the main job, then main_job_is_blocked_ has to be false as main job must not proceed to create connection if blocked. There're also other cases where main_job_is_blocked_ is false but this OnConnectionInitialized is called. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:483: if (rv != OK && rv != ERR_SPDY_SESSION_ALREADY_EXISTS) { On 2016/07/12 00:21:45, Ryan Hamilton wrote: > I think perhaps we should move this ERR_SPDY_SESSION_ALREADY_EXISTS check down > into the Job because it's really the job which knows about this error code, > right? Done. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:506: } On 2016/07/12 00:21:45, Ryan Hamilton wrote: > Should this be: > > if (main_job_is_blocked_) > return true; > > if (main_job_wait_time_.is_zero()) > return false; > > PostTask(); > return true; > > With the code as written, If the main job is blocked, and the wait time is > non-zero, we'll post a task. But I don't think we should do that while the main > job is blocked. (Admittedly, I don't think we can get into this state, but I > think it's cleaner to simply return true and do nothing else if the main job is > blocked.) Done! Hmmm, you are right. We should never run into the case where the wait_time is nonzero but main_job_is_still_blocked. As we only set the nonzero wait time for delayed TCP and returns ERR_IO_PENDING to call OnConnectionInitialzed which then will ResumeMainJob to set the main_job_is_blocked to false. But that does reminds me to only update wait time when main job is blocked in SetWaitTimeForMainJob. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:561: main_job_wait_time_ = delay; On 2016/07/12 00:21:45, Ryan Hamilton wrote: > DCHECK(main_job_is_blocked); Nooo. This method is called by the Job without knowing whether there's main job blocking.There's no guarantee that main job is blocked. In fact, there is a case that we force quic with only one job, then the main job is not blocked, but it will try call SetWaitTimeForMainJob, which if correctly written, should do nothing. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:141: // i.e., |job| won't call Job::ResumeAfterDelay(). Else true and resume On 2016/07/12 00:21:45, Ryan Hamilton wrote: > ResumeAfterDelay is now dead, right? Perhaps this should be reworked? Done. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:246: bool main_job_is_blocked_; On 2016/07/12 00:21:45, Ryan Hamilton wrote: > Let's comment this a bit more. > > When main_job_is_blocked_ is true, then the main job must not create a > connection. If it is ready to create a connection while it is blocked, then it > must wait until is it resumed. > > Or some such... Done. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:121: ; On 2016/07/12 00:21:45, Ryan Hamilton wrote: > nit: extra ; whoops, sorry :( https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:466: EXPECT_CALL(*job_factory_.main_job(), MarkOtherJobComplete(_)).Times(1); On 2016/07/12 00:21:45, Ryan Hamilton wrote: > Is MarkOtherJobComplete going away in a future CL? Yeah, this one is more for resume logic. It should be a simple change if job_status_ is well taken care of. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:517: HttpStreamRequest::HTTP_STREAM); On 2016/07/12 00:21:45, Ryan Hamilton wrote: > Why do we need to use a Peer here? Job::Start() is called by JobController::CreateJobs(). We have to mock it for testing purpose so that we can test step by step. Then here we want to call the real Start() to test more of the resume logic. https://codereview.chromium.org/1952423002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:713: HttpStreamRequest::HTTP_STREAM); On 2016/07/12 00:21:45, Ryan Hamilton wrote: > Can we avoid the peer by making the PAC resolution asyn (or perhaps host > resolution async?) I thought you were saying use the asyn PAC resolver to prevent calling the alternative_job.Start() before main_job.Start(). But this won't help us avoid the peer.
zhongyi@chromium.org changed reviewers: + eroman@chromium.org
Hi Eric, I have left a comment in the job_controller_unittest where I override the url for proxy resolution. PTAL, thanks a lot! Cherie https://codereview.chromium.org/1952423002/diff/320001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1952423002/diff/320001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:587: The original url has to use HTTPS so as to racing the alternative job (QUIC) with the main job. => The two jobs will then use the same URL for proxy resolution. And the ordering is not enforced. => Thus hack to use a different URL (downgrade to HTTP) in the test. Changing the std::set to std::vector for RequrestList in ProxyService sounds fine, but it hurts the efficiency in the production. Thus a little bit hesitated. @eroman: any thought here?
https://codereview.chromium.org/1952423002/diff/320001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1952423002/diff/320001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:587: On 2016/07/15 18:42:49, Zhongyi Shi wrote: > The original url has to use HTTPS so as to racing the alternative job (QUIC) > with the main job. => The two jobs will then use the same URL for proxy > resolution. And the ordering is not enforced. => Thus hack to use a different > URL (downgrade to HTTP) in the test. > > Changing the std::set to std::vector for RequrestList in ProxyService sounds > fine, but it hurts the efficiency in the production. Thus a little bit > hesitated. > > @eroman: any thought here? Does this test specifically need to test using PAC proxy resolution (via auto-detect) ? If that isn't a requirement, you can probably have a better time using a synchronous ProxyService such as: ProxyService::CreateDirect() -- if you don't care to use a proxy ProxyService::CreateFixedFromPacResult() -- if you want to use a single proxy server for all requests Or if you want it to use different proxies for different hosts or schemes, you can create a config for that too. Or lastly if you need finer grained control you can create your own (synchronous) ProxyResolver implementation for this test that returns a proxy of your choosing.
Almost done! https://codereview.chromium.org/1952423002/diff/320001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1952423002/diff/320001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:522: job_factory_.DisableMockStartForJobs(); Discussed offline, let's move to using the 'normal' Start in all MockJobs. This will require the "old" test to use the Async proxy resolver instead of Mocking the Start() method.
Removed the mock method for Start(). Please use this patch for review. I will upload a subsequent CL to rebase to master. Thanks!
Using mock async proxy resolver in existing unittests.
lgtm woo hoo!
Patchset #14 (id:380001) has been deleted
Patchset #15 (id:420001) has been deleted
The CQ bit was checked by zhongyi@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...) win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
The CQ bit was checked by zhongyi@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 2016/07/21 22:34:16, Ryan Hamilton wrote: > lgtm > > woo hoo! Rebase to origin/master. Fixed controller unittests as bnc@ changed the AlternativeService to support HTTPS only. Ryan, could you re-l-g-t-m to this CL? Thanks! The previous path you reviewed is PS 13.
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 zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/1952423002/#ps460001 (title: "fix a include typo")
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 ========== JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. HttpStreamFactoryImpl::JobController will take over scheduling between main job and alternative job. BUG=605398 ========== to ========== JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. HttpStreamFactoryImpl::JobController will take over scheduling between main job and alternative job. BUG=605398 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. HttpStreamFactoryImpl::JobController will take over scheduling between main job and alternative job. BUG=605398 ========== to ========== JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. HttpStreamFactoryImpl::JobController will take over scheduling between main job and alternative job. BUG=605398 Committed: https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5 Cr-Commit-Position: refs/heads/master@{#407846} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5 Cr-Commit-Position: refs/heads/master@{#407846}
Message was sent while issue was closed.
On 2016/07/26 17:50:39, commit-bot: I haz the power wrote: > Patchset 16 (id:??) landed as > https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5 > Cr-Commit-Position: refs/heads/master@{#407846} yay! Now, I am going to work on the Flywheel CL to use QUIC. Stay tuned!
Message was sent while issue was closed.
On 2016/07/26 17:52:54, tbansal1 wrote: > On 2016/07/26 17:50:39, commit-bot: I haz the power wrote: > > Patchset 16 (id:??) landed as > > https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5 > > Cr-Commit-Position: refs/heads/master@{#407846} > > yay! Now, I am going to work on the Flywheel CL to use QUIC. Stay tuned! Sweet! Sorry this CL takes long time to be checked in and might affect your work a little bit. |
