Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1941083002:
JobController 1: Adding a new class HttpStreamFactoryImpl::JobController (Closed)
|
Created:
4 years, 7 months ago by Zhongyi Shi Modified:
4 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionJobController 1: Adding a new class HttpStreamFactoryImpl::JobController and remove cross reference between HttpStreamFactoryImpl::Request, HttpStreamFactoryImpl::Job, and HttpStreamFactoryImpl::Impl.
BUG=605398
Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69
Committed: https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6
Cr-Original-Commit-Position: refs/heads/master@{#400086}
Cr-Commit-Position: refs/heads/master@{#400552}
Patch Set 1 #Patch Set 2 : OrphanJobs #Patch Set 3 : remove ref to factory_impl in Job #Patch Set 4 : remove cross reference #Patch Set 5 : #
Total comments: 64
Patch Set 6 : remove multiple refs to Job in JobController #
Total comments: 1
Patch Set 7 : reorder methods declaration in JobController by consumer type #Patch Set 8 : comment fix #
Total comments: 14
Patch Set 9 : address rch's comments #Patch Set 10 : comment/naming fix #
Total comments: 28
Patch Set 11 : address rdsmith's comments #
Total comments: 4
Patch Set 12 : refine comments #
Total comments: 6
Patch Set 13 : add RequestVisitorInterface and JobVisitorInterface #Patch Set 14 : rename interfaces #Patch Set 15 : merge unittests to this CL #
Total comments: 10
Patch Set 16 : add JobFactory and rewrite test cases #
Total comments: 6
Patch Set 17 : add JobFactory interface in JobController, remove JobControllerPeer #
Total comments: 43
Patch Set 18 : address comments #
Total comments: 1
Patch Set 19 : sync and fix compile only, Plz use PS18 to review new changes. #
Total comments: 26
Patch Set 20 : rename JobFactoryImpl to DefaultJobFactory #Patch Set 21 : Hide DefaultJobFactory in impl.cc, Move JobFactory to Impl, move ImplPeer to test_util #Patch Set 22 : rebase #
Total comments: 12
Patch Set 23 : address rch's nits #Patch Set 24 : Regrassion test and fix for crbug/621069 #
Total comments: 2
Patch Set 25 : address nits #Dependent Patchsets: Messages
Total messages: 80 (25 generated)
zhongyi@chromium.org changed reviewers: + rch@chromium.org, rdsmith@chromium.org
This CL is the step 1 of introducing the JobController. Removing cross reference between Request and Job. Subsequent CLs will remove the reference from Request to factory, remove cross ref between Jobs so as to do scheduling in JobController. Not intended to landed, aimed for early feedback. Thanks!
Patchset #3 (id:40001) has been deleted
Remove reference to HttpStreamFactoryImpl in Job. *Do Not Review*
Cherie: I'm not sure how to understand your "looking for early feedback" and "do not review", but I'm going with the latter since I'm feeling a bit swamped by code reviews at the moment :-}. Let me know if that's the wrong choice.
On 2016/05/04 15:46:27, Randy Smith - Not in Fridays wrote: > Cherie: I'm not sure how to understand your "looking for early feedback" and "do > not review", but I'm going with the latter since I'm feeling a bit swamped by > code reviews at the moment :-}. Let me know if that's the wrong choice. Sorry for the confusion here. This CL now removes all the cross reference between Request <-> Job, and Job <-> FactoryImpl as well as Request <-> FactoryImpl. Thanks!
Description was changed from ========== JobController 1: Remove cross reference between Request and Job. BUG=605398 ========== to ========== JobController 1: Remove cross reference between Request and Job. BUG=605398 ==========
Description was changed from ========== JobController 1: Remove cross reference between Request and Job. BUG=605398 ========== to ========== JobController 1: Remove cross reference between Request, Job, and Impl. BUG=605398 ==========
Patchset #5 (id:100001) has been deleted
This is glorious! I think there's still room to move things around a bit more, but this is a really nice step in the right direction. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:98: proxy_ssl_config, delegate, stream_type, net_log); If this is the only place we call Start and CreatRequest (should that be Creat*e*Request?) we could consider making just a single method. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:75: typedef std::set<JobController*> JobControllerSet; Since the controllers are owned by the factory, this can probably contain unique_ptrs https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:248: stream_type_ = request->stream_type(); nit: In that case, I'd just pass in the stream_type instead of the full request. You might be able to do this in constructor, though I'm not sure about that. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:329: if (!IsPreconnecting()) Why not set this true for the preconnect case? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:427: if (IsOrphaned()) { Ooo! You're going to love (or probably) hate this :> The controller knows (or could easily know) if a job is the bound or orphaned job. As a result, I think the job can just do: job_controller_->MarkRequestComplete(...) job_controller_->OnStreamReady(...) and the controller can do the check to see if the job has been orphaned. This will make the job simpler. (Here and elsewhere). In fact, it wouldn't surprise me if this meant that the job no longer needed to know about being orphaned, which would be nice! https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:349: JobController* job_controller_; nit: // Unowned. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:26: STLDeleteContainerPointers(tmp_job_set.begin(), tmp_job_set.end()); Would this work: STLDeleteContainerPointers(orphaned_job_set_); https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:80: ignore_result(factory_->ApplyHostMappingRules(request_info.url, I wonder if GetAlternativeServiceFor and ApplyHostMappingRules can move to the controller? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:156: it != jobs_.end(); ++it) { nit: I think you can do a C++ loop here: for (auto& job : jobs_) { job->SetPriority(priority) } https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:172: factory_->request_map_[job] = request_; Is request_map_ still used in your new world? If so, what's it for? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:162: std::set<Job*> jobs_; These jobs are owned by this, right? If so, perhaps set<std::unique_ptr<>> ? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:178: std::set<const Job*> orphaned_job_set_; Just an observation, there are quite a few different members which are pointers to the same two jobs: jobs_ main_job_ alternative_job_ bound_job_ orphaned_job_set It might be nice to find a way to clean this up. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (left): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:84: DCHECK(stream); Did you mean to remove these changes? I assume so, but it's not obvious why. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:153: DCHECK_NE(OK, status); Did you mean to remove this? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:46: job_controller_->OnRequestFinish(); nit: I would unify these into a single method on the controller. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:56: request_set.insert(this); Can you add a method on the controller which is something like: AddSpdySessionRequest(const SpdySessionKey& spdy_session_key, HttpStreamFactoryImpl::Request* request); and just call that here: AddSpdySessionRequest(spdy_session_key, this); Then move the logic of manipulating the set into the controller. GAH! Scratch that. It looks like this method (SetSpdySessionKey) is called by the controller, so the controller can do this logic when it calls this method. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:144: } THIS IS SO MUCH CLEANER! https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:181: } I think this is perhaps another case where the controller is calling this method and can just do this work locally? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:130: JobController* const job_controller_; nit: comment on ownership. // Unowned.
Cherie: Some intermediate nits (I'll keep reviewing) but I wanted to summarize a conversation between Ryan and I offline because it may be relevant for planning. In doing this review, I ended up focussing on ownership and lifetime issues (probably obviously from the nits I include below :-}) and found myself wondering specifically about the relationship between the JobController and Request lifetimes. Ryan enlightened me as to the use case for JobController outliving request (currently orphaned jobs are not cancelled, but instead let to run to completion), but I did my usual thing, which is to then ask "Why do they run to completion?" That conversation went in several different places, but I think we eventually decided that it was reasonable to kill all jobs associated with a request (including orphaned ones) if the request is cancelled. If that was done, it would mean that in non-preconnect cases, the lifetimes of the JobController and the Request are linked, in which case I want to consider binding them explicit, i.e. making the Request own the JobController. (In the case where a preconnect is done, the HttpStreamFactoryImpl would continue to own the JobController, but it doesn't do anything with it other than respond to completion by destroying it, so I don't think that's a link that's worth a lot of conceptual weight.) That (the killing of orphaned jobs on request cancellation) is a behavior change, though, which I don't think is ideal in the context of a refactoring CL, and Ryan convinced me that it's a much easier change to make after this CL goes in. So I wanted to give you a heads up that I'm interested in exploring if that change makes sense in a follow-on CL, and see what you think of that idea. I'd be happy to have that discussion on the design document, if you'd prefer; it's only tangentially related to this CL. nits follow, and I'll go back to more generally reviewing now. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:329: if (!IsPreconnecting()) On 2016/05/06 20:49:01, Ryan Hamilton wrote: > Why not set this true for the preconnect case? I think to keep the old behavior--IsOrphaned() used to return false if preconnect. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:338: if (job_controller_->for_websockets() && connection_ && What aspects of your change changed the lifecycle such that connection_ could be null at this point now but couldn't before? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:44: Job(JobController* job_controller, nit, suggestion: I have a bit of a reflex for asking for a comment explaining why storing an unowned pointer is ok. Here it would be something like "Job is owned by JobController, hence this pointer is valid for the lifetime of the Job." https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:18: Request* CreatRequest(const HttpRequestInfo& request_info, Would you be willing to add some documenting comments to this interface? What is the expected use cases? Is CreateRequest ever called more than once? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:160: Request* request_; This naively strikes me as fairly dangerous because it might lead to a use-after-free error. I'd like there to be some documentation about why that doesn't happen (e.g. safe because the request notifies the JobController when it's cancelled/destroyed through X, which nulls this variable.) https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:30: JobController* job_controller, Same reflex for documentation: Why can this class assume the JobController will outlive it?
End of first round review comments. I'm postponing looking at the tests until after we've got the main code in basically the final state. This does look like a major improvement. Thank you! https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:248: stream_type_ = request->stream_type(); On 2016/05/06 20:49:01, Ryan Hamilton wrote: > nit: In that case, I'd just pass in the stream_type instead of the full request. > You might be able to do this in constructor, though I'm not sure about that. Agreed. Specifically, the design flow sorta promises a lack of interaction between Request and Job; all such interactions now go through JobController. What's left is minimal, but leaving any such connections behind invites future modifications to increase those connections. So I'd be a bit ruthless in removing such references. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:327: void HttpStreamFactoryImpl::Job::Orphan(const Request* request) { This routine no longer uses the request argument; remove it? (See above for context.) https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:338: if (job_controller_->for_websockets() && connection_ && On 2016/05/09 20:42:18, Randy Smith - Not in Fridays wrote: > What aspects of your change changed the lifecycle such that connection_ could be > null at this point now but couldn't before? Whoops, sorry, missed that this is identical to previous code. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:427: if (IsOrphaned()) { On 2016/05/06 20:49:01, Ryan Hamilton wrote: > Ooo! You're going to love (or probably) hate this :> > > The controller knows (or could easily know) if a job is the bound or orphaned > job. As a result, I think the job can just do: > > job_controller_->MarkRequestComplete(...) > job_controller_->OnStreamReady(...) > > and the controller can do the check to see if the job has been orphaned. This > will make the job simpler. (Here and elsewhere). In fact, it wouldn't surprise > me if this meant that the job no longer needed to know about being orphaned, > which would be nice! +1, if it can be done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:807: : NetLog::Source(), Hmmm. Parallel to Ryan's suggestions above, I'd be tempted to push the source choice up into the JobController, and have it figure out whether it wants this logged or not. It's a behavior change, but I could imagine at some point we'd want to log these even if they're preconnecting or orphaned. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:49: // Used to orphan all jobs in |jobs_|. nit, suggestion: When you talk about "orphaning" jobs in these comments, could you define that? Given that it used to mean "disassociate the job from the request so that it will outlive the request" and we don't associate jobs with requests anymore, I'm curious what orphaning means now. Is it as simple as "protect the job from being killed if the request is cancelled, and don't tell the request about job completion"? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:52: // Called when a Job succeeds. I think this is an inaccurate comment; at least, it looks to me like this is called on Request destruction, which can happen for reasons other than a Job succeeding. I'd also like to advocate for documenting routines by what they do, from the perspective of the consumer (i.e. after the implementation has been abstracted away). So I think this is "Cancel all non-orphaned jobs."? (This point isn't as important as it usually is because these classes are pretty tightly integrated, but I think it's still goodness.) https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:56: void OnJobSucceeded(Job* job); Why is this public? I think it's only called from the controller. (In general, I think it's worth trying for the minimal interface we need, which means anything not called from outside from the controller should be private, unless there's some strong API reason for exposing it.) https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:60: // Marks completion of the |request_|. Must be called before OnStreamReady(). Suggestion: I wonder if it's worthwhile separating out the different pieces of the JobController interface by what consumer they're targeted at (Request, Job, HttpStreamFactoryImpl). The a-r way to do that would be to have defined "Delegate" interfaces for the Request and Job that JobController inherited from, which I'm not sure is worth it given the tight coupling between the classes. But maybe just group them in this file that way, with comments? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:106: // Notify the |request_| and |factoy_| of the readiness of new SPDY session. nit: factory_ https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:133: void MaybeNotifyFactoryOfCompletion(); As above, why not private? (You should do your own pass; I just happened to notice this one.) https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:208: if (job_controller_->for_websockets()) { Can we hoist this conditional into JobController and just use Request::On{Bidirectional,}Stream{Impl,}Ready? https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:130: JobController* const job_controller_; On 2016/05/06 20:49:02, Ryan Hamilton wrote: > nit: comment on ownership. > > // Unowned. And why it's safe; i.e. that the Request doesn't outlive the JobController (which I don't think is true) or that it's nulled when the JobController is destroyed.
Patchset #6 (id:140001) has been deleted
Addressing comments. Move IsOrphaned to JobController. Remove multiple refs to Jobs in JobController. PTAL. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:98: proxy_ssl_config, delegate, stream_type, net_log); On 2016/05/06 20:49:01, Ryan Hamilton wrote: > If this is the only place we call Start and CreatRequest (should that be > Creat*e*Request?) we could consider making just a single method. I have renamed the old public method Start() to private method CreateJobs(), renamed CreatRequest() to Start(). https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:75: typedef std::set<JobController*> JobControllerSet; On 2016/05/06 20:49:01, Ryan Hamilton wrote: > Since the controllers are owned by the factory, this can probably contain > unique_ptrs Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:248: stream_type_ = request->stream_type(); On 2016/05/09 21:42:09, Randy Smith - Not in Fridays wrote: > On 2016/05/06 20:49:01, Ryan Hamilton wrote: > > nit: In that case, I'd just pass in the stream_type instead of the full > request. > > You might be able to do this in constructor, though I'm not sure about that. > > Agreed. Specifically, the design flow sorta promises a lack of interaction > between Request and Job; all such interactions now go through JobController. > What's left is minimal, but leaving any such connections behind invites future > modifications to increase those connections. So I'd be a bit ruthless in > removing such references. Done. It's actually done in a subsequent CL, but I will be happy to bring it ahead. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:327: void HttpStreamFactoryImpl::Job::Orphan(const Request* request) { On 2016/05/09 21:42:09, Randy Smith - Not in Fridays wrote: > This routine no longer uses the request argument; remove it? (See above for > context.) Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:329: if (!IsPreconnecting()) On 2016/05/06 20:49:01, Ryan Hamilton wrote: > Why not set this true for the preconnect case? orphaned_ indicates whether |this| job has ever been associated with a Request but then Orphaned. Preconnect job is never orphaned as it's never been associated with a Request. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:427: if (IsOrphaned()) { On 2016/05/09 21:42:09, Randy Smith - Not in Fridays wrote: > On 2016/05/06 20:49:01, Ryan Hamilton wrote: > > Ooo! You're going to love (or probably) hate this :> > > > > The controller knows (or could easily know) if a job is the bound or orphaned > > job. As a result, I think the job can just do: > > > > job_controller_->MarkRequestComplete(...) > > job_controller_->OnStreamReady(...) > > > > and the controller can do the check to see if the job has been orphaned. This > > will make the job simpler. (Here and elsewhere). In fact, it wouldn't surprise > > me if this meant that the job no longer needed to know about being orphaned, > > which would be nice! > > +1, if it can be done. Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:807: : NetLog::Source(), On 2016/05/09 21:42:09, Randy Smith - Not in Fridays wrote: > Hmmm. Parallel to Ryan's suggestions above, I'd be tempted to push the source > choice up into the JobController, and have it figure out whether it wants this > logged or not. > > It's a behavior change, but I could imagine at some point we'd want to log these > even if they're preconnecting or orphaned. Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:44: Job(JobController* job_controller, On 2016/05/09 20:42:18, Randy Smith - Not in Fridays wrote: > nit, suggestion: I have a bit of a reflex for asking for a comment explaining > why storing an unowned pointer is ok. Here it would be something like "Job is > owned by JobController, hence this pointer is valid for the lifetime of the > Job." Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:349: JobController* job_controller_; On 2016/05/06 20:49:01, Ryan Hamilton wrote: > nit: // Unowned. Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:26: STLDeleteContainerPointers(tmp_job_set.begin(), tmp_job_set.end()); On 2016/05/06 20:49:01, Ryan Hamilton wrote: > Would this work: > > STLDeleteContainerPointers(orphaned_job_set_); Acknowledged. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:80: ignore_result(factory_->ApplyHostMappingRules(request_info.url, On 2016/05/06 20:49:02, Ryan Hamilton wrote: > I wonder if GetAlternativeServiceFor and ApplyHostMappingRules can move to the > controller? Done! They are all moved in the new patch. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:156: it != jobs_.end(); ++it) { On 2016/05/06 20:49:02, Ryan Hamilton wrote: > nit: I think you can do a C++ loop here: > > for (auto& job : jobs_) { > job->SetPriority(priority) > } Acknowledged. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:172: factory_->request_map_[job] = request_; On 2016/05/06 20:49:02, Ryan Hamilton wrote: > Is request_map_ still used in your new world? If so, what's it for? This is still used as it is in the old code to make sure all the jobs are handed out to requests. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:18: Request* CreatRequest(const HttpRequestInfo& request_info, On 2016/05/09 20:42:18, Randy Smith - Not in Fridays wrote: > Would you be willing to add some documenting comments to this interface? What > is the expected use cases? Is CreateRequest ever called more than once? Comments added. CreateRequest(renamed as Start()) should only be called when we RequestStream(). https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:106: // Notify the |request_| and |factoy_| of the readiness of new SPDY session. On 2016/05/09 21:42:09, Randy Smith - Not in Fridays wrote: > nit: factory_ Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:160: Request* request_; On 2016/05/09 20:42:18, Randy Smith - Not in Fridays wrote: > This naively strikes me as fairly dangerous because it might lead to a > use-after-free error. I'd like there to be some documentation about why that > doesn't happen (e.g. safe because the request notifies the JobController when > it's cancelled/destroyed through X, which nulls this variable.) Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:162: std::set<Job*> jobs_; On 2016/05/06 20:49:02, Ryan Hamilton wrote: > These jobs are owned by this, right? If so, perhaps set<std::unique_ptr<>> ? Acknowledged. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:178: std::set<const Job*> orphaned_job_set_; On 2016/05/06 20:49:02, Ryan Hamilton wrote: > Just an observation, there are quite a few different members which are pointers > to the same two jobs: > > jobs_ > main_job_ > alternative_job_ > bound_job_ > orphaned_job_set > > It might be nice to find a way to clean this up. Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (left): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:84: DCHECK(stream); On 2016/05/06 20:49:02, Ryan Hamilton wrote: > Did you mean to remove these changes? I assume so, but it's not obvious why. This is intentional. Request::OnStreamReady is now called by JobController::OnStreamReady(), in which those checks will be performed. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:153: DCHECK_NE(OK, status); On 2016/05/06 20:49:02, Ryan Hamilton wrote: > Did you mean to remove this? Yup. The Job binding work is done in Controller. Thus in JobController::OnCertificateError, we will check to see whether BindJob should be called. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:46: job_controller_->OnRequestFinish(); On 2016/05/06 20:49:02, Ryan Hamilton wrote: > nit: I would unify these into a single method on the controller. Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:56: request_set.insert(this); On 2016/05/06 20:49:02, Ryan Hamilton wrote: > Can you add a method on the controller which is something like: > > AddSpdySessionRequest(const SpdySessionKey& spdy_session_key, > HttpStreamFactoryImpl::Request* request); > > and just call that here: > > AddSpdySessionRequest(spdy_session_key, this); > > Then move the logic of manipulating the set into the controller. > > GAH! Scratch that. It looks like this method (SetSpdySessionKey) is called by > the controller, so the controller can do this logic when it calls this method. Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:181: } On 2016/05/06 20:49:02, Ryan Hamilton wrote: > I think this is perhaps another case where the controller is calling this method > and can just do this work locally? Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:208: if (job_controller_->for_websockets()) { On 2016/05/09 21:42:10, Randy Smith - Not in Fridays wrote: > Can we hoist this conditional into JobController and just use > Request::On{Bidirectional,}Stream{Impl,}Ready? This method is now removed!! https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:30: JobController* job_controller, On 2016/05/09 20:42:18, Randy Smith - Not in Fridays wrote: > Same reflex for documentation: Why can this class assume the JobController will > outlive it? Done. https://codereview.chromium.org/1941083002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:130: JobController* const job_controller_; On 2016/05/09 21:42:10, Randy Smith - Not in Fridays wrote: > On 2016/05/06 20:49:02, Ryan Hamilton wrote: > > nit: comment on ownership. > > > > // Unowned. > > And why it's safe; i.e. that the Request doesn't outlive the JobController > (which I don't think is true) or that it's nulled when the JobController is > destroyed. Done. https://codereview.chromium.org/1941083002/diff/160001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/160001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:666: // TODO(crbug.com/610546): Fix and re-enable this test. This is already checked in origin/master. Manually added here as I haven't sync the code up to date.
One more note: Please review the patch set 6, then 7. Patch 7 is reordering the declarations of the methods. Thanks!
On 2016/05/12 18:01:00, Zhongyi Shi wrote: > One more note: > > Please review the patch set 6, then 7. Patch 7 is reordering the declarations of > the methods. Thanks! There are a few more steps to do to follow up this CL: Job Controller 2: half done which moves scheduling from Job to JobController. And wait for this CL to be finalized. Job Controller 1 TEST: will add some unittests to this CL. Possible: Job Controller 3: cancel all the Jobs when request is cancelled rather than leaving orphaned Jobs outlive the request. In that case, Request will own JobController, and JobController will own the Job(s). This is a behavior change. We can consider do this once all the other 3 is landed.
I think this is looking pretty good! Just a few small comments. https://codereview.chromium.org/1941083002/diff/200001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:667: TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) { Is this part of this CL? https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:170: }); holy cow, that is hideous :( That's almost enough to consider using raw pointers instead. Or, I guess a map<ptr, unique_ptr>? That being said, it might be simpler to just iterate over the list manually: for (auto it = set_.begin(); it != set_.end(); ++it) { if (it->get() == controller) { set_.erase(it); } } *Shrug* https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:112: request_ = NULL; nit: s/NULL/nullptr/ throughout this CL. Ah, C++ https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:22: /* Methods below are called by HttpStreamFactoryImpl only */ Let's only use // to be consistent. https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:195: bool job_bounded_; Could you get rid of this by checking bound_job_ == nullptr? https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:44: job_controller_->OnRequestFinish(); nit: s/Finish/Complete/ I think would be a better name. (Or Finished, to get the right part of speech).
Thanks Ryan for reviewing. Comments addressed. PTAL! https://codereview.chromium.org/1941083002/diff/200001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:667: TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) { On 2016/05/13 20:50:40, Ryan Hamilton wrote: > Is this part of this CL? Nope. It's just a manually temp fix. It's in origin/master already. I just didn't sync the code up to date as there's another CL depending on this one. https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:170: }); On 2016/05/13 20:50:40, Ryan Hamilton wrote: > holy cow, that is hideous :( > > That's almost enough to consider using raw pointers instead. > > Or, I guess a map<ptr, unique_ptr>? > > That being said, it might be simpler to just iterate over the list manually: > > for (auto it = set_.begin(); it != set_.end(); ++it) { > if (it->get() == controller) { > set_.erase(it); > } > } > > *Shrug* Done. https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:112: request_ = NULL; On 2016/05/13 20:50:40, Ryan Hamilton wrote: > nit: s/NULL/nullptr/ throughout this CL. Ah, C++ Done. https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:22: /* Methods below are called by HttpStreamFactoryImpl only */ On 2016/05/13 20:50:40, Ryan Hamilton wrote: > Let's only use // to be consistent. Done. https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:195: bool job_bounded_; On 2016/05/13 20:50:40, Ryan Hamilton wrote: > Could you get rid of this by checking bound_job_ == nullptr? Unfortunately not. We no longer keep a pointer to orphaned job as we're trying to minimize keeping refs. The new logic for JobController to recognize whether a job is orphaned or not now becomes: 1. if we ever bound a Job, and the bound_job_ is != job, then job is orphaned. 2. if we ever bound a Job, but the bound_job_ is then nulled, then job is orphaned. 3. if we never bound a Job, then the job is definitely not orphaned. Thus job_bounded != (bound_job != nullptr). https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:44: job_controller_->OnRequestFinish(); On 2016/05/13 20:50:40, Ryan Hamilton wrote: > nit: s/Finish/Complete/ I think would be a better name. (Or Finished, to get the > right part of speech). Done. Renamed to OnRequestComplete
https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:194: // True if ever a job is explicitly bounded to the |request_|. s/bounded/bound/ (bound is the past tense of bind) Perhaps // True if a job has been bound to |request_|. or // True if a job has ever been bound to the request. https://codereview.chromium.org/1941083002/diff/200001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:195: bool job_bounded_; On 2016/05/13 22:22:21, Zhongyi Shi wrote: > On 2016/05/13 20:50:40, Ryan Hamilton wrote: > > Could you get rid of this by checking bound_job_ == nullptr? > > Unfortunately not. We no longer keep a pointer to orphaned job as we're trying > to minimize keeping refs. The new logic for JobController to recognize whether a > job is orphaned or not now becomes: > 1. if we ever bound a Job, and the bound_job_ is != job, then job is orphaned. > 2. if we ever bound a Job, but the bound_job_ is then nulled, then job is > orphaned. > 3. if we never bound a Job, then the job is definitely not orphaned. > > Thus job_bounded != (bound_job != nullptr). Ah, I see. That makes sense. Thanks. Might be good to mentioned this in the comment for this member, and perhaps down in bound_job_ mention that it will be set to nullptr after the request is finished?
Fix some comments and naming issues. PTAL :)
Non-test comments; I'm continuing to review the tests, but I figured I'd reduce latency for the others. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:238: connection_.reset(); Why is this dependent on state? And why is it needed at all? connection_ is owned by this class, and hence will be destroyed on destruction of Job. (The reason I ask this question is that I want to make sure that the pathway of JobController outlives request, and then system shutdown results in HttpStreamFactoryImpl destruction, which destroys the JobController works. But what is or is not destroyed by hand on the different destruction paths seems inconsistent; specifically, I'm confused about Orphan() explicitly calling connection_->socket()->Disconnect() in some cases but not in others that I think still lead to Job destruction.) https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:330: void HttpStreamFactoryImpl::Job::Orphan() { A parallel comment to the one on this methods declaration which may render that moot: We've moved the concept of "orphaned" partially up into the JobController (as evidenced by the fact that there's no longer an IsOrphaned() method on this class). Could we move it completely up there? The tests below, except for blocking_job_ which I *think* the controller has independent information on, are all reaching into the controller for information. Can we instead, if the controller wants to Orphan() a job, have it just call a method that does the disconnect and then delete the job, if the appropriate conditions are satisfied? https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:346: // the Request class and isn't retrievable by this job. nit: Is the comment reference to the Request class still valid? https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:350: job_controller_->OnOrphanedJobComplete(this); consistency nit: Elsewhere in this file there are comments if the controller may have deleted this on controller notifications; should there be one here too? (And above.) https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:93: // Used to detach the Job. It's still not clear to me what this means in the context of a caller of the Job. What does it change about the functionality the Job exports? How will the job behave differently after this function is called? Maybe "Notify the Job that it's result will not be used directly; job may cancel itself if there is no value to completing the connection." (Though that's based on my reading of the code and hence may be wrong.) https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:105: HttpStream* ReleaseStream() { return stream_.release(); } This is passing ownership; why doesn't it return a std::unique_ptr<>? https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:141: void HttpStreamFactoryImpl::JobController::OnStreamReady( Sorry, I'm missing something at the design/architectural level. Under what circumstances will we have a ready stream when we *don't* have a separate bound job, and we won't want to Orphan the other jobs? And shouldn't there be a MaybeNotifyFactoryOfCompletion() here somewhere, since this job is complete? (Probably there's just some pathway I've missed, but I'd be grateful if you could point it out.) https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:155: return; nit, suggestion: Move this up above the "MarkRequestComplete()" call? Having it here implies there's something in MarkRequestComplete that doesn't depend on request being present, which is confusing (and not true :-}). https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:160: if (job) nit: Seems weird to indirect through job in ReleaseStream() above and test it here; can it really be null? https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:114: void RemoveRequestFromSpdySessionRequestMap(Job* job); The style guide discourages using overloading, i.e. two methods with the same name but different arguments; specifically, they discourage doing it unless it's pretty clear from the call site what different functionality (if any) will be invoked. My inclination is to say that this isn't such a situation; can you rename one of the two methods? https://google.github.io/styleguide/cppguide.html#Function_Overloading
With regard to the tests, I find myself tripping over the idea that we're adding a new class, but not adding new unit tests specifically targeting that class. IIUC, what's currently in the CL is just an expansion of the Request unit tests to work given the new structure of the JobController. Given that the JobController can exist without a Request, how would you feel about writing unit tests specifically targeting the JobController? I also have some interest in testing the Request class separately from the JobController class; my sense is that that interface is clean enough that it wouldn't be too hard to write a JobController mock and poke only and specifically at the Request logic. But that might require making JobController interface, and a JobControllerImpl subclass, and that may be more hassle than it's worth. I'm not dead set on this idea--the classes are all pretty intertwined. But given that I think the Request/JobController interface is clean enough that it might be possible to write separate unit tests across that boundary, I wanted to at least explore the question. https://codereview.chromium.org/1941083002/diff/240001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:667: TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) { Why is this test being disabled as part of this CL? The bug seems to indicate that it's already disabled? https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (left): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1476: ->num_orphaned_jobs()); Is this a decrease in test coverage? I didn't see a replacement for this check in the other unit test.
Thanks Randy for review, I will leave part of you comments unaddressed and take a look with you together on the side-by-side review. :D https://codereview.chromium.org/1941083002/diff/240001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:667: TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) { On 2016/05/16 22:01:10, Randy Smith - Not in Fridays wrote: > Why is this test being disabled as part of this CL? The bug seems to indicate > that it's already disabled? It's just a manually temp fix. It's in origin/master already. I just didn't sync the code up to date as there's another CL depending on this one. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:238: connection_.reset(); On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > Why is this dependent on state? And why is it needed at all? connection_ is > owned by this class, and hence will be destroyed on destruction of Job. > > (The reason I ask this question is that I want to make sure that the pathway of > JobController outlives request, and then system shutdown results in > HttpStreamFactoryImpl destruction, which destroys the JobController works. But > what is or is not destroyed by hand on the different destruction paths seems > inconsistent; specifically, I'm confused about Orphan() explicitly calling > connection_->socket()->Disconnect() in some cases but not in others that I think > still lead to Job destruction.) Honestly, I don't have a quick answer. I take this CL and the subsequent one as more "refactoring" work since there's no behavior change introduced. But I can definitely dig it down and have a look with you together if you really want to get an answer, https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:330: void HttpStreamFactoryImpl::Job::Orphan() { On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > A parallel comment to the one on this methods declaration which may render that > moot: We've moved the concept of "orphaned" partially up into the JobController > (as evidenced by the fact that there's no longer an IsOrphaned() method on this > class). Could we move it completely up there? The tests below, except for > blocking_job_ which I *think* the controller has independent information on, are > all reaching into the controller for information. Can we instead, if the > controller wants to Orphan() a job, have it just call a method that does the > disconnect and then delete the job, if the appropriate conditions are satisfied? We could, but the removal of blocking_job_ and waiting_job_ is not in this CL, so we probably won't want to do it here. In addition, "if the controller wants to Orphan() a job, have it just call a method that does the disconnect and then delete the job": I thought this is what orphan will do eventually once we removed blocking_job and waiting_job (which are used for Job scheduling). I didn't plan to send it early, but perhaps it's a good ref: https://codereview.chromium.org/1952423002/diff/20001/net/http/http_stream_fa... Note we might want to move calls around to the caller of Job::Orphan and rename the method in the final patch. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:346: // the Request class and isn't retrievable by this job. On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > nit: Is the comment reference to the Request class still valid? Yup. The only thing I changed is the word "accessible" as we used to have a request_ entry in Job. But now we need to query JobController to retrieve Request. There's no change to the ownership of WebSocketHandshakeStreamBase::CreateHelper. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:350: job_controller_->OnOrphanedJobComplete(this); On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > consistency nit: Elsewhere in this file there are comments if the controller may > have deleted this on controller notifications; should there be one here too? > (And above.) Done. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:105: HttpStream* ReleaseStream() { return stream_.release(); } On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > This is passing ownership; why doesn't it return a std::unique_ptr<>? Done. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:141: void HttpStreamFactoryImpl::JobController::OnStreamReady( On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > Sorry, I'm missing something at the design/architectural level. Under what > circumstances will we have a ready stream when we *don't* have a separate bound > job, and we won't want to Orphan the other jobs? And shouldn't there be a > MaybeNotifyFactoryOfCompletion() here somewhere, since this job is complete? > > (Probably there's just some pathway I've missed, but I'd be grateful if you > could point it out.) We want to notify the factory when a JobController completes its work, which means there's no request_, alternative_job_ and no main_job_. There're several places we will delete request, alternative job, and main_job: 1. OnRequestComplete: it cancel orphaned job, deletes request_, and the bound_job. 2. OnPreconnectsComplete: it deletes main_job (the only job for preconnect) 3. OnOrphanedJobComplete: it deletes orphaned_job. Basically, you can search where we called reset on main_job_, alternative_job_, or where we set request_ to null. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:155: return; On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > nit, suggestion: Move this up above the "MarkRequestComplete()" call? Having it > here implies there's something in MarkRequestComplete that doesn't depend on > request being present, which is confusing (and not true :-}). Done. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:114: void RemoveRequestFromSpdySessionRequestMap(Job* job); On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > The style guide discourages using overloading, i.e. two methods with the same > name but different arguments; specifically, they discourage doing it unless it's > pretty clear from the call site what different functionality (if any) will be > invoked. My inclination is to say that this isn't such a situation; can you > rename one of the two methods? > > https://google.github.io/styleguide/cppguide.html#Function_Overloading Done. https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:94: // the socket for |this| Job, and notify JobController upon completion. This comment will be updated on next CL if we modify the implementation or move some logic back to JobController. But for the time being, I think it's it.
I'm quite happy with the design in this CL. Randy has some more questions, and suggestions for additional testing, which I agree with. But other than that, I'm happy with this CL. https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:124: JobControllerSet job_controller_set_; nit: Can you add a comment here? https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:123: JobType job_type() const { return job_type_; } I wonder if this is actually necessary. The job only uses this member to DCHECK() about pre-connecting. So perhaps a bool is_preconnect_ would suffice. The other place this method is used is in the factory, but since the factory explicitly has main_job_ and alternative_job_ members, I think this field is redundant. What do you think?
Patchset #12 (id:280001) has been deleted
Thanks all for reviewing this XL CL and apologize again for the inconvenience in reviewing. I agreed on adding unittest to increase test coverage. I will work on that in a follow-up CL for simplicity. Also, feel free to grab me to walk you through the code if needed =) Just take a look on this CL to make sure we're almost on the final stage of this CL so that moving on to subsequent CL is more convenient. Thanks! https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:238: connection_.reset(); On 2016/05/16 23:08:53, Zhongyi Shi wrote: > On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > > Why is this dependent on state? And why is it needed at all? connection_ is > > owned by this class, and hence will be destroyed on destruction of Job. > > > > (The reason I ask this question is that I want to make sure that the pathway > of > > JobController outlives request, and then system shutdown results in > > HttpStreamFactoryImpl destruction, which destroys the JobController works. > But > > what is or is not destroyed by hand on the different destruction paths seems > > inconsistent; specifically, I'm confused about Orphan() explicitly calling > > connection_->socket()->Disconnect() in some cases but not in others that I > think > > still lead to Job destruction.) > > Honestly, I don't have a quick answer. I take this CL and the subsequent one as > more "refactoring" work since there's no behavior change introduced. But I can > definitely dig it down and have a look with you together if you really want to > get an answer, Confirmed with Randy on offline discussion. The orphaned job is not a raw pointer in new code. In case of confusion, I'd like to summarize the code path leading to deletion of Job. 1. Request is bound with a Job. We keep a ref (bound_job_) to that Job. And then we recognize the other Job if any as orphaned. when the Request is served or canceled, the bound_job_ will be deleted: See JobController::OnRequestComplete() which is called by the dxtor of Request. And the orphaned job can continue or might be canceled already. 2. Request is not bound to any Job. When request is canceled, it will call JobController::OnRequestComplete() -> JobController::CancelJobs(), which eventually deletes all Job(s). 3. If we have HttpStreamFactoryImpl destructs, which then deletes JobController. We then deletes all Job(s) in JobController::~JobController(). https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/260001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:123: JobType job_type() const { return job_type_; } On 2016/05/17 17:02:16, Ryan Hamilton wrote: > I wonder if this is actually necessary. The job only uses this member to > DCHECK() about pre-connecting. So perhaps a bool is_preconnect_ would suffice. > The other place this method is used is in the factory, but since the factory > explicitly has main_job_ and alternative_job_ members, I think this field is > redundant. > > What do you think? Hmmm, we'll need job_type_ as we have more DCHECK() in JobController 2 to differentiate MAIN and ALTERNATIVE Jobs unless we want to remove those DCHECKs. In that case, I prefer to have job_type_ and expose it to public though for the time being the JobController only wants to differentiate whether it's a preconnect Job. Does this make sense?
tbansal@chromium.org changed reviewers: + tbansal@chromium.org
I was looking at this CL in context of https://codereview.chromium.org/1982443003/ which conflicts with this refactoring. I am going to wait until Zhongyi is done with this refactoring. While I am here, I left some comments and questions. https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:35: job_controller_set_.clear(); Is this really needed? I think the set and all its elements should be destroyed automatically. https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:223: DCHECK(session); May be add a DCHECK that if job_type_ is alternative, then alternative_service_ is also initialized. https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:106: std::unique_ptr<HttpStream> GetStream() { return std::move(stream_); } nit: These two functions should be defined in the c++ file. https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:362: JobType job_type_; This can be a const variable. Right? https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:20: bool for_websockets() { return factory_->for_websockets_; } const method? https://codereview.chromium.org/1941083002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:26: HttpStreamRequest::Delegate* delegate, Is this delegate always same as the one passed in the constructor? In that case, would it be simpler to remove it from this method?
Patchset #13 (id:320001) has been deleted
Argh, forgot to send this out. Adding two interfaces: RequestVisitorInterface and JobVisitorInterface which interacts with Request/Job only. The JobController implements both of them. I think for unittests, we don't have too much to test for Request as no new feature is involved in CL. The main focus of the unittests is the interactions between JobController and Request, JobController and Impl, JobController and Job. And to perform that, we might want to introduce a new interface for Job (Request has a delegate to help mock already, so does HttpStreamFacotry) to do mocks for Test. But that sounds not necessarily though it brings easiness when creating mocks. Please correct me if I'm heading for the wrong direction, thanks!
On 2016/05/21 00:08:52, Zhongyi Shi wrote: > Argh, forgot to send this out. Adding two interfaces: RequestVisitorInterface > and JobVisitorInterface which interacts with Request/Job only. The JobController > implements both of them. > > I think for unittests, we don't have too much to test for Request as no new > feature is involved in CL. > > The main focus of the unittests is the interactions between JobController and > Request, JobController and Impl, JobController and Job. Agreed. > And to perform that, we > might want to introduce a new interface for Job (Request has a delegate to help > mock already, so does HttpStreamFacotry) to do mocks for Test. But that sounds > not necessarily though it brings easiness when creating mocks. Please correct me > if I'm heading for the wrong direction, thanks! I'm not sure I'm following. If you mean, when testing any of the individual classes listed above we might want to have mocks for all the others with which it interacts, I follow that argument. And the new visitor interfaces you've put in will make it easier to unit test Request and Job. But I'm not sure what new interface you're suggestion for Job. Is it to make Job easier to test (which would be parallel to the Delegate interface for Request)? Or to make other classes easier to test with Job, to have some interface that Job implements that you can implement with a Mock class? (In general, I'm in favor of minimizing the complexity put into production code for testing. Sometimes there's really no better way than the split a class into interface and implementation to allow it to be mocked for tests. But generally, if they exist, I prefer other alternatives.)
This is very close. A couple of re-pings on earlier comments it looks like you missed, and one top level issue: I like the new interfaces you added for defining and controlling the communication between JobController/Request and JobController/Job. However, I'm concerned about calling the new interfaces that JobController implements to Request and Job "Visitor" interfaces. I'm used to that naming as being used when you're traversing a complex data structure like a tree, and getting callouts for each element in that tree; that doesn't apply here. To me, these look like delegate interfaces and I'd vote for calling them that. Naming them that, of course, will conceptually collide with the existing HttpStreamRequest::Delegate argument to the Request() constructor. But I think I'd rather have that conceptual collision with an explanatory comment (the HttpStreamRequest::Delegate is being called on behalf of the HttpStreamRequest with a result from the overall operation, the RequestDelegate (or Request::Delegate, to make it clear it's scoped to that class) is being called as part of the internal mechanics of the Request class) than name the implemented classes "Visitor". Or I'd be open to some other naming suggestion. Cherie: WDYT? Ryan: Do you have any thoughts in this space? (Including "Randy, you're being too pedantic" :-}.) https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:330: void HttpStreamFactoryImpl::Job::Orphan() { On 2016/05/16 23:08:53, Zhongyi Shi wrote: > On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > > A parallel comment to the one on this methods declaration which may render > that > > moot: We've moved the concept of "orphaned" partially up into the > JobController > > (as evidenced by the fact that there's no longer an IsOrphaned() method on > this > > class). Could we move it completely up there? The tests below, except for > > blocking_job_ which I *think* the controller has independent information on, > are > > all reaching into the controller for information. Can we instead, if the > > controller wants to Orphan() a job, have it just call a method that does the > > disconnect and then delete the job, if the appropriate conditions are > satisfied? > > We could, but the removal of blocking_job_ and waiting_job_ is not in this CL, > so we probably won't want to do it here. In addition, "if the controller wants > to Orphan() a job, have it just call a method that does the disconnect and then > delete the job": I thought this is what orphan will do eventually once we > removed blocking_job and waiting_job (which are used for Job scheduling). I > didn't plan to send it early, but perhaps it's a good ref: > https://codereview.chromium.org/1952423002/diff/20001/net/http/http_stream_fa... > > > Note we might want to move calls around to the caller of Job::Orphan and rename > the method in the final patch. I'm good with doing it in anther CL. I'm happy to discuss what the method should be named in that CL :-}. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:346: // the Request class and isn't retrievable by this job. On 2016/05/16 23:08:53, Zhongyi Shi wrote: > On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > > nit: Is the comment reference to the Request class still valid? > > Yup. The only thing I changed is the word "accessible" as we used to have a > request_ entry in Job. But now we need to query JobController to retrieve > Request. There's no change to the ownership of > WebSocketHandshakeStreamBase::CreateHelper. I'll worry about the architecture in the next CL, so this comment doesn't request any action, but just for explanation: The point of this refactor (to my mind) is making it so that the Job class doesn't need to know about the Request class. Having a reference from Job->Request, even if only in a comment, suggests that we haven't completely managed this. But that's tied in with my other concerns about the "Orphan()" interface, so it's more approriate to deal wiht them in the next CL. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:160: if (job) On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > nit: Seems weird to indirect through job in ReleaseStream() above and test it > here; can it really be null? Ping? https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (left): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1476: ->num_orphaned_jobs()); On 2016/05/16 22:01:10, Randy Smith - Not in Fridays wrote: > Is this a decrease in test coverage? I didn't see a replacement for this check > in the other unit test. Ping?
Had an Offline discussion with Ryan earlier this morning. To avoid the conflict with HttpStreamRequest::Delegate. We will rename the interface to be HttpStreamFactoryImpl::Request::Helper, and the other one will be renamed to HttpStreamFactoryImpl::Job::Delegate. Unittests will be merged to this CL soon. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:160: if (job) On 2016/05/23 21:39:26, Randy Smith - Not in Fridays wrote: > On 2016/05/16 20:46:39, Randy Smith - Not in Fridays wrote: > > nit: Seems weird to indirect through job in ReleaseStream() above and test it > > here; can it really be null? > > Ping? It shouldn't be null here as revealed in the method name. OnStreamReady should always have the stream. But forcing a DCHECK here shouldn't hurt too much I think. https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (left): https://codereview.chromium.org/1941083002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1476: ->num_orphaned_jobs()); On 2016/05/23 21:39:26, Randy Smith - Not in Fridays wrote: > On 2016/05/16 22:01:10, Randy Smith - Not in Fridays wrote: > > Is this a decrease in test coverage? I didn't see a replacement for this > check > > in the other unit test. > > Ping? Yeah, we don't have any replacement here. The old code adds all the orphaned jobs to http_stream_factory, and deletes them. However, in the new code, we hide this in JobController. Since this is a FacotryTest, I won't be too worried on that.
LGTM. Thanks for your patience!
Thanks a lot for reviewing this XL CL. And it's becoming even larger. With offline discussion with rch@, I moved the unittests to this CL for landing purpose. rdsmith@: feel free to skip if you don't want to review unittests. Per discussion earlier this afternoon, we will need to create one more class JobFactory to vending Jobs to test with MockJob. Haven't done this yet, should be really simple. Sending this out before I head more the JobFacotry so as to collect early inputs on the unittests. Thanks!
On 2016/05/27 00:27:33, Zhongyi Shi wrote: > Thanks a lot for reviewing this XL CL. And it's becoming even larger. > > With offline discussion with rch@, I moved the unittests to this CL for landing > purpose. rdsmith@: feel free to skip if you don't want to review unittests. Per > discussion earlier this afternoon, we will need to create one more class > JobFactory to vending Jobs to test with MockJob. Haven't done this yet, should > be really simple. Sending this out before I head more the JobFacotry so as to > collect early inputs on the unittests. Thanks! Friendly ping on this thread?
On 2016/06/01 02:46:55, Zhongyi Shi wrote: > On 2016/05/27 00:27:33, Zhongyi Shi wrote: > > Thanks a lot for reviewing this XL CL. And it's becoming even larger. > > > > With offline discussion with rch@, I moved the unittests to this CL for > landing > > purpose. rdsmith@: feel free to skip if you don't want to review unittests. > Per > > discussion earlier this afternoon, we will need to create one more class > > JobFactory to vending Jobs to test with MockJob. Haven't done this yet, should > > be really simple. Sending this out before I head more the JobFacotry so as to > > collect early inputs on the unittests. Thanks! > > Friendly ping on this thread? If all you've done is add the unit tests here, I'm happy to cede that review to Ryan; the primary thing I care about is the overall architecture of the production code. I'm also happy to review them if you'd specifically like my eyes on them.
overall, the controller test is looking good. a big improvement. It'd be great if we could use the peer even less. I think you're *almost* there, perhaps just a JobFactory. Also, I think the notion of "Bound" job is pretty much internal to the Controller so probably no reason to test this internal state and just rely on the Job -> Controller -> Request calls as you've already done. https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:29: new TestJobControllerPeer(GetParam(), &request_delegate_)) {} where is TestJobControllerPeer defined? https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:37: std::unique_ptr<TestJobControllerPeer> job_controller_peer_; Instead of a unique_ptr, can you just make this a non-pointer member? https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:60: job_controller_peer_->SetMainJob(main_job); It would be better if we didn't have to use a peer here. The controller could take a JobFactory which would normally create a Job*, but the test could pass in a factory which returned a TestJob*. https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:64: // of the stream failure. I think you can remove references to binding the job (and the tests too) since that's not visible via the public interfaces. https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:182: SSL_FAILURE_NONE); nice!
Patchset #16 (id:400001) has been deleted
Patchset #16 (id:420001) has been deleted
JobFactory added!! Basically it's just a wrapper to vend Job. Adn TestJobFactory will need the test to manually create the TestJobs in order to vend TestJob to controller for testing purpose. PTAL! https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:29: new TestJobControllerPeer(GetParam(), &request_delegate_)) {} On 2016/06/01 18:43:07, Ryan Hamilton wrote: > where is TestJobControllerPeer defined? Oh, it's defined in test_util.h, but I have moved it here now. https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:37: std::unique_ptr<TestJobControllerPeer> job_controller_peer_; On 2016/06/01 18:43:07, Ryan Hamilton wrote: > Instead of a unique_ptr, can you just make this a non-pointer member? Done. https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:60: job_controller_peer_->SetMainJob(main_job); On 2016/06/01 18:43:07, Ryan Hamilton wrote: > It would be better if we didn't have to use a peer here. The controller could > take a JobFactory which would normally create a Job*, but the test could pass in > a factory which returned a TestJob*. I added a JobFactory to impl and pass that in JobController, JobFactory::CreateJob() will create HttpStreamFactoryImpl::Job for controller. When testing, we need to call AddMainJobToFactory which will returns a TestJob* to the test for mock, and add that to TestJobFactory. When requested for Job, TestJobFactory will return TestJob to controller so that we can easily set up mock expectations. Does this make sense? https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:64: // of the stream failure. On 2016/06/01 18:43:07, Ryan Hamilton wrote: > I think you can remove references to binding the job (and the tests too) since > that's not visible via the public interfaces. All the references to bound_job and job_bound are removed as we could deduce the status from whether request is notified by the job: if notified by |job|, the request is bound with |job|. Otherwise, not bound. https://codereview.chromium.org/1941083002/diff/380001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:312: This test is removed as MaybeNotifyFactoryOfCompletion is private method, and it's tested inherently in other tests where we checked IsJobControllerDeleted().
I'm loving the way these tests make it clear what the contract is between the request, controller and job! https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:123: DISALLOW_COPY_AND_ASSIGN(TestJobControllerPeer); As discussed offline, I think you can eliminate the peer (or at least not use it to touch the private members of job_controller). TO do this, CreateJob in addition to returning a new MockJob can save the MockJob* to some "list of jobs" or to "first_job_"/"second_job_" or some such. The factory could also do EXPECT_CALL(new_job, Start(..)).WillOnce(DoWhatever()), if necessary. https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory.cc (right): https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory.cc:24: return (new Job(delegate, job_type, session, request_info, priority, nit: no need for ()s around the new. https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory.h (right): https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory.h:17: class HttpStreamFactoryImpl::JobFactory { As discussed offline, I think I would make this an interface in JobController. The HttpStreamFactoryImpl can create a private JobFactory subclass to return HttpStreamFactoryImpl::Jobs and the tests can return MockJobs (or the like)
Sorry for sending it out late. I can walk you over through the code if needed. PTAL! Thanks! Cherie https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:123: DISALLOW_COPY_AND_ASSIGN(TestJobControllerPeer); On 2016/06/03 00:11:24, Ryan Hamilton wrote: > As discussed offline, I think you can eliminate the peer (or at least not use it > to touch the private members of job_controller). TO do this, CreateJob in > addition to returning a new MockJob can save the MockJob* to some "list of jobs" > or to "first_job_"/"second_job_" or some such. > > The factory could also do > EXPECT_CALL(new_job, Start(..)).WillOnce(DoWhatever()), if necessary. Done. https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory.cc (right): https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory.cc:24: return (new Job(delegate, job_type, session, request_info, priority, On 2016/06/03 00:11:24, Ryan Hamilton wrote: > nit: no need for ()s around the new. Done. https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory.h (right): https://codereview.chromium.org/1941083002/diff/440001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory.h:17: class HttpStreamFactoryImpl::JobFactory { On 2016/06/03 00:11:24, Ryan Hamilton wrote: > As discussed offline, I think I would make this an interface in JobController. > The HttpStreamFactoryImpl can create a private JobFactory subclass to return > HttpStreamFactoryImpl::Jobs and the tests can return MockJobs (or the like) Done. Yay!!
Looking great! https://codereview.chromium.org/1941083002/diff/460001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:667: TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) { nit: Remove this from this CL? https://codereview.chromium.org/1941083002/diff/460001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:696: TEST_F(CertVerifyProcTest, DISABLED_PublicKeyHashes) { ditto https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:31: job_factory_(new JobFactoryImpl()), Doesn't this need to be a constructor argument so that we can pass in the test factory in the tests? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:28: class TestJobControllerPeer; I think this is unused? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:78: friend class TestJobFactory; Does these need to be friends? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:83: class NET_EXPORT_PRIVATE JobFactoryImpl; nit: I think we don't need NET_EXPORT_PRIVATE on any of these. Can you remove them and see if it still works? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:137: std::unique_ptr<JobFactoryImpl> job_factory_; nit: comment https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:51: &host_port_pair); nit: Is this line needed? I'm guessing not, because we don't set up any host rules later. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:62: job->SetStream(http_stream); nit: Any reason not to simply inline the call to SetStream() where this method is called? Doesn't seem like it's adding much value. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:172: // We have |main_job| with unknown status when |alternative_job| is failed nit: In the comments, instead of |main_job| and |alternative_job|, I'd just say "the main job" and "the alternative job" since main_job isn't actually the name of a variable. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:186: SSLConfig(), SSL_FAILURE_NONE); It would be good to use a different error code here than in the first failure and to check for that error in the EXPECT_CALL. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:221: request_.reset(); If request_ is reset, then there's no way the controller *could* notify the request, right? which I think makes the comment below a bit moot. Does this test still pass if the request is not reset? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:265: } Woo hoo! https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory_impl.h:17: class HttpStreamFactoryImpl::JobFactoryImpl I think you can just define the Impl in http_stream_factory.cc. You could also add a static method: HttpStreamFactoryImpl::DefaultFactory() which could be called but HttpNetworkSession when constructing an HttpStreamFactory. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:175: } // namespace net Wow, this file is SO much cleaner. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:42: virtual void SetPriority(RequestPriority priority) = 0; nit: comment on these methods and on the class itself, please. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:62: // Called when the |visitor_| determines the appropriate |spdy_session_key| vistor or helper? (here and elsewhere) https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:134: // Safe pointer as |this| Request doesn't outlive the |helper_|. I would write this as: // Unowned. The helper must outlive this request. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_test_util.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.cc:16: TestJobFactory::TestJobFactory() I think the .h and .cc files are in a different order. Can you arrange them to be in the same order? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.cc:31: NetLog* net_log) { DCHECK(!main_job_)? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.cc:53: alternative_job_ = new TestHttpStreamFactoryImplJob( DCHECK(!alternative_job_)? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_test_util.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.h:22: class TestHttpStreamRequestDelegate : public HttpStreamRequest::Delegate { Since these classes MOCK_ the various methods, instead of providing some sort of actual implementation, I think they should be called MockFoo.
https://codereview.chromium.org/1941083002/diff/460001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:667: TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) { On 2016/06/06 17:57:31, Ryan Hamilton wrote: > nit: Remove this from this CL? will do. I should do a sync to origin/master after and investigate whether we have platform issues. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:31: job_factory_(new JobFactoryImpl()), On 2016/06/06 17:57:31, Ryan Hamilton wrote: > Doesn't this need to be a constructor argument so that we can pass in the test > factory in the tests? The HttpStreamFactoryImpl is the owner of factory. I think it's enough for controller to take a constructor argument so when we test controller, we could pass in the test factory. We create the controller explicitly in the test, and set a HttpStreamFactoryImpl to link with controller rather than letting the HttpStreamFactoryImpl to create the controller. Does this make sense? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:78: friend class TestJobFactory; On 2016/06/06 17:57:31, Ryan Hamilton wrote: > Does these need to be friends? Unfortunately, yes. Those all access to HttpStreamFactoryImpl::Job which is a private member of HttpStreamFactoryImpl, thus we have to list those classes as friends. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:83: class NET_EXPORT_PRIVATE JobFactoryImpl; On 2016/06/06 17:57:31, Ryan Hamilton wrote: > nit: I think we don't need NET_EXPORT_PRIVATE on any of these. Can you remove > them and see if it still works? I can try after finish all the other part and sync to origin/master and do a git cl try there then. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:137: std::unique_ptr<JobFactoryImpl> job_factory_; On 2016/06/06 17:57:31, Ryan Hamilton wrote: > nit: comment Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:51: &host_port_pair); On 2016/06/06 17:57:31, Ryan Hamilton wrote: > nit: Is this line needed? I'm guessing not, because we don't set up any host > rules later. Removed. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:62: job->SetStream(http_stream); On 2016/06/06 17:57:31, Ryan Hamilton wrote: > nit: Any reason not to simply inline the call to SetStream() where this method > is called? Doesn't seem like it's adding much value. Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:172: // We have |main_job| with unknown status when |alternative_job| is failed On 2016/06/06 17:57:31, Ryan Hamilton wrote: > nit: In the comments, instead of |main_job| and |alternative_job|, I'd just say > "the main job" and "the alternative job" since main_job isn't actually the name > of a variable. Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:186: SSLConfig(), SSL_FAILURE_NONE); On 2016/06/06 17:57:31, Ryan Hamilton wrote: > It would be good to use a different error code here than in the first failure > and to check for that error in the EXPECT_CALL. Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:221: request_.reset(); On 2016/06/06 17:57:31, Ryan Hamilton wrote: > If request_ is reset, then there's no way the controller *could* notify the > request, right? which I think makes the comment below a bit moot. Does this test > still pass if the request is not reset? The test will also pass if we don't reset the request_. I was mocking the request is successfully served and gone. But it should be okay to have it served but not reset, and the second job's failure should not be reported. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:221: request_.reset(); On 2016/06/06 17:57:31, Ryan Hamilton wrote: > If request_ is reset, then there's no way the controller *could* notify the > request, right? which I think makes the comment below a bit moot. Does this test > still pass if the request is not reset? The test will also pass if we don't reset the request_. I was mocking the request is successfully served and gone. But it should be okay to have it served but not reset, and the second job's failure should not be reported. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory_impl.h:17: class HttpStreamFactoryImpl::JobFactoryImpl On 2016/06/06 17:57:31, Ryan Hamilton wrote: > I think you can just define the Impl in http_stream_factory.cc. You could also > add a static method: > > HttpStreamFactoryImpl::DefaultFactory() > > which could be called but HttpNetworkSession when constructing an > HttpStreamFactory. You mean http_stream_factory.cc or http_stream_factory_impl.cc? https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:42: virtual void SetPriority(RequestPriority priority) = 0; On 2016/06/06 17:57:31, Ryan Hamilton wrote: > nit: comment on these methods and on the class itself, please. Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:62: // Called when the |visitor_| determines the appropriate |spdy_session_key| On 2016/06/06 17:57:31, Ryan Hamilton wrote: > vistor or helper? (here and elsewhere) Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:134: // Safe pointer as |this| Request doesn't outlive the |helper_|. On 2016/06/06 17:57:31, Ryan Hamilton wrote: > I would write this as: > > // Unowned. The helper must outlive this request. Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_test_util.cc (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.cc:16: TestJobFactory::TestJobFactory() On 2016/06/06 17:57:31, Ryan Hamilton wrote: > I think the .h and .cc files are in a different order. Can you arrange them to > be in the same order? Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.cc:31: NetLog* net_log) { On 2016/06/06 17:57:32, Ryan Hamilton wrote: > DCHECK(!main_job_)? Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.cc:53: alternative_job_ = new TestHttpStreamFactoryImplJob( On 2016/06/06 17:57:32, Ryan Hamilton wrote: > DCHECK(!alternative_job_)? Done. https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_test_util.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_test_util.h:22: class TestHttpStreamRequestDelegate : public HttpStreamRequest::Delegate { On 2016/06/06 17:57:32, Ryan Hamilton wrote: > Since these classes MOCK_ the various methods, instead of providing some sort of > actual implementation, I think they should be called MockFoo. Done.
Patchset #19 (id:500001) has been deleted
Patchset #19 (id:520001) has been deleted
Patchset #19 (id:540001) has been deleted
Patchset #19 (id:560001) has been deleted
https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory_impl.h:17: class HttpStreamFactoryImpl::JobFactoryImpl On 2016/06/06 22:50:56, Zhongyi Shi wrote: > On 2016/06/06 17:57:31, Ryan Hamilton wrote: > > I think you can just define the Impl in http_stream_factory.cc. You could also > > add a static method: > > > > HttpStreamFactoryImpl::DefaultFactory() > > > > which could be called but HttpNetworkSession when constructing an > > HttpStreamFactory. > You mean http_stream_factory.cc or http_stream_factory_impl.cc? So we actually can't move the code to http_stream_factory_impl.cc the request_unittest.cc also uses the JobFacotryImpl. But we will manage to move the request_unittest in the Job Controller 2. Once we are in the Job Controller 2 and get rid of the JobFactoryImpl in request_unittest.cc, I will move this code to http_stream_factory_impl.cc.
Cherie: The danger of adding a review in near the end is that they may be uncomfortable with some of the design choices made :-J. So you may want to flip back to using just Ryan when he gets back next week. But I'm going to act as if the review is mine now until Ryan gets back, and we can figure out then how to move forward. Could you give me a sketch of what changes you intend to do in the next couple of CLs? The things that are bothering me about this CL are: * The large number of friend declarations on HttpStreamFactoryImpl * The large number of new classes (JobFactory, JobFactoryImpl along with all the previous onces created--I don't care about the extra classes for testing). * The movement of HttpStreamFactoryImpl::Job to be a "partial" base class with only some methods marked virtual. (In general I try to avoid the model of making a class virtual in order to create a test mock inherited from it since if you add a method to the base class you need to remember to add it to the derived class otherwise bad things, and only making some of the methods virtual makes that pattern even more fragile.) Note that I'm not saying I wouldn't stamp the CL with those things on it--I haven't come up with good alternatives in five or so minutes of thought (partially because the "clean interfaces for testing" and the "avoiding proliferation of new classes" metrics are in tension with each other). But I'd like to know where you're heading, since if you've already got a plan for how to reduce the complexity in the next couple of CLs I could probably be much more comfortable with the current structure. https://codereview.chromium.org/1941083002/diff/480001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/480001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory_impl.h:17: class HttpStreamFactoryImpl::JobFactoryImpl Suggestion: Why not have HttpStreamFactoryImpl derive from JobFactory and implement these methods? These are pretty trivial, I'm not sure we need another class/file set for them. (This would probably require JobFactory to not be a nested class of JobController, but three levels of nested classes is getting a bit extreme anyway.)
This is looking pretty good to me except for the friends issue (and a bunch of nits). https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/460001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:78: friend class TestJobFactory; On 2016/06/06 22:50:55, Zhongyi Shi wrote: > On 2016/06/06 17:57:31, Ryan Hamilton wrote: > > Does these need to be friends? > > Unfortunately, yes. Those all access to HttpStreamFactoryImpl::Job which is a > private member of HttpStreamFactoryImpl, thus we have to list those classes as > friends. In that case, I wonder if we should make HttpStreamFactoryImpl::Job public. The JobFactory needs to be able to use this class, obviously, which makes me think public would be a good idea. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_basic_st... File net/http/http_basic_stream.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_basic_st... net/http/http_basic_stream.h:31: class NET_EXPORT_PRIVATE HttpBasicStream : public HttpStream { Out of curiosity, why is this required? https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:77: friend class TestJobFactory; Let's talk about how to get rid of all these friends. If we really need some tests to access the private members of this class, then let's do it through a Peer which will make it obvious. But perhaps we can be clever. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:421: DCHECK(job_type_ != PRECONNECT); nit: DCHECK_NE(PRECONNECT, job_type_) Here and elsewhere. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:45: class NET_EXPORT_PRIVATE Delegate { nit: class comment please. (And for each of the methods below. One sentence each will suffice. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:57: virtual void OnWebSocketHandshakeStreamReady( nit: newline before. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:190: std::unique_ptr<HttpStream> GetStream() { return std::move(stream_); } nit: since this is destructive for stream_ I think this should probably be called ReleaseStream() https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:194: std::unique_ptr<BidirectionalStreamImpl> GetBidirectionalStream() { ditto. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:207: virtual void MarkOtherJobComplete(const Job& job); I'm surprised this needs to be virtual. Can you explain why? https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:21: class NET_EXPORT_PRIVATE JobFactory { nit: comment on the class and methods, please. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:58: const Job* main_job() const { return main_job_.get(); } are these methods needed outside of tests? https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:101: void OnBidirectionalStreamImplReady( nit: comments. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:24: void DeleteHttpStreamPointer(const SSLConfig& used_ssl_config, nit: can you put this in an anon namespace? https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory_impl.h:20: JobFactoryImpl(); Can add a method like this: static JobController::JobFactory* DefaultFactory(); to HttpStreamFactoryImpl and then move this class to http_stream_factory_impl.cc?
Tried multiple ways to hide DefaultJobFactory implementation to http_stream_factory_impl.cc. Unfortunately, neither of them works mainly because the dependency of class definition. 1. Tried to define a static method static JobController::JobFactory* DefaultJobFactory() in http_stream_factory_impl.h Why not work: JobController::JobFactory is incomplete type as JobController is forward declared as nested class in http_stream_factory_impl.h. 2. Tried to move JobFactory up in http_stream_factory_impl.h and then define a static method static JobController::JobFactory* DefaultJobFactory() in http_stream_factory_impl.h. Why not work: JobFactory need Job linked (and its signature uses Job*) thus need a complete Job definition. However Job is forward declared as a nested class in http_stream_factory_impl.h. I only renamed the JobFactoryImpl to DefaultJobFactory in this patch. I think we might be able to leave as is, as the main cause is the request_unittests, which will be moved to job_controller_unittest or so. By that time, we should be able to easily hide the implementation of DefaultJobFacotry in HttpStreamFactoryImpl.cc file. How does that sound? https://codereview.chromium.org/1941083002/diff/580001/net/http/http_basic_st... File net/http/http_basic_stream.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_basic_st... net/http/http_basic_stream.h:31: class NET_EXPORT_PRIVATE HttpBasicStream : public HttpStream { On 2016/06/13 19:25:26, Ryan Hamilton wrote: > Out of curiosity, why is this required? Cauz we now use TestJob in job_controller_unittests. We call JobController::OnStreamReady() to indicate Job has the stream ready but really without a stream set in TestJob. Thus we need to manually create a HttpBasicStream, and feed in to the Job before calling JobContoller https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:77: friend class TestJobFactory; On 2016/06/13 19:25:26, Ryan Hamilton wrote: > Let's talk about how to get rid of all these friends. If we really need some > tests to access the private members of this class, then let's do it through a > Peer which will make it obvious. But perhaps we can be clever. Most of the friend class can be removed by changing Request, Job and JobController to public nested classes. We will need to access job_controller_set_ in this class to setup the JobController in factory when we are testing job_controller_unittests. Thus adding a Peer here. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:421: DCHECK(job_type_ != PRECONNECT); On 2016/06/13 19:25:26, Ryan Hamilton wrote: > nit: DCHECK_NE(PRECONNECT, job_type_) > > Here and elsewhere. Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:45: class NET_EXPORT_PRIVATE Delegate { On 2016/06/13 19:25:26, Ryan Hamilton wrote: > nit: class comment please. (And for each of the methods below. One sentence each > will suffice. Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:57: virtual void OnWebSocketHandshakeStreamReady( On 2016/06/13 19:25:26, Ryan Hamilton wrote: > nit: newline before. Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:190: std::unique_ptr<HttpStream> GetStream() { return std::move(stream_); } On 2016/06/13 19:25:26, Ryan Hamilton wrote: > nit: since this is destructive for stream_ I think this should probably be > called ReleaseStream() Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:194: std::unique_ptr<BidirectionalStreamImpl> GetBidirectionalStream() { On 2016/06/13 19:25:26, Ryan Hamilton wrote: > ditto. Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:207: virtual void MarkOtherJobComplete(const Job& job); On 2016/06/13 19:25:26, Ryan Hamilton wrote: > I'm surprised this needs to be virtual. Can you explain why? Cauz the TestJob mocked this method. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:21: class NET_EXPORT_PRIVATE JobFactory { On 2016/06/13 19:25:27, Ryan Hamilton wrote: > nit: comment on the class and methods, please. Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:58: const Job* main_job() const { return main_job_.get(); } On 2016/06/13 19:25:27, Ryan Hamilton wrote: > are these methods needed outside of tests? Yup. These are only get methods and I added comments above. Do we need to use Peer? https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:101: void OnBidirectionalStreamImplReady( On 2016/06/13 19:25:27, Ryan Hamilton wrote: > nit: comments. Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:24: void DeleteHttpStreamPointer(const SSLConfig& used_ssl_config, On 2016/06/13 19:25:27, Ryan Hamilton wrote: > nit: can you put this in an anon namespace? Done. https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/580001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_factory_impl.h:20: JobFactoryImpl(); On 2016/06/13 19:25:27, Ryan Hamilton wrote: > Can add a method like this: > > static JobController::JobFactory* DefaultFactory(); > > to HttpStreamFactoryImpl and then move this class to > http_stream_factory_impl.cc? Ummm, cauz JobFactory is nested in JobController and JobController is only forward declared in HttpStreamFactoryImpl, we couldn't have this signature in the HttpStreamFactoryImpl header files?
Per offline discussion: 1. Move JobFactory upward to be nested in HttpStreamFactoryImpl, fwd declared. Defined in job.h. 2. Hide DefaultJobFactory in http_stream_factory_impl.cc 3. in request_unittests, since we need a nonconst pointer to JobFactory and HttpStreamFactoryImpl by default has one, instead of creating new instances, use HttpStreamFactoryImplPeer to fetch the job_factory. Since HttpStreamFactoryImpl is used in both request_unittests and job_controller_unittests, move HttpStreamFactoryImpl to test_util PTAL! Thanks =)
A few more cosmetic cleanups, but nothing structural. Woo hoo! I think this looks ready to ship! https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:27: class DefaultJobFactory : public HttpStreamFactoryImpl::JobFactory { nit: can you wrap this in namespace {} nit: class comment, which can be just a single line. It's pretty self explanatory... but still :> https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:77: job_controller_set_.clear(); nit: Is this required, or will the default destruction do the right thing? https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:145: // Jobs. nit: // Factory used by the controller for creating jobs. https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:534: // JobFactory manages to create Jobs. This is a bit hard to read. How about: // Factory for creating Jobs. https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:13: class TestJobControllerPeer; Since this already has Peer in the name, I'd ditch Test https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_test_util.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_test_util.h:91: class TestHttpStreamFactoryImplJob : public HttpStreamFactoryImpl::Job { nit: this should be Mock
Can't wait to see this being shipped out!! Thanks for all your guys help and patience on this XXL cl! https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:27: class DefaultJobFactory : public HttpStreamFactoryImpl::JobFactory { On 2016/06/15 23:34:20, Ryan Hamilton wrote: > nit: can you wrap this in namespace {} > > nit: class comment, which can be just a single line. It's pretty self > explanatory... but still :> Done. https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:77: job_controller_set_.clear(); On 2016/06/15 23:34:20, Ryan Hamilton wrote: > nit: Is this required, or will the default destruction do the right thing? Done. https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:145: // Jobs. On 2016/06/15 23:34:20, Ryan Hamilton wrote: > nit: // Factory used by the controller for creating jobs. Done. https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:534: // JobFactory manages to create Jobs. On 2016/06/15 23:34:20, Ryan Hamilton wrote: > This is a bit hard to read. How about: > > // Factory for creating Jobs. Done. https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:13: class TestJobControllerPeer; On 2016/06/15 23:34:20, Ryan Hamilton wrote: > Since this already has Peer in the name, I'd ditch Test I actually forgot to remove it. Sorry. But I did check elsewhere, this is the only one left when we start to remove all the friends and peers. https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... File net/http/http_stream_factory_test_util.h (right): https://codereview.chromium.org/1941083002/diff/640001/net/http/http_stream_f... net/http/http_stream_factory_test_util.h:91: class TestHttpStreamFactoryImplJob : public HttpStreamFactoryImpl::Job { On 2016/06/15 23:34:20, Ryan Hamilton wrote: > nit: this should be Mock Done.
lgtm!
The CQ bit was checked by zhongyi@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/1941083002/#ps660001 (title: "address rch's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941083002/660001
Message was sent while issue was closed.
Description was changed from ========== JobController 1: Remove cross reference between Request, Job, and Impl. BUG=605398 ========== to ========== JobController 1: Remove cross reference between Request, Job, and Impl. BUG=605398 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:660001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== JobController 1: Remove cross reference between Request, Job, and Impl. BUG=605398 ========== to ========== JobController 1: Remove cross reference between Request, Job, and Impl. BUG=605398 Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Cr-Commit-Position: refs/heads/master@{#400086} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Cr-Commit-Position: refs/heads/master@{#400086}
Message was sent while issue was closed.
In the future, could you please make your CL descriptions a little clearer. There a lot of things named "*Job" and "*Impl" accross the codebase.
Message was sent while issue was closed.
On 2016/06/16 14:16:22, mmenke wrote: > In the future, could you please make your CL descriptions a little clearer. > There a lot of things named "*Job" and "*Impl" accross the codebase. Sorry about that. For a long time, we have been focused on the content of this CL while didn't notice to refine the description. Will do in the future! Thanks for letting me know.
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:660001) has been created in https://codereview.chromium.org/2073293002/ by zhongyi@chromium.org. The reason for reverting is: This CL is top 1 causing crash. Revert this patch and re-land once we fixed the bug..
Message was sent while issue was closed.
Description was changed from ========== JobController 1: Remove cross reference between Request, Job, and Impl. BUG=605398 Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Cr-Commit-Position: refs/heads/master@{#400086} ========== to ========== JobController 1: Adding a new class HttpStreamFactoryImpl::JobController and remove cross reference between HttpStreamFactoryImpl::Request, HttpStreamFactoryImpl::Job, and HttpStreamFactoryImpl::Impl. BUG=605398 Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Cr-Commit-Position: refs/heads/master@{#400086} ==========
Add regression test and the corresponding fix for crbug/621069.
LGTM, modulo one nit https://codereview.chromium.org/1941083002/diff/680001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/680001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:114: return alternative_job_->GetLoadState(); How about: return main_job_ ? main_job_->GetLoadState() : alternative_job_->GetLoadState();
Re-landing this CL after fix crbug/621069. Thanks! https://codereview.chromium.org/1941083002/diff/680001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/1941083002/diff/680001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:114: return alternative_job_->GetLoadState(); On 2016/06/17 23:14:36, Ryan Hamilton wrote: > How about: > return main_job_ ? main_job_->GetLoadState() : > alternative_job_->GetLoadState(); Done.
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/1941083002/#ps700001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941083002/700001
Message was sent while issue was closed.
Description was changed from ========== JobController 1: Adding a new class HttpStreamFactoryImpl::JobController and remove cross reference between HttpStreamFactoryImpl::Request, HttpStreamFactoryImpl::Job, and HttpStreamFactoryImpl::Impl. BUG=605398 Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Cr-Commit-Position: refs/heads/master@{#400086} ========== to ========== JobController 1: Adding a new class HttpStreamFactoryImpl::JobController and remove cross reference between HttpStreamFactoryImpl::Request, HttpStreamFactoryImpl::Job, and HttpStreamFactoryImpl::Impl. BUG=605398 Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Cr-Commit-Position: refs/heads/master@{#400086} ==========
Message was sent while issue was closed.
Committed patchset #25 (id:700001)
Message was sent while issue was closed.
Description was changed from ========== JobController 1: Adding a new class HttpStreamFactoryImpl::JobController and remove cross reference between HttpStreamFactoryImpl::Request, HttpStreamFactoryImpl::Job, and HttpStreamFactoryImpl::Impl. BUG=605398 Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Cr-Commit-Position: refs/heads/master@{#400086} ========== to ========== JobController 1: Adding a new class HttpStreamFactoryImpl::JobController and remove cross reference between HttpStreamFactoryImpl::Request, HttpStreamFactoryImpl::Job, and HttpStreamFactoryImpl::Impl. BUG=605398 Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69 Committed: https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6 Cr-Original-Commit-Position: refs/heads/master@{#400086} Cr-Commit-Position: refs/heads/master@{#400552} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6 Cr-Commit-Position: refs/heads/master@{#400552}
Message was sent while issue was closed.
Patchset #26 (id:720001) has been deleted |
