Use the new FTP LIST parsing code in the browser.

Use the new FTP LIST parsing code in the browser.

Fall back to the old parsing code on errors.

TEST=none
BUG=25520
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31379

[eroman, 2009-11-02 19:39:21]

What if we took this one step further --- rather than only falling back to
Mozilla code on parse failure, what if during the transitionary period we ALWAYS
run it through BOTH implementations?

This would have the advantage that we can verify whether successful parses were
actually correct (in case there are subtle compatibility differences in how we
actually parsed it). Moreover, we could track a compatibility index via UMA
histograms to build up confidence on how compatible the new implementation is.

http://codereview.chromium.org/343073/diff/1/3
File net/ftp/ftp_directory_listing_buffer.h (right):

http://codereview.chromium.org/343073/diff/1/3#newcode42
Line 42: // Return recognized server type. It is valid to call this function at
any
style-nit: "Return" --> "Returns".

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

http://codereview.chromium.org/343073/diff/1/4#newcode36
Line 36: virtual FtpServerType GetServerType() = 0;
style-nit: Can this be "const" to match the signature in
FtpDirectoryListingBuffer?

http://codereview.chromium.org/343073/diff/1/5
File webkit/glue/ftp_directory_listing_response_delegate.cc (right):

http://codereview.chromium.org/343073/diff/1/5#newcode139
Line 139: FeedFallbackParser();
style-nit: I would suggest a different ordering here... either list the if
(parser_fallback_) { } case first (with an early return), or group the fallback
code within an else block. Something about the current ordering feels strange to
me.

http://codereview.chromium.org/343073/diff/1/5#newcode176
Line 176: FeedFallbackParser();
Same comment as above. I think this would be clearer if the fallback case were
written as the exceptional case (the thing we test for in the "if" block.

That would also keep the early return section shorter. (reading the above, there
is a large block after which we return.

[Paweł Hajdan Jr., 2009-11-02 21:09:19]

On 2009/11/02 19:39:21, eroman wrote:
> What if we took this one step further --- rather than only falling back to
> Mozilla code on parse failure, what if during the transitionary period we
ALWAYS
> run it through BOTH implementations?

It's not going to work that well I think. For example, the current code is
completely br0ken for VMS (or maybe I'm calling it in a wrong way, but this is
non-obvious). When the two return different results, how can we be sure which
was correct? Well, Mozilla code has much higher chances because it has been
tested for years in the field. However, I'm really confident in the new parser
once it recognizes the format. This is the advantage of its strictness: I
believe that when the new parser gets confused, it will just admit that instead
of returning some gibberish.

> This would have the advantage that we can verify whether successful parses
were
> actually correct (in case there are subtle compatibility differences in how we
> actually parsed it). Moreover, we could track a compatibility index via UMA
> histograms to build up confidence on how compatible the new implementation is.

Yeah, I'm going to add UMA stats about that (in other patch). They will be
critical when making a decision to drop the old parsing code. The current stats
show that a very high percentage of servers (at least 80%, and I think even
more, I don't remember exactly) use Unix ls format. I don't see a way to subtly
break it and confuse the new parser. I'd even boldly claim that the old parser
is more likely to be confused in that case. :) What do you think?

I'm going to fix the issues you pointed out tomorrow.

[Paweł Hajdan Jr., 2009-11-03 19:51:17]

Patch updated. Re-ordering seems to make things even more complicated. We need
to consider the non-fallback scenario first, at least in OnRequestComplete,
because processing there may trigger a fallback.

What do you think about my comments on running always both implementations?

[Paweł Hajdan Jr., 2009-11-04 18:43:04]

ping! ping! ping!

[eroman, 2009-11-04 20:08:18]

lgtm.

any unit-tests for this?

http://codereview.chromium.org/343073/diff/2002/5004
File webkit/glue/ftp_directory_listing_response_delegate.cc (right):

http://codereview.chromium.org/343073/diff/2002/5004#newcode133
Line 133: if (!parser_fallback_) {
May want to add a TODO about eventually removing the fallback.

http://codereview.chromium.org/343073/diff/2002/5004#newcode148
Line 148: switch (entry.type) {
This logic seems like it could be extracted to a function.

  std::string BuildStringEntry(const FtpDirectoryListingEntry);
   response_buffer_.append(BuildStringEntry(entry));

[Paweł Hajdan Jr., 2009-11-05 11:35:45]

Darin: Please take a look, it's a change in webkit/glue.

On 2009/11/04 20:08:18, eroman wrote:
> lgtm.
> 
> any unit-tests for this?

Good idea. I'm not sure yet if it's best to test here (not much difference from
testing in net, and we have tests there), or in browser_tests (I definitely plan
to write more tests there).

> http://codereview.chromium.org/343073/diff/2002/5004
> File webkit/glue/ftp_directory_listing_response_delegate.cc (right):
> 
> http://codereview.chromium.org/343073/diff/2002/5004#newcode133
> Line 133: if (!parser_fallback_) {
> May want to add a TODO about eventually removing the fallback.

Done.

> http://codereview.chromium.org/343073/diff/2002/5004#newcode148
> Line 148: switch (entry.type) {
> This logic seems like it could be extracted to a function.
> 
>   std::string BuildStringEntry(const FtpDirectoryListingEntry);
>    response_buffer_.append(BuildStringEntry(entry));

Done.

[eroman, 2009-11-05 21:43:20]

lgtm