Clean up FTPNetworkTransaction to match the latest trace

Clean up FTPNetworkTransaction to match the latest trace

This CL is an follow-up CL to crrev.com/861193007. This CL makes sure state
transitions and comments match the latest trace.

BUG=452657
Committed: https://crrev.com/abc41a5466e2cd7ad072cc9f2a31691b9de3070d
Cr-Commit-Position: refs/heads/master@{#314882}

[xunjieli, 2015-01-30 21:50:44]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org

[xunjieli, 2015-01-30 21:50:44]

PTAL. Thanks!

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.cc (left):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:242: int
FtpNetworkTransaction::RestartIgnoringLastError(
This is never used anywhere. Is it okay to remove it?

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:1167: std::string command("LIST -l");
I wonder if we should remove "-l". This is not a part of RFC. This is confusing
some servers. For instance, ftp://ftp.vsm.com.au/ doesn't like "-l", and closes
data connection if we do "LIST -l". Some googling brought up this
http://stackoverflow.com/questions/21065113/ftp-list-why-does-chrome-use-the-...

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction_unittest.cc:921:
ExecuteTransaction(&ctrl_socket, "ftp://host/;type=d", OK);
Added a "/" here so that ";type=d" will not be interpreted as a part of the
host. It seems there should be a slash according to RFC 1738 section 3.2.2.

type=i and type=a are tested in DownloadTransactionWithTypecodeI and
DownloadTransactionWithTypecodeA.

[davidben, 2015-02-04 18:51:18]

lgtm. Thanks for picking this up!

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.cc (left):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:242: int
FtpNetworkTransaction::RestartIgnoringLastError(
On 2015/01/30 21:50:44, xunjieli wrote:
> This is never used anywhere. Is it okay to remove it?

Sounds good. :-)

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:1094: }
Do you know what this code does? I'm confused what it's for. As far as I can
tell, this is the only place where RESOURCE_TYPE_UNKNOWN vs
RESOURCE_TYPE_DIRECTORY even does anything.

(This CL also doesn't change this block any, so if you're not sure either and
can't find the answer quickly, let's just leave it for now.)

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:1167: std::string command("LIST -l");
On 2015/01/30 21:50:44, xunjieli wrote:
> I wonder if we should remove "-l". This is not a part of RFC. This is
confusing
> some servers. For instance, ftp://ftp.vsm.com.au/ doesn't like "-l", and
closes
> data connection if we do "LIST -l". Some googling brought up this
>
http://stackoverflow.com/questions/21065113/ftp-list-why-does-chrome-use-the-...

Hrm. Apparently it was added in https://chromiumcodereview.appspot.com/11364204
to deal with OTHER server quirks.

Yaay. :-/ I guess we can leave it alone until someone notices? I don't think
people actually use ftp anymore anyway.

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.h (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.h:36: virtual int Stop(int error);
While you're here, is there any reason this is virtual? It doesn't look like
it...

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction_unittest.cc:921:
ExecuteTransaction(&ctrl_socket, "ftp://host/;type=d", OK);
On 2015/01/30 21:50:44, xunjieli wrote:
> Added a "/" here so that ";type=d" will not be interpreted as a part of the
> host. It seems there should be a slash according to RFC 1738 section 3.2.2.
> 
> type=i and type=a are tested in DownloadTransactionWithTypecodeI and
> DownloadTransactionWithTypecodeA.

Hah. Did this test just never actually test the type=d case before?

[xunjieli, 2015-02-04 20:07:45]

New patchsets have been uploaded after l-g-t-m from davidben@chromium.org

[xunjieli, 2015-02-04 20:29:29]

I have removed RESOURCE_TYPE_UNKNOWN part. Take another look?

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:1094: }
On 2015/02/04 18:51:18, David Benjamin wrote:
> Do you know what this code does? I'm confused what it's for. As far as I can
> tell, this is the only place where RESOURCE_TYPE_UNKNOWN vs
> RESOURCE_TYPE_DIRECTORY even does anything.
> 
> (This CL also doesn't change this block any, so if you're not sure either and
> can't find the answer quickly, let's just leave it for now.)

The Stop case is not tested in the code. I think it is probably safe to get rid
of RESOURCE_TYPE_UNKNOWN here, since according to RFC 3659, it seems that we
should try the next command regardless of the type when we have a 550. WDYT?

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:1167: std::string command("LIST -l");
On 2015/02/04 18:51:18, David Benjamin wrote:
> On 2015/01/30 21:50:44, xunjieli wrote:
> > I wonder if we should remove "-l". This is not a part of RFC. This is
> confusing
> > some servers. For instance, ftp://ftp.vsm.com.au/ doesn't like "-l", and
> closes
> > data connection if we do "LIST -l". Some googling brought up this
> >
>
http://stackoverflow.com/questions/21065113/ftp-list-why-does-chrome-use-the-...
> 
> Hrm. Apparently it was added in
https://chromiumcodereview.appspot.com/11364204
> to deal with OTHER server quirks.
> 
> Yaay. :-/ I guess we can leave it alone until someone notices? I don't think
> people actually use ftp anymore anyway.

Sounds good to me.  no easy way around this I guess.

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.h (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.h:36: virtual int Stop(int error);
On 2015/02/04 18:51:18, David Benjamin wrote:
> While you're here, is there any reason this is virtual? It doesn't look like
> it...

Done. Agreed, there should not be a virtual modifier.

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction_unittest.cc:921:
ExecuteTransaction(&ctrl_socket, "ftp://host/;type=d", OK);
On 2015/02/04 18:51:18, David Benjamin wrote:
> On 2015/01/30 21:50:44, xunjieli wrote:
> > Added a "/" here so that ";type=d" will not be interpreted as a part of the
> > host. It seems there should be a slash according to RFC 1738 section 3.2.2.
> > 
> > type=i and type=a are tested in DownloadTransactionWithTypecodeI and
> > DownloadTransactionWithTypecodeA.
> 
> Hah. Did this test just never actually test the type=d case before?

Yep. It was not tested before since "host;type=d" cannot be correctly parsed.

[davidben, 2015-02-05 19:42:57]

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:1148: if (resource_type_ ==
RESOURCE_TYPE_DIRECTORY) {
Oof, I'm sorry! I was wrong that RESOURCE_TYPE_UNKNOWN and
RESOURCE_TYPE_DIRECTORY are treated the same outside of that bit up there. This
logic makes it so that if you add type=d and CWD fails, we don't go on to RETR
it, which is probably desirable.

Probably best to put that bit back. (Maybe with a TODO to test this case. :-) )

[xunjieli, 2015-02-05 20:21:58]

Thanks for the review!

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/891013002/diff/1/net/ftp/ftp_network_transact...
net/ftp/ftp_network_transaction.cc:1148: if (resource_type_ ==
RESOURCE_TYPE_DIRECTORY) {
On 2015/02/05 19:42:57, David Benjamin wrote:
> Oof, I'm sorry! I was wrong that RESOURCE_TYPE_UNKNOWN and
> RESOURCE_TYPE_DIRECTORY are treated the same outside of that bit up there.
This
> logic makes it so that if you add type=d and CWD fails, we don't go on to RETR
> it, which is probably desirable.
> 
> Probably best to put that bit back. (Maybe with a TODO to test this case. :-)
)

Done. Hmm yep, saving a RETR is probably better than none :)

[xunjieli, 2015-02-05 20:22:01]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:22:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891013002/40001

[commit-bot: I haz the power, 2015-02-05 21:34:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-05 21:35:50]

Patchset 3 (id:??) landed as
https://crrev.com/abc41a5466e2cd7ad072cc9f2a31691b9de3070d
Cr-Commit-Position: refs/heads/master@{#314882}