net/ftp: Fix control response parsing for a corner cases where the first line...

net/ftp: Fix control response parsing for a corner cases where the first line of a continuation has 0 length.

BUG=29322
TEST=ftp://sourceware.org works.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33986

[eroman, 2009-12-05 03:27:06]

Deferring this review to Pawel.

http://codereview.chromium.org/467032/diff/1/4
File net/ftp/ftp_ctrl_response_buffer.h (right):

http://codereview.chromium.org/467032/diff/1/4#newcode97
net/ftp/ftp_ctrl_response_buffer.h:97: // Wheether or not we are in the middle
of parsing multiple lines.
typo.

[Paweł Hajdan Jr., 2009-12-05 09:24:39]

Thanks for the fix. Some comments below.

http://codereview.chromium.org/467032/diff/1/3
File net/ftp/ftp_ctrl_response_buffer.cc (right):

http://codereview.chromium.org/467032/diff/1/3#newcode24
net/ftp/ftp_ctrl_response_buffer.cc:24: if (!multiline_) {
Could you now reverse the order of the "if" branches?

Like if (!multiline) { A } else { B } -> if (multiline) { B } else { A }

Double-negation may be harder to follow.

http://codereview.chromium.org/467032/diff/1/3#newcode44
net/ftp/ftp_ctrl_response_buffer.cc:44: multiline_ = true;
multiline_ is already true here, no reason to set it again

http://codereview.chromium.org/467032/diff/1/3#newcode51
net/ftp/ftp_ctrl_response_buffer.cc:51: multiline_ = true;
same here

http://codereview.chromium.org/467032/diff/1/4
File net/ftp/ftp_ctrl_response_buffer.h (right):

http://codereview.chromium.org/467032/diff/1/4#newcode28
net/ftp/ftp_ctrl_response_buffer.h:28: : multiline_(false) {
nit: Probably no reason to break the line here (you can put it at the end of the
previous one).

http://codereview.chromium.org/467032/diff/1/4#newcode97
net/ftp/ftp_ctrl_response_buffer.h:97: // Wheether or not we are in the middle
of parsing multiple lines.
I'd prefer "True if we are in the middle of parsing a multi-line response."

Also, could you put it just before line_buf_ declaration? This way the order
would be more logical.

http://codereview.chromium.org/467032/diff/1/2
File net/ftp/ftp_ctrl_response_buffer_unittest.cc (right):

http://codereview.chromium.org/467032/diff/1/2#newcode103
net/ftp/ftp_ctrl_response_buffer_unittest.cc:103: EXPECT_EQ(net::OK,
PushDataToBuffer("sourceware.org\r\n"));
Could you use example.com here?

[Lei Zhang, 2009-12-07 20:07:40]

http://codereview.chromium.org/467032/diff/1/3
File net/ftp/ftp_ctrl_response_buffer.cc (right):

http://codereview.chromium.org/467032/diff/1/3#newcode24
net/ftp/ftp_ctrl_response_buffer.cc:24: if (!multiline_) {
On 2009/12/05 09:24:39, Paweł Hajdan Jr. wrote:
> Could you now reverse the order of the "if" branches?
> 
> Like if (!multiline) { A } else { B } -> if (multiline) { B } else { A }
> 
> Double-negation may be harder to follow.

Sure. I was just trying to minimize the diff / preserve svn history.

http://codereview.chromium.org/467032/diff/1/3#newcode44
net/ftp/ftp_ctrl_response_buffer.cc:44: multiline_ = true;
On 2009/12/05 09:24:39, Paweł Hajdan Jr. wrote:
> multiline_ is already true here, no reason to set it again

Done.

http://codereview.chromium.org/467032/diff/1/3#newcode51
net/ftp/ftp_ctrl_response_buffer.cc:51: multiline_ = true;
On 2009/12/05 09:24:39, Paweł Hajdan Jr. wrote:
> same here

Done.

http://codereview.chromium.org/467032/diff/1/4
File net/ftp/ftp_ctrl_response_buffer.h (right):

http://codereview.chromium.org/467032/diff/1/4#newcode28
net/ftp/ftp_ctrl_response_buffer.h:28: : multiline_(false) {
On 2009/12/05 09:24:39, Paweł Hajdan Jr. wrote:
> nit: Probably no reason to break the line here (you can put it at the end of
the
> previous one).

Done.

http://codereview.chromium.org/467032/diff/1/4#newcode97
net/ftp/ftp_ctrl_response_buffer.h:97: // Wheether or not we are in the middle
of parsing multiple lines.
On 2009/12/05 09:24:39, Paweł Hajdan Jr. wrote:
> I'd prefer "True if we are in the middle of parsing a multi-line response."
> 
> Also, could you put it just before line_buf_ declaration? This way the order
> would be more logical.

Done.

http://codereview.chromium.org/467032/diff/1/2
File net/ftp/ftp_ctrl_response_buffer_unittest.cc (right):

http://codereview.chromium.org/467032/diff/1/2#newcode103
net/ftp/ftp_ctrl_response_buffer_unittest.cc:103: EXPECT_EQ(net::OK,
PushDataToBuffer("sourceware.org\r\n"));
On 2009/12/05 09:24:39, Paweł Hajdan Jr. wrote:
> Could you use http://example.com here?

Done.

[Paweł Hajdan Jr., 2009-12-07 20:10:20]

LGTM