Command Line switch for Portable FTP (Patch SET-1)

(This CL has been committed: http://codereview.chromium.org/42197/show.)

BUG=4965
R=darin,wtc,dank,eroman

Description:

Command Line switch "--new-ftp" added for new portable FTP implementation.

[darin (slow to review), 2009-02-27 21:19:32]

http://codereview.chromium.org/28266/diff/1/5
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/28266/diff/1/5#newcode392
Line 392: const wchar_t kNewFtp[]             = L"new-ftp";
nit: it looks like bad/inconsistent indentation.  i think you should line up the
"=" signs vertically.

http://codereview.chromium.org/28266/diff/1/3
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/1/3#newcode15
Line 15: #include "chrome/common/chrome_switches.h"
sorry, it is not valid to include chrome headers from the net module.  the
decision about what FTP implementation to use should be made at a higher level. 
e.g., in ChromeURLRequestContext.

http://codereview.chromium.org/28266/diff/1/6
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/1/6#newcode21
Line 21: // #include "net/ftp/ftp_response_info.h"
same nit about commented out code.

http://codereview.chromium.org/28266/diff/1/7
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/28266/diff/1/7#newcode14
Line 14: // #include "net/ftp/ftp_request_info.h"
nit: please either delete commented out code or add a TODO(ibrar) comment
explaining what needs to be done.

http://codereview.chromium.org/28266/diff/1/7#newcode16
Line 16: using std::string;
nit: we generally avoid using statements in headers because that then taints the
namespace of anyone who includes such a header.

http://codereview.chromium.org/28266/diff/1/7#newcode24
Line 24: static URLRequestJob* Factory(URLRequest* request, const std::string&
scheme);
looks like you are writing std::string anyways, so maybe the using directive
should just be killed.

http://codereview.chromium.org/28266/diff/1/7#newcode54
Line 54: net::AuthState proxy_auth_state_;
if we require proxy auth, then we must also be using a proxy.  that means we are
using HTTP instead of FTP.  so, we would then need to create a
URLRequestHttpJob.  in other words, we should have no need for a
proxy_auth_state_ variable here.

i recommend not worrying about supporting proxies in the first version of new
ftp.

[ibrar, 2009-02-27 23:00:48]

http://codereview.chromium.org/28266/diff/1/5
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/28266/diff/1/5#newcode392
Line 392: const wchar_t kNewFtp[]             = L"new-ftp";
On 2009/02/27 21:19:33, darin wrote:
> nit: it looks like bad/inconsistent indentation.  i think you should line up
the
> "=" signs vertically.

Done.

http://codereview.chromium.org/28266/diff/1/3
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/1/3#newcode15
Line 15: #include "chrome/common/chrome_switches.h"
On 2009/02/27 21:19:33, darin wrote:
> sorry, it is not valid to include chrome headers from the net module.  the
> decision about what FTP implementation to use should be made at a higher
level. 
> e.g., in ChromeURLRequestContext.

Done.

http://codereview.chromium.org/28266/diff/1/6
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/1/6#newcode21
Line 21: // #include "net/ftp/ftp_response_info.h"
On 2009/02/27 21:19:33, darin wrote:
> same nit about commented out code.

Done.

http://codereview.chromium.org/28266/diff/1/7
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/28266/diff/1/7#newcode14
Line 14: // #include "net/ftp/ftp_request_info.h"
On 2009/02/27 21:19:33, darin wrote:
> nit: please either delete commented out code or add a TODO(ibrar) comment
> explaining what needs to be done.

Done.

http://codereview.chromium.org/28266/diff/1/7#newcode16
Line 16: using std::string;
On 2009/02/27 21:19:33, darin wrote:
> nit: we generally avoid using statements in headers because that then taints
the
> namespace of anyone who includes such a header.

Done.

http://codereview.chromium.org/28266/diff/1/7#newcode24
Line 24: static URLRequestJob* Factory(URLRequest* request, const std::string&
scheme);
On 2009/02/27 21:19:33, darin wrote:
> looks like you are writing std::string anyways, so maybe the using directive
> should just be killed.

Done.

http://codereview.chromium.org/28266/diff/1/7#newcode54
Line 54: net::AuthState proxy_auth_state_;
On 2009/02/27 21:19:33, darin wrote:
> if we require proxy auth, then we must also be using a proxy.  that means we
are
> using HTTP instead of FTP.  so, we would then need to create a
> URLRequestHttpJob.  in other words, we should have no need for a
> proxy_auth_state_ variable here.
> 
> i recommend not worrying about supporting proxies in the first version of new
> ftp.

Done.

[darin (slow to review), 2009-03-06 16:40:16]

you'll also need to update net/net.gyp and net/net_lib.scons

http://codereview.chromium.org/28266/diff/1015/1022
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/28266/diff/1015/1022#newcode69
Line 69: bool is_new_ftp() {
instead of storing a boolean here, i think it would be better to hold a pointer
to the FtpTransactionFactory (just as in the HTTP case).  reason: the
FtpTransactionFactory has state (e.g., a ClientSocketPool instance), and the
place to hold such state is the URLRequestContext.  also, by putting the factory
on the context, you let the consumer optionally specify a mock
FtpTransactionFactory, which can be very useful for unit tests.  so, i would say
that if you have a context with a null FtpTransactionFactory then that should
imply using the old FTP stack.  otherwise, the new FTP stack is used. 
eventually, that will go away and we'll require a valid FtpTransactionFactory.

http://codereview.chromium.org/28266/diff/1015/1017
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/1015/1017#newcode6
Line 6: #include "net/url_request/url_request_new_ftp_job.h"
the rule for header ordering is to put foo.h at the top of foo.cc, and then the
rest of the headers go into the alphabetically ordered sections below.

http://codereview.chromium.org/28266/diff/1015/1020
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/1015/1020#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009

http://codereview.chromium.org/28266/diff/1015/1020#newcode31
Line 31: 
nit: kill the extra newline

http://codereview.chromium.org/28266/diff/1015/1020#newcode37
Line 37: start_callback_(this, &URLRequestNewFtpJob::OnStartCompleted)),
nit: we usually indent the argument to ALLOW_THIS_IN_...

[ibrar, 2009-03-10 15:55:24]

http://codereview.chromium.org/28266/diff/1015/1022
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/28266/diff/1015/1022#newcode69
Line 69: bool is_new_ftp() {
On 2009/03/06 16:40:16, darin wrote:
> instead of storing a boolean here, i think it would be better to hold a
pointer
> to the FtpTransactionFactory (just as in the HTTP case).  reason: the
> FtpTransactionFactory has state (e.g., a ClientSocketPool instance), and the
> place to hold such state is the URLRequestContext.  also, by putting the
factory
> on the context, you let the consumer optionally specify a mock
> FtpTransactionFactory, which can be very useful for unit tests.  so, i would
say
> that if you have a context with a null FtpTransactionFactory then that should
> imply using the old FTP stack.  otherwise, the new FTP stack is used. 
> eventually, that will go away and we'll require a valid FtpTransactionFactory.


I have used the same technique in my my original patch. Actually I want it to be
in next patch set, but to use 
FtpTransactionFactory for new-ftp detection is really a good idea. I never
thought about this. Now I have done the same.

Done.

http://codereview.chromium.org/28266/diff/1015/1020
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/1015/1020#newcode31
Line 31: 
On 2009/03/06 16:40:16, darin wrote:
> nit: kill the extra newline

Done.

http://codereview.chromium.org/28266/diff/1015/1020#newcode37
Line 37: start_callback_(this, &URLRequestNewFtpJob::OnStartCompleted)),
On 2009/03/06 16:40:16, darin wrote:
> nit: we usually indent the argument to ALLOW_THIS_IN_...

Done.

[wtc, 2009-03-10 21:34:31]

Hi,

Darin asked me to review your FTP CLs.  Thank you for
working on the portable FTP implementation.  Below
are my review comments.  Most of them are nits.  The
major issue is the lack of a private section in the
URLRequestNewFtpJob class and the unnecessary methods
you copied from URLRequestHttpJob.

http://codereview.chromium.org/28266/diff/4016/3028
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/28266/diff/4016/3028#newcode92
Line 92: context->ftp_transaction_factory_ =
net::FtpNetworkLayer::CreateFactory();
This line should be indented by 2 instead of 4.

http://codereview.chromium.org/28266/diff/4016/3028#newcode302
Line 302: delete ftp_transaction_factory_;
Nit: move "delete ftp_transaction_factory" before
"delete http_transaction_factory".  Let's delete
the members in the opposite order in which they're
declared in the class.

http://codereview.chromium.org/28266/diff/4016/3029
File net/net.gyp (right):

http://codereview.chromium.org/28266/diff/4016/3029#newcode284
Line 284: 'url_request/url_request_new_ftp_job.cc',
Are these two lines over-indented?

http://codereview.chromium.org/28266/diff/4016/3027
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/28266/diff/4016/3027#newcode21
Line 21: class FtpTransactionFactory;
Nit: we probably should list these forward declarations in
alphabetical order.

http://codereview.chromium.org/28266/diff/4016/3022
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/4016/3022#newcode63
Line 63: // Cheking, whether we are using new or old FTP implementation.
"Cheking" => "Checking".

Remove the comma (,).

http://codereview.chromium.org/28266/diff/4016/3025
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/4016/3025#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Nit: only need 2009 for new files.

http://codereview.chromium.org/28266/diff/4016/3025#newcode14
Line 14: #include "net/base/cookie_monster.h"
Delete this header.  FTP doesn't have cookies.

http://codereview.chromium.org/28266/diff/4016/3025#newcode15
Line 15: #include "net/base/filter.h"
I believe this can be deleted.

http://codereview.chromium.org/28266/diff/4016/3025#newcode19
Line 19: #include "net/base/sdch_manager.h"
Delete this header.

You need to prune the header #include's that you copied
from some other file (url_request_http_job.cc?).

http://codereview.chromium.org/28266/diff/4016/3025#newcode27
Line 27: const std::string& scheme) {
This parameter should be aligned with the opening parenthesis.
See the second formatting style at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/28266/diff/4016/3025#newcode94
Line 94: bool URLRequestNewFtpJob::ReadRawData(net::IOBuffer* buf,
The parameter list of this function should be reformatted
using one of the three styles at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/28266/diff/4016/3026
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/28266/diff/4016/3026#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Nit: New files should have only 2009 in the copyright notice.

http://codereview.chromium.org/28266/diff/4016/3026#newcode20
Line 20: public:
This class should have a private section.  (I don't think it
needs a protected section because it doesn't have subclasses.)

http://codereview.chromium.org/28266/diff/4016/3026#newcode21
Line 21: static URLRequestJob* Factory(URLRequest* request, const std::string&
scheme);
This method isn't being used right now.  I guess this is
for future use?  Perhaps you should have URLRequestFtpJob::Factory() call this
method?

This method should be declared after the constructor and
destructor.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

I know it's declared here in the existing url_request_xxx_job.h headers.  But we
should try
to follow the coding style guide in new files.

http://codereview.chromium.org/28266/diff/4016/3026#newcode25
Line 25: explicit URLRequestNewFtpJob(URLRequest* request);
Nit: declare the constructor before the destructor.

But this constructor should ideally be moved to the protected
or private section.

http://codereview.chromium.org/28266/diff/4016/3026#newcode27
Line 27: // URLRequestJob methods:
Some of these methods don't make sense for FTP.
At least these should be removed:
IsSdchResponse
ContinueDespiteLastError

I can tell that you copied these methods from
url_request_http_job.h. Some of these methods are
HTTP-specific, and should be removed from this class; you
can just use the definitions of those methods in the base
class URLRequestJob.

http://codereview.chromium.org/28266/diff/4016/3026#newcode39
Line 39: // Shadows URLRequestJob's version of this method so we can grab
cookies.
Is this comment right?  FTP doesn't have cookies.
Did you copy this comment from url_request_http_job.h by
mistake?

http://codereview.chromium.org/28266/diff/4016/3026#newcode48
Line 48: std::vector<std::string> response_cookies_;
Delete this member.  FTP doesn't have cookies.

[ibrar, 2009-03-11 08:56:47]

Thanks wtc for your kind review. I have fixed what you pointed out.

http://codereview.chromium.org/28266/diff/4016/3028
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/28266/diff/4016/3028#newcode92
Line 92: context->ftp_transaction_factory_ =
net::FtpNetworkLayer::CreateFactory();
On 2009/03/10 21:34:32, wtc wrote:
> This line should be indented by 2 instead of 4.

Done.

http://codereview.chromium.org/28266/diff/4016/3028#newcode302
Line 302: delete ftp_transaction_factory_;
On 2009/03/10 21:34:32, wtc wrote:
> Nit: move "delete ftp_transaction_factory" before
> "delete http_transaction_factory".  Let's delete
> the members in the opposite order in which they're
> declared in the class.
> 

Done.

http://codereview.chromium.org/28266/diff/4016/3029
File net/net.gyp (right):

http://codereview.chromium.org/28266/diff/4016/3029#newcode284
Line 284: 'url_request/url_request_new_ftp_job.cc',
On 2009/03/10 21:34:32, wtc wrote:
> Are these two lines over-indented?

Done.

http://codereview.chromium.org/28266/diff/4016/3027
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/28266/diff/4016/3027#newcode21
Line 21: class FtpTransactionFactory;
On 2009/03/10 21:34:32, wtc wrote:
> Nit: we probably should list these forward declarations in
> alphabetical order.

Done.

http://codereview.chromium.org/28266/diff/4016/3022
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/4016/3022#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Changed 2009 to 2008 because its not a new file.

http://codereview.chromium.org/28266/diff/4016/3022#newcode63
Line 63: // Cheking, whether we are using new or old FTP implementation.
On 2009/03/10 21:34:32, wtc wrote:
> "Cheking" => "Checking".
> 
> Remove the comma (,).

Done.

http://codereview.chromium.org/28266/diff/4016/3025
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/4016/3025#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/03/10 21:34:32, wtc wrote:
> Nit: only need 2009 for new files.

Actually  net/url_request/url_request_new_ftp_job.h is a new file.

http://codereview.chromium.org/28266/diff/4016/3025#newcode14
Line 14: #include "net/base/cookie_monster.h"
On 2009/03/10 21:34:32, wtc wrote:
> Delete this header.  FTP doesn't have cookies.

Done.

http://codereview.chromium.org/28266/diff/4016/3025#newcode15
Line 15: #include "net/base/filter.h"
On 2009/03/10 21:34:32, wtc wrote:
> I believe this can be deleted.

Done.

http://codereview.chromium.org/28266/diff/4016/3025#newcode19
Line 19: #include "net/base/sdch_manager.h"
On 2009/03/10 21:34:32, wtc wrote:
> Delete this header.
> 
> You need to prune the header #include's that you copied
> from some other file (url_request_http_job.cc?).

Yes, I have copied it from url_request_http_job.cc. Now I can delete almost all
header files because this file does not contain any code just NOTIMPLEMENTED();
macro :)

Done.

http://codereview.chromium.org/28266/diff/4016/3025#newcode27
Line 27: const std::string& scheme) {
On 2009/03/10 21:34:32, wtc wrote:
> This parameter should be aligned with the opening parenthesis.
> See the second formatting style at
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/28266/diff/4016/3025#newcode94
Line 94: bool URLRequestNewFtpJob::ReadRawData(net::IOBuffer* buf,
On 2009/03/10 21:34:32, wtc wrote:
> The parameter list of this function should be reformatted
> using one of the three styles at
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/28266/diff/4016/3026
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/28266/diff/4016/3026#newcode20
Line 20: public:
On 2009/03/10 21:34:32, wtc wrote:
> This class should have a private section.  (I don't think it
> needs a protected section because it doesn't have subclasses.)

Done.

http://codereview.chromium.org/28266/diff/4016/3026#newcode21
Line 21: static URLRequestJob* Factory(URLRequest* request, const std::string&
scheme);
On 2009/03/10 21:34:32, wtc wrote:
> This method isn't being used right now.  I guess this is
> for future use?  Perhaps you should have URLRequestFtpJob::Factory() call this
> method?
> 
> This method should be declared after the constructor and
> destructor.  See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> 
> I know it's declared here in the existing url_request_xxx_job.h headers.  But
we
> should try
> to follow the coding style guide in new files.

Done.

http://codereview.chromium.org/28266/diff/4016/3026#newcode25
Line 25: explicit URLRequestNewFtpJob(URLRequest* request);
On 2009/03/10 21:34:32, wtc wrote:
> Nit: declare the constructor before the destructor.
> 
> But this constructor should ideally be moved to the protected
> or private section.

Done.

http://codereview.chromium.org/28266/diff/4016/3026#newcode27
Line 27: // URLRequestJob methods:
On 2009/03/10 21:34:32, wtc wrote:
> Some of these methods don't make sense for FTP.
> At least these should be removed:
> IsSdchResponse
> ContinueDespiteLastError
> 
> I can tell that you copied these methods from
> url_request_http_job.h. Some of these methods are
> HTTP-specific, and should be removed from this class; you
> can just use the definitions of those methods in the base
> class URLRequestJob.

True, I have copied it from HTTP code, I have removed some now

http://codereview.chromium.org/28266/diff/4016/3026#newcode39
Line 39: // Shadows URLRequestJob's version of this method so we can grab
cookies.
On 2009/03/10 21:34:32, wtc wrote:
> Is this comment right?  FTP doesn't have cookies.
> Did you copy this comment from url_request_http_job.h by
> mistake?

Done.

http://codereview.chromium.org/28266/diff/4016/3026#newcode48
Line 48: std::vector<std::string> response_cookies_;
On 2009/03/10 21:34:32, wtc wrote:
> Delete this member.  FTP doesn't have cookies.

Done.

[wtc, 2009-03-13 02:08:24]

Ibrar,

This looks much better.  Please fix the following nits,
and then we can check this in.

Do you need one of us to check this in for you?

http://codereview.chromium.org/28266/diff/5029/6018
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/28266/diff/5029/6018#newcode86
Line 86: // The kNewFtp switch is Windows specific only because we have 
multiple FTP
Nit: delete the extra space before "multiple".

http://codereview.chromium.org/28266/diff/5029/6012
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/5029/6012#newcode65
Line 65: return new URLRequestNewFtpJob(request);
Please change this to:

  return URLRequestNewFtpJob::Factory(request, scheme);

and move it (including the "if") to the beginning of this
function.

This way we will start to use the URLRequestNewFtpJob::Factory method.

http://codereview.chromium.org/28266/diff/5029/6015
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/5029/6015#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Let me find out whether new files should have 2009 or
2006-2009 in the copyright notice.

http://codereview.chromium.org/28266/diff/5029/6015#newcode7
Line 7: #include "base/base_switches.h"
Please remove all the headers that you don't need now, and
add back the ones you need when you write new code.

http://codereview.chromium.org/28266/diff/5029/6015#newcode23
Line 23: URLRequestJob* URLRequestNewFtpJob::Factory(URLRequest* request,
Nit: move this method after the destructor to match the
declaration order in the header.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/28266/diff/5029/6015#newcode70
Line 70: return 0;
Return -1 instead of 0 on failure.

http://codereview.chromium.org/28266/diff/5029/6015#newcode75
Line 75: return true;
Return false on failure.

http://codereview.chromium.org/28266/diff/5029/6015#newcode79
Line 79: int buf_size, int *bytes_read) {
Nit: in function declaration and definitions, if we
can't list all the parameters on one line,
we're supposed to list the parameters one on each line,
even though we don't follow that guideline strictly in the
net module.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

[ibrar, 2009-03-13 16:50:08]

Thanks wtc,

I don't have commit rights, you have to do that

http://codereview.chromium.org/28266/diff/5029/6018
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/28266/diff/5029/6018#newcode86
Line 86: // The kNewFtp switch is Windows specific only because we have 
multiple FTP
On 2009/03/13 02:08:24, wtc wrote:
> Nit: delete the extra space before "multiple".

Done.

http://codereview.chromium.org/28266/diff/5029/6012
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/5029/6012#newcode65
Line 65: return new URLRequestNewFtpJob(request);
On 2009/03/13 02:08:24, wtc wrote:
> Please change this to:
> 
>   return URLRequestNewFtpJob::Factory(request, scheme);
> 
> and move it (including the "if") to the beginning of this
> function.
> 
> This way we will start to use the URLRequestNewFtpJob::Factory method.

Done.

http://codereview.chromium.org/28266/diff/5029/6015
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/28266/diff/5029/6015#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/03/13 02:08:24, wtc wrote:
> Let me find out whether new files should have 2009 or
> 2006-2009 in the copyright notice.

Ok

http://codereview.chromium.org/28266/diff/5029/6015#newcode7
Line 7: #include "base/base_switches.h"
On 2009/03/13 02:08:24, wtc wrote:
> Please remove all the headers that you don't need now, and
> add back the ones you need when you write new code.

Done.

http://codereview.chromium.org/28266/diff/5029/6015#newcode23
Line 23: URLRequestJob* URLRequestNewFtpJob::Factory(URLRequest* request,
On 2009/03/13 02:08:24, wtc wrote:
> Nit: move this method after the destructor to match the
> declaration order in the header.  See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/28266/diff/5029/6015#newcode70
Line 70: return 0;
On 2009/03/13 02:08:24, wtc wrote:
> Return -1 instead of 0 on failure.

Done.

http://codereview.chromium.org/28266/diff/5029/6015#newcode75
Line 75: return true;
On 2009/03/13 02:08:24, wtc wrote:
> Return false on failure.

Done.

http://codereview.chromium.org/28266/diff/5029/6015#newcode79
Line 79: int buf_size, int *bytes_read) {
On 2009/03/13 02:08:24, wtc wrote:
> Nit: in function declaration and definitions, if we
> can't list all the parameters on one line,
> we're supposed to list the parameters one on each line,
> even though we don't follow that guideline strictly in the
> net module.  See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...
> 

Done.