Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10537056:
Replaced static URLRequestFtpJob factory with non-static protocol handler for FTP jobs. (Closed)
|
Created:
8 years, 6 months ago by shalev Modified:
8 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionReplaced static URLRequestFtpJob factory with non-static protocol handler for FTP jobs.
Note: all tests for FTP are currently disabled. The tests have been run locally and a new mock-based test will be added before committing.
Note: this depends on
http://codereview.chromium.org/10704021/
BUG=None
TEST=net_unittests --gtest_filter=URLRequestTestFTP.* --gtest_also_run_disabled_tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146125
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed Erik's comments #Patch Set 3 : Added DCHECKs back #Patch Set 4 : Added DISALLOW_COPY_AND_ASSIGN #Patch Set 5 : Fixed ftp_auth_cache ownership #Patch Set 6 : Added DCHECKs #
Total comments: 5
Patch Set 7 : Created different factories for the different contexts in profile_impl_io_data and off_the_record #
Total comments: 4
Patch Set 8 : Merged with network_delegate change #
Total comments: 10
Patch Set 9 : Removed variables #Patch Set 10 : Added for loop in impl_io_data #Patch Set 11 : Removed whitespace #
Total comments: 20
Patch Set 12 : Fixed nits #Patch Set 13 : fixed merge #Patch Set 14 : Fixed job factories in off_the_record #
Total comments: 14
Patch Set 15 : Fixed nits #Patch Set 16 : Moved profile changes to a different cl #Patch Set 17 : Fixed ftp_auth_cache #
Total comments: 2
Patch Set 18 : Fixed nit #Patch Set 19 : Merged with latest sync #Messages
Total messages: 32 (0 generated)
http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... chrome/browser/profiles/profile_impl_io_data.cc:35: #include "net/url_request/url_request_ftp_job.h" Probably don't need url_request_ftp_job.h anymore. http://codereview.chromium.org/10537056/diff/1/net/url_request/ftp_protocol_h... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/ftp_protocol_h... net/url_request/ftp_protocol_handler.h:22: FtpTransactionFactory* ftp_transaction_factory, indentation. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.cc (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.cc:75: DCHECK(ftp_transaction_factory_); Remove. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:19: #include "net/base/network_delegate.h" presumably these includes are no longer required here? Just some forward-decls? http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:43: FtpTransactionFactory* ftp_transaction_factory() { Why are these accessors added? http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:85: DISALLOW_COPY_AND_ASSIGN(URLRequestFtpJob); DISALLOW_... should be the last line in the private member section.
I'm not sure what's the main point of this change. More indirection? I think we already have too much indirection for FTP... Anyway, this is fine if that makes sense for net/ OWNERS. http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... chrome/browser/profiles/profile_impl_io_data.cc:37: #include "net/url_request/ftp_protocol_handler.h" nit: Hey, keep the headers sorted! 1. #include net should be sorted. 2. That webkit header above should also go to its right place. http://codereview.chromium.org/10537056/diff/1/net/url_request/ftp_protocol_h... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/ftp_protocol_h... net/url_request/ftp_protocol_handler.h:29: }; nit: DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.cc (left): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.cc:38: DCHECK(request->context()); Please keep the DCHECKs. In fact, add even more: for network_delegate() and ftp_auth_cache(). http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:17: nit: Why an empty line? Keep things sorted and forward-declare where possible as Erik said. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:43: FtpTransactionFactory* ftp_transaction_factory() { On 2012/06/07 19:14:51, erikwright wrote: > Why are these accessors added? Yup, they really shouldn't be needed.
http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.cc (left): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.cc:38: DCHECK(request->context()); On 2012/06/07 19:30:21, Paweł Hajdan Jr. wrote: > Please keep the DCHECKs. In fact, add even more: for network_delegate() and > ftp_auth_cache(). I'm indifferent as to DCHECKing network_delegate etc. as they are already DCHECK'd in URLRequestFtpJob::URLRequestFtpJob. In any case, a future CL will remove this static Factory method. But yes, for now at least a DCHECK on request->context() is correct.
Hi Will, I think this is ready for you to take first look at. Conceptually, this is how we would like to proceed with the conversion. There is one significant issue that I would like you to weigh in on: Currently the job factory is shared by all contexts (main, media, and extension). This will need to change since the configuration of protocol handlers will vary from context to context. As far as I can tell, the list of protocol handlers and interceptors that are registered are already pretty much not applicable to all the contexts (should the extension context have devtools:? should the main context have extension:?). So I suggest just creating different job factories, probably in ProfileImplIOData rather than in ProfileIOData as is currently done. If any protocol handler or interceptor is used by multiple job factories, multiple instances will be created. The dependencies (i.e. FtpTransactionFactory) would still be shared if appropriate and owned by the ProfileImplIOData. WDYT? -Erik http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... net/url_request/ftp_protocol_handler.h:18: class NET_EXPORT FtpProtocolHandler : Add a class comment: // Implements a ProtocolHandler for FTP.
Hi Will, Forgot to add you as a reviewer. Please see my last comment. Thanks, Erik
We need to update OffTheRecordProfileIOData too, right? http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... File net/url_request/ftp_protocol_handler.cc (right): http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... net/url_request/ftp_protocol_handler.cc:22: } I suspect we should add DCHECKs here, since it'll just crash later if some of these are NULL, right?
Yes, the OffTheRecord profile can be updated too. It's not required to be a part of this CL since we maintain a compatibility layer, but it can be done at the same time. http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... File net/url_request/ftp_protocol_handler.cc (right): http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... net/url_request/ftp_protocol_handler.cc:22: } On 2012/06/11 21:02:27, willchan wrote: > I suspect we should add DCHECKs here, since it'll just crash later if some of > these are NULL, right? Yes.
Will, PTAL. In particular, having to configure the 2-3 job factories independently and nearly identically is not excellent, but: 1) I wonder if, in actual fact, they should not all have the same ProtocolHandlers and Interceptors but it was just a matter of convenience to have handlers/interceptors configured globally but only used via a subset of contexts. 2) It may be a reasonable option to change the ownership model so that the URLRequestJobFactory does not own its protocol handlers/interceptors. Instead the ProfileIOData (or subclass) could own them so that they could be shared when appropriate. 3) Another option is to introduce chaining so that there would be one common job factory to which the distinct media/main/extensions factories would fall-back if they didn't have a matching protocol handler/interceptor. Thoughts? -Erik http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/of... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:253: main_factory_.reset(new net::URLRequestJobFactory); rename to {main,extensions}_job_factory_ http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_impl_io_data.cc:444: main_factory_.reset(new net::URLRequestJobFactory); rename as requested in OffTheRecord...
On 2012/06/13 15:31:42, erikwright wrote: > Will, PTAL. > > In particular, having to configure the 2-3 job factories independently and > nearly identically is not excellent, but: > > 1) I wonder if, in actual fact, they should not all have the same > ProtocolHandlers and Interceptors but it was just a matter of convenience to > have handlers/interceptors configured globally but only used via a subset of > contexts. SGTM > > 2) It may be a reasonable option to change the ownership model so that the > URLRequestJobFactory does not own its protocol handlers/interceptors. Instead > the ProfileIOData (or subclass) could own them so that they could be shared when > appropriate. SGTM > > 3) Another option is to introduce chaining so that there would be one common job > factory to which the distinct media/main/extensions factories would fall-back if > they didn't have a matching protocol handler/interceptor. This is possible, but I don't like it as much. I prefer hard failure rather than graceful fallback, so we can catch unexpected configurations. > > Thoughts? > > -Erik > > http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/of... > File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): > > http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/of... > chrome/browser/profiles/off_the_record_profile_io_data.cc:253: > main_factory_.reset(new net::URLRequestJobFactory); > rename to {main,extensions}_job_factory_ > > http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/pr... > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/pr... > chrome/browser/profiles/profile_impl_io_data.cc:444: main_factory_.reset(new > net::URLRequestJobFactory); > rename as requested in OffTheRecord...
http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... chrome/browser/profiles/profile_impl_io_data.cc:35: #include "net/url_request/url_request_ftp_job.h" On 2012/06/07 19:14:51, erikwright wrote: > Probably don't need url_request_ftp_job.h anymore. Done. http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... chrome/browser/profiles/profile_impl_io_data.cc:35: #include "net/url_request/url_request_ftp_job.h" On 2012/06/07 19:14:51, erikwright wrote: > Probably don't need url_request_ftp_job.h anymore. Done. http://codereview.chromium.org/10537056/diff/1/chrome/browser/profiles/profil... chrome/browser/profiles/profile_impl_io_data.cc:37: #include "net/url_request/ftp_protocol_handler.h" On 2012/06/07 19:30:21, Paweł Hajdan Jr. wrote: > nit: Hey, keep the headers sorted! > > 1. #include net should be sorted. > 2. That webkit header above should also go to its right place. Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/ftp_protocol_h... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/ftp_protocol_h... net/url_request/ftp_protocol_handler.h:22: FtpTransactionFactory* ftp_transaction_factory, On 2012/06/07 19:14:51, erikwright wrote: > indentation. Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/ftp_protocol_h... net/url_request/ftp_protocol_handler.h:29: }; On 2012/06/07 19:30:21, Paweł Hajdan Jr. wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.cc (left): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.cc:38: DCHECK(request->context()); On 2012/06/07 19:40:40, erikwright wrote: > On 2012/06/07 19:30:21, Paweł Hajdan Jr. wrote: > > Please keep the DCHECKs. In fact, add even more: for network_delegate() and > > ftp_auth_cache(). > > I'm indifferent as to DCHECKing network_delegate etc. as they are already > DCHECK'd in URLRequestFtpJob::URLRequestFtpJob. In any case, a future CL will > remove this static Factory method. > > But yes, for now at least a DCHECK on request->context() is correct. Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.cc (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.cc:75: DCHECK(ftp_transaction_factory_); On 2012/06/07 19:14:51, erikwright wrote: > Remove. Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.cc:75: DCHECK(ftp_transaction_factory_); On 2012/06/07 19:14:51, erikwright wrote: > Remove. Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:17: On 2012/06/07 19:30:21, Paweł Hajdan Jr. wrote: > nit: Why an empty line? Keep things sorted and forward-declare where possible as > Erik said. Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:19: #include "net/base/network_delegate.h" On 2012/06/07 19:14:51, erikwright wrote: > presumably these includes are no longer required here? Just some forward-decls? Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:43: FtpTransactionFactory* ftp_transaction_factory() { On 2012/06/07 19:14:51, erikwright wrote: > Why are these accessors added? Done. http://codereview.chromium.org/10537056/diff/1/net/url_request/url_request_ft... net/url_request/url_request_ftp_job.h:85: DISALLOW_COPY_AND_ASSIGN(URLRequestFtpJob); On 2012/06/07 19:14:51, erikwright wrote: > DISALLOW_... should be the last line in the private member section. Done. http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... File net/url_request/ftp_protocol_handler.cc (right): http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... net/url_request/ftp_protocol_handler.cc:22: } On 2012/06/11 21:02:27, willchan wrote: > I suspect we should add DCHECKs here, since it'll just crash later if some of > these are NULL, right? Done. http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/3009/net/url_request/ftp_protoco... net/url_request/ftp_protocol_handler.h:18: class NET_EXPORT FtpProtocolHandler : On 2012/06/11 15:43:02, erikwright wrote: > Add a class comment: > > // Implements a ProtocolHandler for FTP. Done. http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/of... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:253: main_factory_.reset(new net::URLRequestJobFactory); On 2012/06/13 15:31:42, erikwright wrote: > rename to {main,extensions}_job_factory_ Done. http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10537056/diff/15001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_impl_io_data.cc:444: main_factory_.reset(new net::URLRequestJobFactory); On 2012/06/13 15:31:42, erikwright wrote: > rename as requested in OffTheRecord... Done.
http://codereview.chromium.org/10537056/diff/16001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/10537056/diff/16001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_io_data.h:217: net::URLRequestJobFactory* job_factory() const { Remove http://codereview.chromium.org/10537056/diff/16001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_io_data.h:327: mutable scoped_ptr<net::URLRequestJobFactory> job_factory_; Remove. http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... File net/url_request/url_request_ftp_job.cc (right): http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:27: : URLRequestJob(request, request->context()->network_delegate()), pass network_delegate instead of request->context()->network_delegate() http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:47: DCHECK(request->context()); remove this DCHECK now that context is required. http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... net/url_request/url_request_ftp_job.h:78: NetworkDelegate* network_delegate_; not used. remove.
http://codereview.chromium.org/10537056/diff/16001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/10537056/diff/16001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_io_data.h:217: net::URLRequestJobFactory* job_factory() const { On 2012/06/21 20:21:30, erikwright wrote: > Remove Done. http://codereview.chromium.org/10537056/diff/16001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_io_data.h:327: mutable scoped_ptr<net::URLRequestJobFactory> job_factory_; On 2012/06/21 20:21:30, erikwright wrote: > Remove. Done. http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... File net/url_request/url_request_ftp_job.cc (right): http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:47: DCHECK(request->context()); On 2012/06/21 20:21:30, erikwright wrote: > remove this DCHECK now that context is required. Done. http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... net/url_request/url_request_ftp_job.h:78: NetworkDelegate* network_delegate_; On 2012/06/21 20:21:30, erikwright wrote: > not used. remove. Done.
Hi Will and Matt, could one of you take a look at this? Thanks, Shalev
On 2012/06/26 17:56:23, shalev wrote: > Hi Will and Matt, > > could one of you take a look at this? > > Thanks, > > Shalev Will's busy, so I'll take a look at both CLs.
A couple nits on the net side of things, which look pretty good to me. I'm going to have to familiarize myself a bit more with the profile code. http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:261: network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); nit: Fix indent. I'd suggest: main_job_factory_->SetProtocolHandler( chrome::kFtpScheme, new net::FtpProtocolHandler( network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:264: network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); nit: Fix indent. http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/pr... chrome/browser/profiles/profile_impl_io_data.cc:550: network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); nit: Fix indent http://codereview.chromium.org/10537056/diff/34002/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/34002/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:26: private: nit: Add line break above private. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... File net/url_request/url_request_ftp_job.cc (right): http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:27: : URLRequestJob(request, request->context()->network_delegate()), Second argument should now be network_delegate. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:44: return new URLRequestErrorJob(request, ERR_UNSAFE_PORT); While you're here, could you fix the formatting of the code above? (Add brackets, indent second line of the if two more). Thanks. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:47: DCHECK(request->context()->ftp_auth_cache()); These two DCHECKs are now in URLRequestFtpJob's constructor, so I don't think they're needed here any more. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:123: ftp_auth_cache_->Lookup(origin); nit: Think this will fit on one line. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:215: server_auth_->credentials); nit: Think this will just barely fit on one line now. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.h:35: const std::string& scheme); The plan is to get rid of this function in favor of the FtpProtocolHandler, right? If so, could you add a TODO along those lines?
Hi Matt, After looking at the code again, I realized I am setting up the job factories in off_the_record_io_data differently than in profile_impl_io_data. I will change off_the_record_io_data to make it analogous to profile_impl_io_data. http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:261: network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); On 2012/06/26 18:36:10, Matt Menke wrote: > nit: Fix indent. I'd suggest: > > main_job_factory_->SetProtocolHandler( > chrome::kFtpScheme, > new net::FtpProtocolHandler( > network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); Done. http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:264: network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); On 2012/06/26 18:36:10, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/pr... chrome/browser/profiles/profile_impl_io_data.cc:550: network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); On 2012/06/26 18:36:10, Matt Menke wrote: > nit: Fix indent Done. http://codereview.chromium.org/10537056/diff/34002/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/34002/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:26: private: On 2012/06/26 18:36:10, Matt Menke wrote: > nit: Add line break above private. Done. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... File net/url_request/url_request_ftp_job.cc (right): http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:27: : URLRequestJob(request, request->context()->network_delegate()), On 2012/06/26 18:36:10, Matt Menke wrote: > Second argument should now be network_delegate. Done. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:44: return new URLRequestErrorJob(request, ERR_UNSAFE_PORT); On 2012/06/26 18:36:10, Matt Menke wrote: > While you're here, could you fix the formatting of the code above? (Add > brackets, indent second line of the if two more). > > Thanks. Done. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:47: DCHECK(request->context()->ftp_auth_cache()); On 2012/06/26 18:36:10, Matt Menke wrote: > These two DCHECKs are now in URLRequestFtpJob's constructor, so I don't think > they're needed here any more. Done. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:123: ftp_auth_cache_->Lookup(origin); On 2012/06/26 18:36:10, Matt Menke wrote: > nit: Think this will fit on one line. Done. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:215: server_auth_->credentials); On 2012/06/26 18:36:10, Matt Menke wrote: > nit: Think this will just barely fit on one line now. Done. http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... net/url_request/url_request_ftp_job.h:35: const std::string& scheme); On 2012/06/26 18:36:10, Matt Menke wrote: > The plan is to get rid of this function in favor of the FtpProtocolHandler, > right? If so, could you add a TODO along those lines? Done.
Files modified from your earlier CL are now appearing as modified in this one. Bad merge? On 2012/06/26 19:05:01, shalev wrote: > Hi Matt, > > After looking at the code again, I realized I am setting up the job factories in > off_the_record_io_data differently than in profile_impl_io_data. I will change > off_the_record_io_data to make it analogous to profile_impl_io_data. > > http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... > File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): > > http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... > chrome/browser/profiles/off_the_record_profile_io_data.cc:261: > network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); > On 2012/06/26 18:36:10, Matt Menke wrote: > > nit: Fix indent. I'd suggest: > > > > main_job_factory_->SetProtocolHandler( > > chrome::kFtpScheme, > > new net::FtpProtocolHandler( > > network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); > > Done. > > http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/of... > chrome/browser/profiles/off_the_record_profile_io_data.cc:264: > network_delegate(), ftp_factory_.get(), ftp_auth_cache_.get())); > On 2012/06/26 18:36:10, Matt Menke wrote: > > nit: Fix indent. > > Done. > > http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/pr... > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > http://codereview.chromium.org/10537056/diff/34002/chrome/browser/profiles/pr... > chrome/browser/profiles/profile_impl_io_data.cc:550: network_delegate(), > ftp_factory_.get(), ftp_auth_cache_.get())); > On 2012/06/26 18:36:10, Matt Menke wrote: > > nit: Fix indent > > Done. > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/ftp_protoc... > File net/url_request/ftp_protocol_handler.h (right): > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/ftp_protoc... > net/url_request/ftp_protocol_handler.h:26: private: > On 2012/06/26 18:36:10, Matt Menke wrote: > > nit: Add line break above private. > > Done. > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > File net/url_request/url_request_ftp_job.cc (right): > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > net/url_request/url_request_ftp_job.cc:27: : URLRequestJob(request, > request->context()->network_delegate()), > On 2012/06/26 18:36:10, Matt Menke wrote: > > Second argument should now be network_delegate. > > Done. > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > net/url_request/url_request_ftp_job.cc:44: return new > URLRequestErrorJob(request, ERR_UNSAFE_PORT); > On 2012/06/26 18:36:10, Matt Menke wrote: > > While you're here, could you fix the formatting of the code above? (Add > > brackets, indent second line of the if two more). > > > > Thanks. > > Done. > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > net/url_request/url_request_ftp_job.cc:47: > DCHECK(request->context()->ftp_auth_cache()); > On 2012/06/26 18:36:10, Matt Menke wrote: > > These two DCHECKs are now in URLRequestFtpJob's constructor, so I don't think > > they're needed here any more. > > Done. > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > net/url_request/url_request_ftp_job.cc:123: ftp_auth_cache_->Lookup(origin); > On 2012/06/26 18:36:10, Matt Menke wrote: > > nit: Think this will fit on one line. > > Done. > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > net/url_request/url_request_ftp_job.cc:215: server_auth_->credentials); > On 2012/06/26 18:36:10, Matt Menke wrote: > > nit: Think this will just barely fit on one line now. > > Done. > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > File net/url_request/url_request_ftp_job.h (right): > > http://codereview.chromium.org/10537056/diff/34002/net/url_request/url_reques... > net/url_request/url_request_ftp_job.h:35: const std::string& scheme); > On 2012/06/26 18:36:10, Matt Menke wrote: > > The plan is to get rid of this function in favor of the FtpProtocolHandler, > > right? If so, could you add a TODO along those lines? > > Done.
Hi Matt, Everything should be fixed now. Feel free to take another look. Shalev
I think this CL does a little too much. In addition to adding the ftp protocol handler (Which is all the description says), it also: 1) Partially moves FtpAuthCache out of the URLRequestContext. 2) Switches from having one URLRequestJobFactory per profile to having two or three. I'd rather see FtpAuthCache moved completely out of URLRequestContext in a single CL, largely because it's easier to verify that way. If it's not too big and can currently be done, fine to do it in this CL. I don't like having one still created in the context, but then actually using another cache instead. Seems like that could lead to bugs. Having multiple URLRequestJobFactories per profile seems largely unrelated to the main thing this CL aims to accomplish. http://codereview.chromium.org/10537056/diff/55001/chrome/browser/profiles/of... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10537056/diff/55001/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:262: CreateFtpProtocolHandler(job_factories[i]); nit: Fix indent. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.cc (right): http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.cc:6: #include "net/url_request/url_request_ftp_job.h" Include "base/logging.h" for the DCHECKs. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.cc:13: class URLRequest; These aren't needed, since they're in the header. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:9: #include "net/url_request/url_request_job_factory.h" include "base/basictypes.h" for DISALLOW_COPY_AND_ASSIGN, and "base/compiler_specific.h" for OVERRIDE. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:16: class FtpAuthCache; nit: Alphabetize. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:31: DISALLOW_COPY_AND_ASSIGN(FtpProtocolHandler); nit: Add a line break just above DISALLOW_COPY_AND_ASSIGN, to separate it from the variables. http://codereview.chromium.org/10537056/diff/55001/net/url_request/url_reques... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/55001/net/url_request/url_reques... net/url_request/url_request_ftp_job.h:23: class URLRequestContext; While you're here, can you remove the URLRequestContext forward declaration? It's not used in this file.
message: Hi Matt, You are right that this cl is doing a lot of things. A few comments: 1) The plan is to eventually get rid of URLRequestContext completely. Right now, I want to convert the static factories for ftp, http, file, about, etc. into protocol handlers which do not use URLRequestContext at all. This is what I did for ftp, and is the reason why FtpAuthCache, FtpTransactionFactory, and NetworkDelegate have been moved out of URLRequestContext in the protocol handler. I do not want to have to move everything out of Context yet. I prefer first switching the static factories to protocol handlers. Does this seem reasonable? 2) The switch from one URLRequestJobFactory per profile to two or three will allow the different contexts to have a different set of protocol handlers. If you prefer, I can move this change to a different cl and make the current cl depend on it. http://codereview.chromium.org/10537056/diff/55001/chrome/browser/profiles/of... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10537056/diff/55001/chrome/browser/profiles/of... chrome/browser/profiles/off_the_record_profile_io_data.cc:262: CreateFtpProtocolHandler(job_factories[i]); On 2012/06/27 17:20:07, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.cc (right): http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.cc:6: #include "net/url_request/url_request_ftp_job.h" On 2012/06/27 17:20:07, Matt Menke wrote: > Include "base/logging.h" for the DCHECKs. Done. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.cc:13: class URLRequest; On 2012/06/27 17:20:07, Matt Menke wrote: > These aren't needed, since they're in the header. Done. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.h (right): http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:9: #include "net/url_request/url_request_job_factory.h" On 2012/06/27 17:20:07, Matt Menke wrote: > include "base/basictypes.h" for DISALLOW_COPY_AND_ASSIGN, and > "base/compiler_specific.h" for OVERRIDE. Done. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:16: class FtpAuthCache; On 2012/06/27 17:20:07, Matt Menke wrote: > nit: Alphabetize. Done. http://codereview.chromium.org/10537056/diff/55001/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.h:31: DISALLOW_COPY_AND_ASSIGN(FtpProtocolHandler); On 2012/06/27 17:20:07, Matt Menke wrote: > nit: Add a line break just above DISALLOW_COPY_AND_ASSIGN, to separate it from > the variables. Done. http://codereview.chromium.org/10537056/diff/55001/net/url_request/url_reques... File net/url_request/url_request_ftp_job.h (right): http://codereview.chromium.org/10537056/diff/55001/net/url_request/url_reques... net/url_request/url_request_ftp_job.h:23: class URLRequestContext; On 2012/06/27 17:20:07, Matt Menke wrote: > While you're here, can you remove the URLRequestContext forward declaration? > It's not used in this file. Done.
On 2012/06/27 20:10:43, shalev wrote: > message: Hi Matt, > > You are right that this cl is doing a lot of things. A few comments: > > 1) The plan is to eventually get rid of URLRequestContext completely. Right now, > I want to convert the static factories for ftp, http, file, about, etc. into > protocol handlers which do not use URLRequestContext at all. This is what I did > for ftp, and is the reason why FtpAuthCache, FtpTransactionFactory, and > NetworkDelegate have been moved out of URLRequestContext in the protocol > handler. > > I do not want to have to move everything out of Context yet. I prefer first > switching the static factories to protocol handlers. Does this seem reasonable? The problem is that we still have UrlRequestContext::ftp_auth_handler(), and now any consumer of it is not actually getting the correct ftp_auth_handler(). It doesn't look like there are any consumers of it, but we should have a function that returns the incorrect object. You should either remove that function and the underlying field from URLRequestContext, or use the URLRequestContext's ftp_auth_handler() until you're ready to get rid of that method. Alternatively, you could give the URLRequestContext a non-owning reference to the ftp_auth_handler owned by the ProfileIOData. > 2) The switch from one URLRequestJobFactory per profile to two or three will > allow the different contexts to have a different set of protocol handlers. If > you prefer, I can move this change to a different cl and make the current cl > depend on it. I'm more comfortable with it in another CL - partly because I'm no profile expert, and partly because it seems completely unrelated to the FTP change.
On 2012/06/27 20:39:47, Matt Menke wrote: > On 2012/06/27 20:10:43, shalev wrote: > > message: Hi Matt, > > > > You are right that this cl is doing a lot of things. A few comments: > > > > 1) The plan is to eventually get rid of URLRequestContext completely. Right > now, > > I want to convert the static factories for ftp, http, file, about, etc. into > > protocol handlers which do not use URLRequestContext at all. This is what I > did > > for ftp, and is the reason why FtpAuthCache, FtpTransactionFactory, and > > NetworkDelegate have been moved out of URLRequestContext in the protocol > > handler. > > > > I do not want to have to move everything out of Context yet. I prefer first > > switching the static factories to protocol handlers. Does this seem > reasonable? > > The problem is that we still have UrlRequestContext::ftp_auth_handler(), and now > any consumer of it is not actually getting the correct ftp_auth_handler(). It > doesn't look like there are any consumers of it, but we should have a function > that returns the incorrect object. You should either remove that function and > the underlying field from URLRequestContext, or use the URLRequestContext's > ftp_auth_handler() until you're ready to get rid of that method. Alternatively, > you could give the URLRequestContext a non-owning reference to the > ftp_auth_handler owned by the ProfileIOData. Should *not* have a function that returns the incorrect object, that is.
On 2012/06/27 20:40:41, Matt Menke wrote: > On 2012/06/27 20:39:47, Matt Menke wrote: > > On 2012/06/27 20:10:43, shalev wrote: > > > message: Hi Matt, > > > > > > You are right that this cl is doing a lot of things. A few comments: > > > > > > 1) The plan is to eventually get rid of URLRequestContext completely. Right > > now, > > > I want to convert the static factories for ftp, http, file, about, etc. into > > > protocol handlers which do not use URLRequestContext at all. This is what I > > did > > > for ftp, and is the reason why FtpAuthCache, FtpTransactionFactory, and > > > NetworkDelegate have been moved out of URLRequestContext in the protocol > > > handler. > > > > > > I do not want to have to move everything out of Context yet. I prefer first > > > switching the static factories to protocol handlers. Does this seem > > reasonable? > > > > The problem is that we still have UrlRequestContext::ftp_auth_handler(), and > now > > any consumer of it is not actually getting the correct ftp_auth_handler(). It > > doesn't look like there are any consumers of it, but we should have a function > > that returns the incorrect object. You should either remove that function and > > the underlying field from URLRequestContext, or use the URLRequestContext's > > ftp_auth_handler() until you're ready to get rid of that method. > Alternatively, > > you could give the URLRequestContext a non-owning reference to the > > ftp_auth_handler owned by the ProfileIOData. > > Should *not* have a function that returns the incorrect object, that is. Okay, I agree with everything you said. I moved the factory changes in profile to a different cl: http://codereview.chromium.org/10704021/ I also changed the ownership of ftp_auth_cache back to what it was, so that it is now always owned by the context. Is this acceptable as a temporary solution?
On 2012/06/29 15:35:25, shalev wrote: > I also changed the ownership of ftp_auth_cache back to what it was, so that it > is now always owned by the context. Is this acceptable as a temporary solution? This is great. LGTM. You'll also want to have a profile OWNER take a look.
That would be me, I'll take a look. On Fri, Jun 29, 2012 at 9:08 AM, <mmenke@chromium.org> wrote: > On 2012/06/29 15:35:25, shalev wrote: > >> I also changed the ownership of ftp_auth_cache back to what it was, so >> that it >> is now always owned by the context. Is this acceptable as a temporary >> > solution? > > This is great. LGTM. > > You'll also want to have a profile OWNER take a look. > > http://codereview.chromium.**org/10537056/<http://codereview.chromium.org/105... >
lgtm
http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... File net/url_request/url_request_ftp_job.cc (right): http://codereview.chromium.org/10537056/diff/16001/net/url_request/url_reques... net/url_request/url_request_ftp_job.cc:27: : URLRequestJob(request, request->context()->network_delegate()), On 2012/06/21 20:21:30, erikwright wrote: > pass network_delegate instead of request->context()->network_delegate() Done.
http://codereview.chromium.org/10537056/diff/62003/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.cc (right): http://codereview.chromium.org/10537056/diff/62003/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.cc:6: #include "net/url_request/ftp_protocol_handler.h" This should go first, with a blank line below it.
http://codereview.chromium.org/10537056/diff/62003/net/url_request/ftp_protoc... File net/url_request/ftp_protocol_handler.cc (right): http://codereview.chromium.org/10537056/diff/62003/net/url_request/ftp_protoc... net/url_request/ftp_protocol_handler.cc:6: #include "net/url_request/ftp_protocol_handler.h" On 2012/06/29 20:49:59, Matt Menke wrote: > This should go first, with a blank line below it. Done.
LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shalev@chromium.org/10537056/66007
Change committed as 146125 |
