FTP Transaction code for new Portable FTP(Patch SET-3)

[Replaced by http://codereview.chromium.org/115137 ]

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

Description:

FTP Transaction code for new Portable FTP code

[wtc, 2009-04-07 19:20:24]

Hi Ibrar,

Thanks a lot for the CL.  I didn't review the actual
protocol implementation in ftp_network_transaction.cc
and url_request_new_ftp_job.cc in this pass, so most of
the comments below are on style issues.

I will review the "meat" of your CL carefully in the next
pass.

http://codereview.chromium.org/39130/diff/2001/3003
File net/ftp/ftp_network_layer.h (right):

http://codereview.chromium.org/39130/diff/2001/3003#newcode10
Line 10: #include "net/ftp/ftp_auth_cache.h"
Nit: list this header in alphabetical order.

http://codereview.chromium.org/39130/diff/2001/3007
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/39130/diff/2001/3007#newcode7
Line 7: #include "base/string_util.h"
Nit: list the headers in alphabetical order.  (Three of them
are in the wrong places.)

http://codereview.chromium.org/39130/diff/2001/3007#newcode25
Line 25: :  ALLOW_THIS_IN_INITIALIZER_LIST(io_callback_(this,
Nit: the colon that starts the initializer list should be
indented by 4:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Initializer_Lists

http://codereview.chromium.org/39130/diff/2001/3007#newcode38
Line 38: last_error_(0) {
Nit: use OK instead of 0.

http://codereview.chromium.org/39130/diff/2001/3007#newcode39
Line 39: read_ctrl_buf_size_ = kCtrlBufLen;
Nit: indent by 2.

http://codereview.chromium.org/39130/diff/2001/3007#newcode40
Line 40: read_ctrl_buf_ = new net::IOBuffer(kCtrlBufLen);
Nit: remove the net:: prefix throughout this file.

http://codereview.chromium.org/39130/diff/2001/3007#newcode53
Line 53: UpdateConnectionTypeHistograms(CONNECTION_ANY);
Remove this.

http://codereview.chromium.org/39130/diff/2001/3007#newcode79
Line 79: void FtpNetworkTransaction::LogTransactionMetrics() const {
Remove this.

http://codereview.chromium.org/39130/diff/2001/3007#newcode379
Line 379: response_message_buf_->data() + i + 2,
Indentation is wrong: in this case (a function call), the
parameters should be aligned with the opening parenthesis
on the previous line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

http://codereview.chromium.org/39130/diff/2001/3007#newcode394
Line 394: if (request_->url.has_username())
Nit: in the net module, we always use braces when there is
an "else", even if the bodies have only one statement.

Please make this style change throughout this file.

http://codereview.chromium.org/39130/diff/2001/3008
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/39130/diff/2001/3008#newcode32
Line 32: virtual int Stop(int err);
Nit: spell the parameter fully as "error", which isn't that
long:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

http://codereview.chromium.org/39130/diff/2001/3008#newcode37
Line 37: void LogTransactionMetrics() const;
We don't need the LogTransactionMetrics method.

If we do need it, don't list it in the
"FtpTransaction methods:" section because it is not
a method of FtpTransaction.

http://codereview.chromium.org/39130/diff/2001/3008#newcode38
Line 38: virtual int Read(net::IOBuffer* buf, int buf_len,
Nit: remove the net:: prefix for IOBuffer because this is in
the net namespace.

List one parameter per line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/39130/diff/2001/3008#newcode71
Line 71: ERROR_CLASS_INITIATED = 1,     // The requested action was initiated.
Nit: you may want to align the first comment with the other
comments.

http://codereview.chromium.org/39130/diff/2001/3008#newcode82
Line 82: ErrorClass GetErrorClass(int rcode) {
Nit: pick a better name for 'rcode'.  'response_code'?

http://codereview.chromium.org/39130/diff/2001/3008#newcode143
Line 143: scoped_refptr<net::IOBuffer> read_ctrl_buf_;
Nit: remove the net:: prefix throughout this file.

http://codereview.chromium.org/39130/diff/2001/3008#newcode146
Line 146: net::IOBuffer *response_message_buf_;
Nit: put * next to the type rather than the variable name
here and below (for read_data_buf_).

http://codereview.chromium.org/39130/diff/2001/3002
File net/ftp/ftp_request_info.h (right):

http://codereview.chromium.org/39130/diff/2001/3002#newcode8
Line 8: #include "base/ref_counted.h"
Remove this header because you're not using scoped_refptr.

http://codereview.chromium.org/39130/diff/2001/3002#newcode10
Line 10: #include "net/base/upload_data.h"
Remove this header because you're not using UploadData.

http://codereview.chromium.org/39130/diff/2001/3002#newcode20
Line 20: GURL referrer;
Does FTP have the notion of a referrer?

http://codereview.chromium.org/39130/diff/2001/3002#newcode23
Line 23: int load_flags;
Are we using the load_flags member?

http://codereview.chromium.org/39130/diff/2001/3005
File net/ftp/ftp_response_info.h (right):

http://codereview.chromium.org/39130/diff/2001/3005#newcode31
Line 31: FtpResponseInfo() : content_length(-1), is_directory_listing(false) {
Constructor should be listed before data members:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/39130/diff/2001/3004
File net/ftp/ftp_transaction.h (right):

http://codereview.chromium.org/39130/diff/2001/3004#newcode9
Line 9: #include "net/ftp/ftp_request_info.h"
A forward declaration of FtpRequestInfo should suffice,
so you shouldn't need to include ftp_request_info.h.

http://codereview.chromium.org/39130/diff/2001/3004#newcode12
Line 12: #include "net/base/io_buffer.h"
Nit: list io_buffer.h before load_states.h (in alphabetical
order).

A forward declaration of IOBuffer should suffice.

http://codereview.chromium.org/39130/diff/2001/3004#newcode17
Line 17: class FtpResponseInfo;
It's not necessary to include ftp_response_info.h and
forward-declare FtpResponseInfo at the same time.
Between the two, a forward decalration is preferred (if it
works):
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...

http://codereview.chromium.org/39130/diff/2001/3004#newcode47
Line 47: virtual int RestartIgnoringLastError(CompletionCallback* callback) = 0;
I think the RestartIgnoringLastError method doesn't make
sense for FTP.  In HttpTransaction, the
RestartIgnoringLastError method is used to restart a
transaction ignoring an SSL certificate error.  FTP
doesn't run over SSL.

http://codereview.chromium.org/39130/diff/2001/3004#newcode62
Line 62: virtual int Read(net::IOBuffer* buf, int buf_len,
Nit: this is in the net namespace, so it's not necessary
to use the net:: prefix for IOBuffer.

Nit: the style guide suggests listing one parameter per
line in function declaration and definition, if all
parameters can't fit in one line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/39130/diff/2001/3011
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/39130/diff/2001/3011#newcode11
Line 11: #include "net/ftp/ftp_directory_parser.h"
Nit: list the headers in alphabetical order.

http://codereview.chromium.org/39130/diff/2001/3011#newcode48
Line 48: request_info_.url = request_->url();
Perhaps we don't need a FtpRequestInfo class because the
only member of it we're using is 'url'.  We're not using
the 'referrer' or 'load_flags' members.

http://codereview.chromium.org/39130/diff/2001/3011#newcode91
Line 91: // If this isn't top level directory (i.e. the path isn't "/",) add a
link to
This line and line 94 below may be longer than 80 characters.

http://codereview.chromium.org/39130/diff/2001/3011#newcode119
Line 119: true, rv, base::Time()));
See the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
on how to format the arguments for function calls.

The case net::FTP_TYPE_FILE below needs the same change.

http://codereview.chromium.org/39130/diff/2001/3011#newcode201
Line 201: } else {
Nit: don't use 'else' when if's body ends with a return
statement.

http://codereview.chromium.org/39130/diff/2001/3009
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/39130/diff/2001/3009#newcode19
Line 19: class FtpResponseInfo;
You include the ftp_response_info.h and ftp_transaction.h
above, so it isn't necessary to have forward declarations
for FtpResponseInfo and FtpTransaction here.

If the forward declarations are sufficient, we prefer them
over including the headers:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...

http://codereview.chromium.org/39130/diff/2001/3009#newcode39
Line 39: virtual void GetResponseInfo(net::FtpResponseInfo* info);
The GetResponseInfo method is problematic because its
parameter list is different from the GetResponseInfo method
in the base class URLRequestJob.

Does FTP really need its own ResponseInfo class?  Can
we make HttpResponseInfo work for FTP?

If we have to use FtpResponseInfo, we probably should
rename this method and not list it under
"URLRequestJob methods:", because the style guide seems
to discourage function overloading:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

http://codereview.chromium.org/39130/diff/2001/3009#newcode55
Line 55: std::vector<std::string> response_cookies_;
Does FTP have cookies?!

[ibrar, 2009-04-17 06:10:55]

Hi wtc,

Thanks for your review. I have done almost all the things you have mentioned. I
am still looking at the style problem my self. 

I think now you can review actual protocol code.

http://codereview.chromium.org/39130/diff/2001/3003
File net/ftp/ftp_network_layer.h (right):

http://codereview.chromium.org/39130/diff/2001/3003#newcode10
Line 10: #include "net/ftp/ftp_auth_cache.h"
On 2009/04/07 19:20:24, wtc wrote:
> Nit: list this header in alphabetical order.

Done.

http://codereview.chromium.org/39130/diff/2001/3007
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/39130/diff/2001/3007#newcode7
Line 7: #include "base/string_util.h"
On 2009/04/07 19:20:24, wtc wrote:
> Nit: list the headers in alphabetical order.  (Three of them
> are in the wrong places.)

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode25
Line 25: :  ALLOW_THIS_IN_INITIALIZER_LIST(io_callback_(this,
On 2009/04/07 19:20:24, wtc wrote:
> Nit: the colon that starts the initializer list should be
> indented by 4:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Initializer_Lists

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode38
Line 38: last_error_(0) {
On 2009/04/07 19:20:24, wtc wrote:
> Nit: use OK instead of 0.

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode39
Line 39: read_ctrl_buf_size_ = kCtrlBufLen;
On 2009/04/07 19:20:24, wtc wrote:
> Nit: indent by 2.

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode40
Line 40: read_ctrl_buf_ = new net::IOBuffer(kCtrlBufLen);
On 2009/04/07 19:20:24, wtc wrote:
> Nit: remove the net:: prefix throughout this file.

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode53
Line 53: UpdateConnectionTypeHistograms(CONNECTION_ANY);
On 2009/04/07 19:20:24, wtc wrote:
> Remove this.

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode79
Line 79: void FtpNetworkTransaction::LogTransactionMetrics() const {
On 2009/04/07 19:20:24, wtc wrote:
> Remove this.

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode379
Line 379: response_message_buf_->data() + i + 2,
On 2009/04/07 19:20:24, wtc wrote:
> Indentation is wrong: in this case (a function call), the
> parameters should be aligned with the opening parenthesis
> on the previous line:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

Done.

http://codereview.chromium.org/39130/diff/2001/3007#newcode394
Line 394: if (request_->url.has_username())
On 2009/04/07 19:20:24, wtc wrote:
> Nit: in the net module, we always use braces when there is
> an "else", even if the bodies have only one statement.
> 
> Please make this style change throughout this file.

Done.

http://codereview.chromium.org/39130/diff/2001/3008
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/39130/diff/2001/3008#newcode37
Line 37: void LogTransactionMetrics() const;
On 2009/04/07 19:20:24, wtc wrote:
> We don't need the LogTransactionMetrics method.
> 
> If we do need it, don't list it in the
> "FtpTransaction methods:" section because it is not
> a method of FtpTransaction.

Done.

http://codereview.chromium.org/39130/diff/2001/3008#newcode38
Line 38: virtual int Read(net::IOBuffer* buf, int buf_len,
On 2009/04/07 19:20:24, wtc wrote:
> Nit: remove the net:: prefix for IOBuffer because this is in
> the net namespace.
> 
> List one parameter per line:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/39130/diff/2001/3008#newcode71
Line 71: ERROR_CLASS_INITIATED = 1,     // The requested action was initiated.
On 2009/04/07 19:20:24, wtc wrote:
> Nit: you may want to align the first comment with the other
> comments.

Done.

http://codereview.chromium.org/39130/diff/2001/3008#newcode82
Line 82: ErrorClass GetErrorClass(int rcode) {
On 2009/04/07 19:20:24, wtc wrote:
> Nit: pick a better name for 'rcode'.  'response_code'?

Done.

http://codereview.chromium.org/39130/diff/2001/3008#newcode143
Line 143: scoped_refptr<net::IOBuffer> read_ctrl_buf_;
On 2009/04/07 19:20:24, wtc wrote:
> Nit: remove the net:: prefix throughout this file.

Done.

http://codereview.chromium.org/39130/diff/2001/3008#newcode146
Line 146: net::IOBuffer *response_message_buf_;
On 2009/04/07 19:20:24, wtc wrote:
> Nit: put * next to the type rather than the variable name
> here and below (for read_data_buf_).

Done.

http://codereview.chromium.org/39130/diff/2001/3002
File net/ftp/ftp_request_info.h (right):

http://codereview.chromium.org/39130/diff/2001/3002#newcode8
Line 8: #include "base/ref_counted.h"
On 2009/04/07 19:20:24, wtc wrote:
> Remove this header because you're not using scoped_refptr.

Done.

http://codereview.chromium.org/39130/diff/2001/3002#newcode10
Line 10: #include "net/base/upload_data.h"
On 2009/04/07 19:20:24, wtc wrote:
> Remove this header because you're not using UploadData.

Done.

http://codereview.chromium.org/39130/diff/2001/3002#newcode20
Line 20: GURL referrer;
On 2009/04/07 19:20:24, wtc wrote:
> Does FTP have the notion of a referrer?

Done.

http://codereview.chromium.org/39130/diff/2001/3002#newcode23
Line 23: int load_flags;
On 2009/04/07 19:20:24, wtc wrote:
> Are we using the load_flags member?

Done.

http://codereview.chromium.org/39130/diff/2001/3005
File net/ftp/ftp_response_info.h (right):

http://codereview.chromium.org/39130/diff/2001/3005#newcode31
Line 31: FtpResponseInfo() : content_length(-1), is_directory_listing(false) {
On 2009/04/07 19:20:24, wtc wrote:
> Constructor should be listed before data members:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/39130/diff/2001/3004
File net/ftp/ftp_transaction.h (right):

http://codereview.chromium.org/39130/diff/2001/3004#newcode9
Line 9: #include "net/ftp/ftp_request_info.h"
On 2009/04/07 19:20:24, wtc wrote:
> A forward declaration of FtpRequestInfo should suffice,
> so you shouldn't need to include ftp_request_info.h.

Done.

http://codereview.chromium.org/39130/diff/2001/3004#newcode12
Line 12: #include "net/base/io_buffer.h"
On 2009/04/07 19:20:24, wtc wrote:
> Nit: list io_buffer.h before load_states.h (in alphabetical
> order).
> 
> A forward declaration of IOBuffer should suffice.

Done.

http://codereview.chromium.org/39130/diff/2001/3004#newcode17
Line 17: class FtpResponseInfo;
On 2009/04/07 19:20:24, wtc wrote:
> It's not necessary to include ftp_response_info.h and
> forward-declare FtpResponseInfo at the same time.
> Between the two, a forward decalration is preferred (if it
> works):
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...

Done.

http://codereview.chromium.org/39130/diff/2001/3004#newcode47
Line 47: virtual int RestartIgnoringLastError(CompletionCallback* callback) = 0;
On 2009/04/07 19:20:24, wtc wrote:
> I think the RestartIgnoringLastError method doesn't make
> sense for FTP.  In HttpTransaction, the
> RestartIgnoringLastError method is used to restart a
> transaction ignoring an SSL certificate error.  FTP
> doesn't run over SSL.

Done.

http://codereview.chromium.org/39130/diff/2001/3004#newcode62
Line 62: virtual int Read(net::IOBuffer* buf, int buf_len,
On 2009/04/07 19:20:24, wtc wrote:
> Nit: this is in the net namespace, so it's not necessary
> to use the net:: prefix for IOBuffer.
> 
> Nit: the style guide suggests listing one parameter per
> line in function declaration and definition, if all
> parameters can't fit in one line:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/39130/diff/2001/3011
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/39130/diff/2001/3011#newcode48
Line 48: request_info_.url = request_->url();
On 2009/04/07 19:20:24, wtc wrote:
> Perhaps we don't need a FtpRequestInfo class because the
> only member of it we're using is 'url'.  We're not using
> the 'referrer' or 'load_flags' members.


I think we should keep request_info to be consistent with http code. If you
insists I will remove it.

http://codereview.chromium.org/39130/diff/2001/3011#newcode91
Line 91: // If this isn't top level directory (i.e. the path isn't "/",) add a
link to
On 2009/04/07 19:20:24, wtc wrote:
> This line and line 94 below may be longer than 80 characters.

Done.

http://codereview.chromium.org/39130/diff/2001/3011#newcode119
Line 119: true, rv, base::Time()));
On 2009/04/07 19:20:24, wtc wrote:
> See the style guide:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> on how to format the arguments for function calls.
> 
> The case net::FTP_TYPE_FILE below needs the same change.

Done.

http://codereview.chromium.org/39130/diff/2001/3011#newcode201
Line 201: } else {
On 2009/04/07 19:20:24, wtc wrote:
> Nit: don't use 'else' when if's body ends with a return
> statement.

I think you are missing something here. I am not returning any case

if (rv == net::ERR_IO_PENDING) check is there.

http://codereview.chromium.org/39130/diff/2001/3009
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/39130/diff/2001/3009#newcode19
Line 19: class FtpResponseInfo;
On 2009/04/07 19:20:24, wtc wrote:
> You include the ftp_response_info.h and ftp_transaction.h
> above, so it isn't necessary to have forward declarations
> for FtpResponseInfo and FtpTransaction here.
> 
> If the forward declarations are sufficient, we prefer them
> over including the headers:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...

Done.

http://codereview.chromium.org/39130/diff/2001/3009#newcode39
Line 39: virtual void GetResponseInfo(net::FtpResponseInfo* info);
On 2009/04/07 19:20:24, wtc wrote:
> The GetResponseInfo method is problematic because its
> parameter list is different from the GetResponseInfo method
> in the base class URLRequestJob.
> 
> Does FTP really need its own ResponseInfo class?  Can
> we make HttpResponseInfo work for FTP?
> 
> If we have to use FtpResponseInfo, we probably should
> rename this method and not list it under
> "URLRequestJob methods:", because the style guide seems
> to discourage function overloading:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

Done.


For time being I have just change the name of function

http://codereview.chromium.org/39130/diff/2001/3009#newcode55
Line 55: std::vector<std::string> response_cookies_;
On 2009/04/07 19:20:24, wtc wrote:
> Does FTP have cookies?!

Done.

[wtc, 2009-04-28 18:04:25]

http://codereview.chromium.org/39130/diff/2001/3011
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/39130/diff/2001/3011#newcode201
Line 201: } else {
On 2009/04/17 06:10:55, ibrar wrote:
>
> I think you are missing something here. I am not returning any case
> 
> if (rv == net::ERR_IO_PENDING) check is there.

Ah, you're right.  I was confused by the incorrect
indentation.  The return statement at line 200 needs
to be indented by 2.

[wtc, 2009-04-28 21:55:07]

Sorry about the late review.  Thanks for addressing my
previous review comments.  I have some comments below on
the current version of the CL.  Many of them are just nits,
but some of them are worth addressing.

http://codereview.chromium.org/39130/diff/12008/12011
File net/ftp/ftp_network_layer.h (right):

http://codereview.chromium.org/39130/diff/12008/12011#newcode9
Line 9: #include "net/ftp/ftp_auth_cache.h"
Nit: remove "net/ftp/ftp_auth_cache.h".  I believe the
forward declaration of class FtpAuthCache below is enough.

http://codereview.chromium.org/39130/diff/12008/12011#newcode26
Line 26: virtual FtpAuthCache* GetAuthCache();
In HTTP, the auth cache is part of the HttpNetworkSession
class.  The GetCache method of HttpTransactionFactory
returns the HTTP cache as opposed to the HTTP auth cache.

Does the FtpTransactionFactory interface really need the
GetAuthCache method?

http://codereview.chromium.org/39130/diff/12008/12015
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/39130/diff/12008/12015#newcode62
Line 62: int FtpNetworkTransaction::Stop(int err) {
Nit: add a blank line above this line.

Rename the parameter 'error'.

http://codereview.chromium.org/39130/diff/12008/12015#newcode85
Line 85: return OK;  // Data socket closed, no more data left.
For the Read method, the return value is a byte count, so
it is better to use 0 instead of OK.

http://codereview.chromium.org/39130/diff/12008/12015#newcode139
Line 139: rv = DoCtrlReadUSER(response_code);
Nit: our convention is to use a DoFoo function for the
state STATE_FOO.

So the DoCtrlReadFOO functions here should probaboy be
renamed because they don't correspond to states.  Or
do they correspond to states conceptually?

Perhaps we can rename them ProcessPASSResponse,
ProcessACCTResponse, ProcessSYSTResponse, etc.  (Or put the
command names at the end, like ProcessResponsePASS,
ProcessResponseACCT, ProcessResponseSYST, etc.)

http://codereview.chromium.org/39130/diff/12008/12015#newcode326
Line 326: DidFinishDnsResolutionWithStatus(ok, request_->url, this);
Since FTP doesn't have the notion of a referrer, we should
pass an empty string 'std::string()' instead of request_->url.

http://codereview.chromium.org/39130/diff/12008/12015#newcode442
Line 442: if (response_code == 503) {
The indentation of this if-else statement needs to be fixed.

http://codereview.chromium.org/39130/diff/12008/12015#newcode573
Line 573: if (sscanf_s(ptr,
It's not necessary to list only one argument per line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

http://codereview.chromium.org/39130/diff/12008/12016
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/39130/diff/12008/12016#newcode12
Line 12: #include "net/ftp/ftp_response_info.h"
We don't need to include "net/ftp/ftp_response_info.h" and
forward-declare class FtpResponseInfo at the same time.
Only one of them is necessary.

http://codereview.chromium.org/39130/diff/12008/12016#newcode24
Line 24: FtpNetworkTransaction(
Nit: the parameter lists for the constructor and the Start,
RestartWithAuth, and Read methods should be formatted
according to the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/39130/diff/12008/12016#newcode26
Line 26: ~FtpNetworkTransaction();
The destructor should be declared as 'virtual'.

http://codereview.chromium.org/39130/diff/12008/12016#newcode29
Line 29: virtual void Destroy();
We should replace the Destroy method by the destructor.

http://codereview.chromium.org/39130/diff/12008/12010
File net/ftp/ftp_request_info.h (right):

http://codereview.chromium.org/39130/diff/12008/12010#newcode12
Line 12: class FtpRequestInfo {
It's fine to leave the FtpRequestInfo class for future
expansion.

http://codereview.chromium.org/39130/diff/12008/12013
File net/ftp/ftp_response_info.h (right):

http://codereview.chromium.org/39130/diff/12008/12013#newcode21
Line 21: int64 content_length;
I believe you're not using the 'content_length' member.
Can we remove it?

http://codereview.chromium.org/39130/diff/12008/12012
File net/ftp/ftp_transaction.h (right):

http://codereview.chromium.org/39130/diff/12008/12012#newcode9
Line 9: #include "net/base/io_buffer.h"
Nit: it should be sufficient to just use a forward declaration
of class IOBuffer.

http://codereview.chromium.org/39130/diff/12008/12012#newcode15
Line 15: class FtpRequestInfo;
Why don't we need a forward declaration of class FtpResponseInfo?

http://codereview.chromium.org/39130/diff/12008/12012#newcode21
Line 21: virtual void Destroy() = 0;
We should add a virtual destructor to this class.

Why does this class have a Destroy method?  The
HttpTransaction class doesn't have a Destroy method.
I seem to remember HttpTransaction used to have a Destroy
method, and we changed to just use the destructor.  Perhaps
you created this file based on an older version of
http_transaction.h?

http://codereview.chromium.org/39130/diff/12008/12012#newcode69
Line 69: // zero will be returned.  This does not include the request headers.
Nit: if FTP doesn't have request headers, the comment
"This does not include the request headers." should be
deleted.

http://codereview.chromium.org/39130/diff/12008/12018
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/39130/diff/12008/12018#newcode66
Line 66: int URLRequestNewFtpJob::GetResponseCode() const {
We should remove this method.  URLRequestFtpJob doesn't
define this method.

http://codereview.chromium.org/39130/diff/12008/12018#newcode86
Line 86: // If this isn't top level directory (i.e. the path isn't "/",)
This line and the next line should be aligned with the if
statement below.

http://codereview.chromium.org/39130/diff/12008/12017
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/39130/diff/12008/12017#newcode14
Line 14: #include "net/ftp/ftp_response_info.h"
Nit: a forward declaration of class FtpResponseInfo should
be enough.

http://codereview.chromium.org/39130/diff/12008/12017#newcode36
Line 36: virtual void GetFtpResponseInfo(net::FtpResponseInfo* info);
GetFtpResponseInfo should not be listed under
"URLRequestJob methods" because it is not a
URLRequestJob method.

But I think it's better to just add the
'is_directory_listing' member to HttpResponseInfo (perhaps
rename it 'is_ftp_directory_listing') and use
HttpResponseInfo.  The reason is that URLRequestFileJob
is also using HttpResponseInfo this way.

Alternatively, just remove the GetFtpResponseInfo method
from this class.  It's not being used anyway.

http://codereview.chromium.org/39130/diff/12008/12017#newcode37
Line 37: virtual int GetResponseCode() const;
We should remove this method.  URLRequestFtpJob doesn't
define this method.

[ibrar, 2009-05-02 10:57:28]

WTC,

Thanks for your kind review.

http://codereview.chromium.org/39130/diff/12008/12011
File net/ftp/ftp_network_layer.h (right):

http://codereview.chromium.org/39130/diff/12008/12011#newcode9
Line 9: #include "net/ftp/ftp_auth_cache.h"
On 2009/04/28 21:55:08, wtc wrote:
> Nit: remove "net/ftp/ftp_auth_cache.h".  I believe the
> forward declaration of class FtpAuthCache below is enough.

Done.

http://codereview.chromium.org/39130/diff/12008/12011#newcode26
Line 26: virtual FtpAuthCache* GetAuthCache();
On 2009/04/28 21:55:08, wtc wrote:
> In HTTP, the auth cache is part of the HttpNetworkSession
> class.  The GetCache method of HttpTransactionFactory
> returns the HTTP cache as opposed to the HTTP auth cache.
> 
> Does the FtpTransactionFactory interface really need the
> GetAuthCache method?

Done.

http://codereview.chromium.org/39130/diff/12008/12016
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/39130/diff/12008/12016#newcode12
Line 12: #include "net/ftp/ftp_response_info.h"
On 2009/04/28 21:55:08, wtc wrote:
> We don't need to include "net/ftp/ftp_response_info.h" and
> forward-declare class FtpResponseInfo at the same time.
> Only one of them is necessary.

Done.

http://codereview.chromium.org/39130/diff/12008/12016#newcode24
Line 24: FtpNetworkTransaction(
On 2009/04/28 21:55:08, wtc wrote:
> Nit: the parameter lists for the constructor and the Start,
> RestartWithAuth, and Read methods should be formatted
> according to the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/39130/diff/12008/12016#newcode26
Line 26: ~FtpNetworkTransaction();
On 2009/04/28 21:55:08, wtc wrote:
> The destructor should be declared as 'virtual'.

Done.

http://codereview.chromium.org/39130/diff/12008/12016#newcode29
Line 29: virtual void Destroy();
On 2009/04/28 21:55:08, wtc wrote:
> We should replace the Destroy method by the destructor.

Done.

http://codereview.chromium.org/39130/diff/12008/12013
File net/ftp/ftp_response_info.h (right):

http://codereview.chromium.org/39130/diff/12008/12013#newcode21
Line 21: int64 content_length;
On 2009/04/28 21:55:08, wtc wrote:
> I believe you're not using the 'content_length' member.
> Can we remove it?

Done.

http://codereview.chromium.org/39130/diff/12008/12012
File net/ftp/ftp_transaction.h (right):

http://codereview.chromium.org/39130/diff/12008/12012#newcode9
Line 9: #include "net/base/io_buffer.h"
On 2009/04/28 21:55:08, wtc wrote:
> Nit: it should be sufficient to just use a forward declaration
> of class IOBuffer.

Cannot remove this header!. Otherwise I have to look at the arrangement of
header files (in my code) again. Forward declaration does not solve the problem.
I will look at it again.

http://codereview.chromium.org/39130/diff/12008/12012#newcode15
Line 15: class FtpRequestInfo;
On 2009/04/28 21:55:08, wtc wrote:
> Why don't we need a forward declaration of class FtpResponseInfo?

Hmm! We are using this in this class!

http://codereview.chromium.org/39130/diff/12008/12012#newcode21
Line 21: virtual void Destroy() = 0;
On 2009/04/28 21:55:08, wtc wrote:
> We should add a virtual destructor to this class.
> 
> Why does this class have a Destroy method?  The
> HttpTransaction class doesn't have a Destroy method.
> I seem to remember HttpTransaction used to have a Destroy
> method, and we changed to just use the destructor.  Perhaps
> you created this file based on an older version of
> http_transaction.h?

Yes true :)

http://codereview.chromium.org/39130/diff/12008/12012#newcode69
Line 69: // zero will be returned.  This does not include the request headers.
On 2009/04/28 21:55:08, wtc wrote:
> Nit: if FTP doesn't have request headers, the comment
> "This does not include the request headers." should be
> deleted.

Done.

http://codereview.chromium.org/39130/diff/12008/12018
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/39130/diff/12008/12018#newcode66
Line 66: int URLRequestNewFtpJob::GetResponseCode() const {
On 2009/04/28 21:55:08, wtc wrote:
> We should remove this method.  URLRequestFtpJob doesn't
> define this method.

Done.

http://codereview.chromium.org/39130/diff/12008/12018#newcode86
Line 86: // If this isn't top level directory (i.e. the path isn't "/",)
On 2009/04/28 21:55:08, wtc wrote:
> This line and the next line should be aligned with the if
> statement below.

Done.

http://codereview.chromium.org/39130/diff/12008/12017
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/39130/diff/12008/12017#newcode14
Line 14: #include "net/ftp/ftp_response_info.h"
On 2009/04/28 21:55:08, wtc wrote:
> Nit: a forward declaration of class FtpResponseInfo should
> be enough.

I think not!. I have removed forward declaration BTW.

http://codereview.chromium.org/39130/diff/12008/12017#newcode36
Line 36: virtual void GetFtpResponseInfo(net::FtpResponseInfo* info);
On 2009/04/28 21:55:08, wtc wrote:
> GetFtpResponseInfo should not be listed under
> "URLRequestJob methods" because it is not a
> URLRequestJob method.
> 
> But I think it's better to just add the
> 'is_directory_listing' member to HttpResponseInfo (perhaps
> rename it 'is_ftp_directory_listing') and use
> HttpResponseInfo.  The reason is that URLRequestFileJob
> is also using HttpResponseInfo this way.
> 
> Alternatively, just remove the GetFtpResponseInfo method
> from this class.  It's not being used anyway.

Removed!

http://codereview.chromium.org/39130/diff/12008/12017#newcode37
Line 37: virtual int GetResponseCode() const;
On 2009/04/28 21:55:08, wtc wrote:
> We should remove this method.  URLRequestFtpJob doesn't
> define this method.

Done.

[wtc, 2009-05-07 00:12:03]

http://codereview.chromium.org/39130/diff/12008/12017
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/39130/diff/12008/12017#newcode14
Line 14: #include "net/ftp/ftp_response_info.h"
On 2009/05/02 10:57:28, ibrar wrote:
> 
> I think not!. I have removed forward declaration BTW.

Is it because you misspelled FtpResponseInfo?

In any case, please remove the forward declaration of FTPResponseInfo (note the
all capital "FTP").

I believe we can also remove the "net/ftp/ftp_response_info.h"
and the response_info_ member.

http://codereview.chromium.org/39130/diff/15016/15023
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/39130/diff/15016/15023#newcode322
Line 322: DidFinishDnsResolutionWithStatus(ok, static_cast<GURL>(std::string()),
this);
Sorry, my suggestion of passing an empty string is wrong.
It should be an empty GURL.

http://codereview.chromium.org/39130/diff/15016/15026
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/39130/diff/15016/15026#newcode66
Line 66: response_info_ = transaction_->GetResponseInfo();
We can just use a local variable 'response_info' in this
method.  We don't need the 'response_info_' member.

[wtc, 2009-05-07 00:14:55]

http://codereview.chromium.org/39130/diff/15016/15023
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/39130/diff/15016/15023#newcode322
Line 322: DidFinishDnsResolutionWithStatus(ok, static_cast<GURL>(std::string()),
this);
Just pass GURL() here.

[ibrar, 2009-05-07 10:51:31]

http://codereview.chromium.org/39130/diff/12008/12017
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/39130/diff/12008/12017#newcode14
Line 14: #include "net/ftp/ftp_response_info.h"
On 2009/05/07 00:12:03, wtc wrote:
> On 2009/05/02 10:57:28, ibrar wrote:
> > 
> > I think not!. I have removed forward declaration BTW.
> 
> Is it because you misspelled FtpResponseInfo?
> 
> In any case, please remove the forward declaration of FTPResponseInfo (note
the
> all capital "FTP").
> 
> I believe we can also remove the "net/ftp/ftp_response_info.h"
> and the response_info_ member.
> 
Forward declaration and net/ftp/ftp_response_info.h removed but cannot remove
response_info_ at this time.

http://codereview.chromium.org/39130/diff/15016/15023
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/39130/diff/15016/15023#newcode322
Line 322: DidFinishDnsResolutionWithStatus(ok, static_cast<GURL>(std::string()),
this);
On 2009/05/07 00:14:55, wtc wrote:
> Just pass GURL() here.

Done.

http://codereview.chromium.org/39130/diff/15016/15026
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/39130/diff/15016/15026#newcode66
Line 66: response_info_ = transaction_->GetResponseInfo();
On 2009/05/07 00:12:03, wtc wrote:
> We can just use a local variable 'response_info' in this
> method.  We don't need the 'response_info_' member.

Actually I am using response_info_ == NULL as flag (is it first time call of
ReadRawData or subsequent calls of that function). If we want to make it local I
have to rethink the logic.