Move FTP LIST parsing code to the renderer process.

Move FTP LIST parsing code to the renderer process.

TEST=none
BUG=none

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

[Paweł Hajdan Jr., 2009-09-19 00:40:15]

So, it works!

One thing I'm concerned about is that the NetModule-related code is duplicated
from BrowserMain. I think that moving the normal directory listing to renderer
as well would allow removing that piece of code from BrowserMain, and only
having it in RendererMain.

Also, please check if I didn't misuse the API when setting the MIME type to
"text/html". Without it the contents displayed as text/plain, but maybe it's
better done in some other way.

[eroman, 2009-09-21 00:36:34]

Cool, I am excited to see this happening!


General implementation comment: I don't think we want to be exposing a new
mime-type.

In other words, a random HTTP server shouldn't be able to return a response
with:
    Content-Type: text/vnd.chromium.ftp-dir
And have it run through the FTP listing code.

I suggest doing away with the "text/vnd.chromium.ftp-dir" mimetype, and instead
plumbing through the fact that it was an ftp directory listing using an enum or
a bool.


Testing: Is there any existing UI test which exercises FTP listing? If not, can
you add one?

http://codereview.chromium.org/210027/diff/3001/4002
File chrome/renderer/renderer_main.cc (right):

http://codereview.chromium.org/210027/diff/3001/4002#newcode87
Line 87: // be localized.  The Chrome locale will never change while we're
running, so
Are you sure about this?

http://codereview.chromium.org/210027/diff/3001/4002#newcode113
Line 113: base::LazyInstance<LazyDirectoryListerCacher> lazy_dir_lister(
I am generally anti-caching.

As I see it, there are two main downsides to caching this:

  (a) memory bloat (especially since this is going to be per-renderer-process)
  (b) more complexity (i.e. what happens if the locale can change).

Unless we have reason to believe that rebuilding the template each time is going
to be measurably slow, I would recommend avoiding caching.

http://codereview.chromium.org/210027/diff/3001/4002#newcode136
Line 136: // Config the network module so it has access to resources.
nit: Can you write "Configure" instead of "Config" ?

http://codereview.chromium.org/210027/diff/3001/4008
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/210027/diff/3001/4008#newcode432
Line 432: if (info.mime_type == "text/vnd.chromium.ftp-dir")
Should this be a case-insensitive comparison?

[darin (slow to review), 2009-09-21 16:50:49]

http://codereview.chromium.org/210027/diff/3001/4002
File chrome/renderer/renderer_main.cc (right):

http://codereview.chromium.org/210027/diff/3001/4002#newcode113
Line 113: base::LazyInstance<LazyDirectoryListerCacher> lazy_dir_lister(
On 2009/09/21 00:36:34, eroman wrote:
> I am generally anti-caching.
> 
> As I see it, there are two main downsides to caching this:
> 
>   (a) memory bloat (especially since this is going to be per-renderer-process)
>   (b) more complexity (i.e. what happens if the locale can change).
> 
> Unless we have reason to believe that rebuilding the template each time is
going
> to be measurably slow, I would recommend avoiding caching.

I think something like this is necessitated by the fact that 
NetResourceProvider returns a StringPiece.

[darin (slow to review), 2009-09-21 16:51:11]

Oh, and I agree in general that re-computing the result each time is probably
better for non-performance sensitive stuff like this.

[wtc, 2009-09-21 18:59:13]

eroman and darin can review the renderer changes better
than I can.  I have just some preliminary comments below.

http://codereview.chromium.org/210027/diff/3001/4002
File chrome/renderer/renderer_main.cc (right):

http://codereview.chromium.org/210027/diff/3001/4002#newcode29
Line 29: #include "net/base/net_module.h"
Nit: This should be listed after the "grit" headers.

http://codereview.chromium.org/210027/diff/3001/4007
File webkit/glue/ftp_directory_listing_response_delegate.h (right):

http://codereview.chromium.org/210027/diff/3001/4007#newcode13
Line 13: #include "net/third_party/parseftp/ParseFTPList.h"
Why don't we use third_party\WebKit\WebCore\loader\FTPDirectoryParser.h?
We don't need to duplicate code.

[Paweł Hajdan Jr., 2009-09-21 20:22:44]

I moved the "caching" code to chrome/common/net. We're stuck with this "caching"
because StringPiece doesn't copy underlying data. But I simplified the storage a
little, from LazyInstance to local static, as Darin suggested.

wtc, using WebKit code here would require more plumbing through the WebKit API
(or ask Darin for details, I'm not familiar enough with it), and I plan to
rewrite the code anyway.

[Paweł Hajdan Jr., 2009-09-21 20:27:35]

eroman, I plan to add a UI test but currently it would just navigate to an FTP
directory listing without being able to do much more. I'll have to figure out
how to test more things, which will take some time.

If you think it's valuable, I can add a simple "just navigate" test, to hit the
new code paths.

[Paweł Hajdan Jr., 2009-09-22 16:32:09]

ping! ping! ping!

[darin (slow to review), 2009-09-22 18:21:20]

overall, looks great to me.  just some minor issues:

http://codereview.chromium.org/210027/diff/2003/1014
File chrome/common/net/net_resource_provider.cc (right):

http://codereview.chromium.org/210027/diff/2003/1014#newcode6
Line 6: 
might be nice to prepare this file using "svn cp" so that svn history is
preserved

http://codereview.chromium.org/210027/diff/2003/1021
File webkit/glue/ftp_directory_listing_response_delegate.cc (right):

http://codereview.chromium.org/210027/diff/2003/1021#newcode5
Line 5: #include "webkit/glue/ftp_directory_listing_response_delegate.h"
again, using 'svn cp' to preserve history and make the diff easier to review
would be good.

http://codereview.chromium.org/210027/diff/2003/1021#newcode122
Line 122: input_buffer_.append(std::string(data, data_len));
i think you can avoid the intermediate std::string object here.

input_buffer_.append(data, data_len);

http://codereview.chromium.org/210027/diff/2003/1023
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/210027/diff/2003/1023#newcode433
Line 433: response.setMIMEType(StdStringToWebString("text/html"));
nit: use WebString::fromUTF8 here

as part of the webkit api work, i plan on deleting methods like
StdStringToWebString.

[eroman, 2009-09-23 21:14:49]

lgtm!

http://codereview.chromium.org/210027/diff/2012/2023
File webkit/glue/ftp_directory_listing_response_delegate.h (right):

http://codereview.chromium.org/210027/diff/2012/2023#newcode44
Line 44: // starting point for each parts response.
nit: "parts" --> "part's"

http://codereview.chromium.org/210027/diff/2012/2024
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/210027/diff/2012/2024#newcode432
Line 432: if (info.mime_type == "text/vnd.chromium.ftp-dir")
I am still not convinced that we want to expose this mime type for other server
responses to use... But I guess if Darin is OK with it then it's fine.

http://codereview.chromium.org/210027/diff/2012/2024#newcode433
Line 433: response.setMIMEType(WebString::fromUTF8("text/html"));
should we additionally specify the charset? Or is the listing HTML going to be
purely ISO-8859-1?