Move filename determination to net_util

Move filename determination logic to net_util so that it's not split between net_util and download_util.

BUG=78085
TEST=net_unittests --gtest_filter=*Generate*FileName


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

[asanka, 2011-07-22 19:54:47]

Download filename determination logic is split between download_util and
net_util.  This CL is to move the logic to net_util with the intention of
centralizing filename generation to net::GetSuggestedFilename().

[asanka, 2011-07-25 01:10:16]

[+rvargas]

[Paweł Hajdan Jr., 2011-07-25 18:38:27]

LGTM, I didn't look into much details assuming it's just a move.

[rvargas (doing something else), 2011-07-26 00:10:56]

Who's doing the detailed review?

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:948: return "";
nit: return std::string();

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1447: // component of the URL and doesn't failover to
returning the hostname.
nit: failover as a verb

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1455: if (filename.empty() && default_name.empty() &&
The previous logic didn't reach this point for data or about urls.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1461: TrimGeneratedFileName(filename);
Looks like it's better to call this after the next line instead of keep doing
this.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode177
net/base/net_util.h:177: // net_util_unittest.
You can remove this TODO because that's basically the new behavior.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util_unittest.cc
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util_unittest....
net/base/net_util_unittest.cc:1094: FILE_PATH_LITERAL("C:\\foo\\bar.htm")},
nit: now that we're here let's get closer to the regular format for a struct and
send the closing bracket to the next line. Maybe do the same with the opening
one, or at least add a space so that there is proper indentation.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util_unittest....
net/base/net_util_unittest.cc:1133:
FILE_PATH_LITERAL("C:\\foo\\harmless.{not-really-this-may-be-a-guid}"),
This looks like a new test.

[Randy Smith (Not in Mondays), 2011-07-26 13:44:30]

On 2011/07/26 00:10:56, rvargas wrote:
> Who's doing the detailed review?

Ricardo: If you feel like you can do the detailed review, I'd be grateful--this
week most of the balls I try to keep in the air are dropping at the same time. 
But to answer your question, if it's not you, it's me.

[rvargas (doing something else), 2011-07-26 22:25:41]

> On 2011/07/26 00:10:56, rvargas wrote:
> > Who's doing the detailed review?
>
> Ricardo: If you feel like you can do the detailed review, I'd
> be grateful--this

I was afraid that was going to be the answer.

I still have to go through the unit tests.

http://codereview.chromium.org/7300005/diff/15006/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/7300005/diff/15006/content/browser/download/sa...
content/browser/download/save_package.cc:1270: // made from the UI thread.
Could you clarify (for me) what does this mean?. Are we changing the current
behavior (it doesn't look like)? I'm just double checking that this is just
documenting a pre-existing problem.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode264
net/base/net_util.h:264: // OS_WIN, trailing spaces are also removed.
nit: I think that we use "Windows" when describing the code.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode282
net/base/net_util.h:282: // Similar to GetSuggestedFilename(), but returns a
FilePath.
It looks like long term we should have just one method.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode290
net/base/net_util.h:290: // Ensure that the filename and extension is safe to
use in the filesystem.
nit: Ensures

[Randy Smith (Not in Mondays), 2011-07-27 19:07:45]

On 2011/07/26 22:25:41, rvargas wrote:
> > On 2011/07/26 00:10:56, rvargas wrote:
> > > Who's doing the detailed review?
> >
> > Ricardo: If you feel like you can do the detailed review, I'd
> > be grateful--this
> 
> I was afraid that was going to be the answer.

Thank you very much.   I do think this makes sense--I don't have a lot of
familiarity with this code.  But I'm going to take a swing through and do at
least a surface review now.

[Randy Smith (Not in Mondays), 2011-07-27 19:46:14]

Control flow refactor, */browser/download/* cleanup LGTM.  Thanks very much for
doing this; even this first step leaves things cleaner.

[rvargas (doing something else), 2011-07-27 22:19:59]

Actually, I don't have any more comments for the unit tests. just the style
thing that I mentioned before.

[asanka, 2011-07-28 20:04:38]

http://codereview.chromium.org/7300005/diff/15006/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/7300005/diff/15006/content/browser/download/sa...
content/browser/download/save_package.cc:1270: // made from the UI thread.
On 2011/07/26 22:25:41, rvargas wrote:
> Could you clarify (for me) what does this mean?. Are we changing the current
> behavior (it doesn't look like)? I'm just double checking that this is just
> documenting a pre-existing problem.

This is documenting a pre-existing problem.  net::GenerateSafeFileName() (as did
download_util::GenerateSafeFileName()) ends up calling
net::GetPreferredExtensionForMimeType().  The lookup logic may go to disk to
look-up the MIME->extension mapping.  See http://crbug.com/61827.

I'll add a bug annotation.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:948: return "";
On 2011/07/26 00:10:57, rvargas wrote:
> nit: return std::string();

Done.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1447: // component of the URL and doesn't failover to
returning the hostname.
On 2011/07/26 00:10:57, rvargas wrote:
> nit: failover as a verb

Done.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1455: if (filename.empty() && default_name.empty() &&
On 2011/07/26 00:10:57, rvargas wrote:
> The previous logic didn't reach this point for data or about urls.

Shouldn't the !url.host().empty() exclude 'about' and 'data' URLs?

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1461: TrimGeneratedFileName(filename);
On 2011/07/26 00:10:57, rvargas wrote:
> Looks like it's better to call this after the next line instead of keep doing
> this.

I did it this way so that if trimming the filename causes it to disappear
completely, the next filename.empty() would try the next method.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode177
net/base/net_util.h:177: // net_util_unittest.
On 2011/07/26 00:10:57, rvargas wrote:
> You can remove this TODO because that's basically the new behavior.

Done.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode264
net/base/net_util.h:264: // OS_WIN, trailing spaces are also removed.
On 2011/07/26 22:25:41, rvargas wrote:
> nit: I think that we use "Windows" when describing the code.

Done.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode282
net/base/net_util.h:282: // Similar to GetSuggestedFilename(), but returns a
FilePath.
On 2011/07/26 22:25:41, rvargas wrote:
> It looks like long term we should have just one method.

Perhaps the correct solution is to move the encoding switching somewhere else
(FilePath? file_util?) and then have callers use that to create a FilePath from
the string16() that GetSuggestedFilename() returns.

Some call sites (e.g.: in weburlloader_impl.cc) require that the filename be
Unicode.  So, for the moment, we can't do away with a  function that generates a
string16() filename.

All GenerateFileName() does for us now is avoid duplication of the encoding
switching code.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode290
net/base/net_util.h:290: // Ensure that the filename and extension is safe to
use in the filesystem.
On 2011/07/26 22:25:41, rvargas wrote:
> nit: Ensures

Done.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util_unittest.cc
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util_unittest....
net/base/net_util_unittest.cc:1094: FILE_PATH_LITERAL("C:\\foo\\bar.htm")},
On 2011/07/26 00:10:57, rvargas wrote:
> nit: now that we're here let's get closer to the regular format for a struct
and
> send the closing bracket to the next line. Maybe do the same with the opening
> one, or at least add a space so that there is proper indentation.

Done.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util_unittest....
net/base/net_util_unittest.cc:1133:
FILE_PATH_LITERAL("C:\\foo\\harmless.{not-really-this-may-be-a-guid}"),
On 2011/07/26 00:10:57, rvargas wrote:
> This looks like a new test.

Yup.  I added a couple of new tests for validation/sanitation steps that we
weren't testing for.

[rvargas (doing something else), 2011-07-29 01:34:53]

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1455: if (filename.empty() && default_name.empty() &&
On 2011/07/28 20:04:38, asanka wrote:
> On 2011/07/26 00:10:57, rvargas wrote:
> > The previous logic didn't reach this point for data or about urls.
> 
> Shouldn't the !url.host().empty() exclude 'about' and 'data' URLs?

Yes. Please add a comment about that (we are losing the old comment)

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.cc#newcod...
net/base/net_util.cc:1461: TrimGeneratedFileName(filename);
On 2011/07/28 20:04:38, asanka wrote:
> On 2011/07/26 00:10:57, rvargas wrote:
> > Looks like it's better to call this after the next line instead of keep
doing
> > this.
> 
> I did it this way so that if trimming the filename causes it to disappear
> completely, the next filename.empty() would try the next method.

I'm not so sure about that. For instance, if the suggested name evaporates after
trimming I'd consider that a programmer's error, and while it may make sense to
generate something else that's probably not the right thing to do.

I think it's better to just consider that a bug with whatever path is going away
(CD for instance), and let it reach the last safety net (kFinalFallbackName)
instead of the whole staircase.

It's also better to have a CL that only moves stuff without changing the logic,
and then change what we want. That's consistent with the CL description.

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode282
net/base/net_util.h:282: // Similar to GetSuggestedFilename(), but returns a
FilePath.
On 2011/07/28 20:04:38, asanka wrote:
> On 2011/07/26 22:25:41, rvargas wrote:
> > It looks like long term we should have just one method.
> 
> Perhaps the correct solution is to move the encoding switching somewhere else
> (FilePath? file_util?) and then have callers use that to create a FilePath
from
> the string16() that GetSuggestedFilename() returns.
> 
> Some call sites (e.g.: in weburlloader_impl.cc) require that the filename be
> Unicode.  So, for the moment, we can't do away with a  function that generates
a
> string16() filename.
> 
> All GenerateFileName() does for us now is avoid duplication of the encoding
> switching code.

Yeah, I saw the WebUrlLoader case... The helper can be part of this file
(although that's a little weird), but having two  public 6-arguments methods
that only differ on the return type feels wrong.

In any case, that can wait for another CL.

[asanka, 2011-07-29 13:37:42]

On 2011/07/29 01:34:53, rvargas wrote:
> It's also better to have a CL that only moves stuff without changing the
logic,
> and then change what we want. That's consistent with the CL description.

Understood.  I'll move the logic change out to a different CL and upload a new
patchset.

> http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
http://codereview.chromium.org/7300005/diff/15006/net/base/net_util.h#newcode282
> net/base/net_util.h:282: // Similar to GetSuggestedFilename(), but returns a
> FilePath.
> On 2011/07/28 20:04:38, asanka wrote:
> > On 2011/07/26 22:25:41, rvargas wrote:
> > > It looks like long term we should have just one method.
> > 
> > Perhaps the correct solution is to move the encoding switching somewhere
else
> > (FilePath? file_util?) and then have callers use that to create a FilePath
> from
> > the string16() that GetSuggestedFilename() returns.
> > 
> > Some call sites (e.g.: in weburlloader_impl.cc) require that the filename be
> > Unicode.  So, for the moment, we can't do away with a  function that
generates
> a
> > string16() filename.
> > 
> > All GenerateFileName() does for us now is avoid duplication of the encoding
> > switching code.
> 
> Yeah, I saw the WebUrlLoader case... The helper can be part of this file
> (although that's a little weird), but having two  public 6-arguments methods
> that only differ on the return type feels wrong.
> 
> In any case, that can wait for another CL.

Agreed.

[asanka, 2011-08-01 18:27:14]

Removed logic changes, which will go into a separate CL.  PTAL.

[rvargas (doing something else), 2011-08-01 19:12:19]

RBSTMP LGTM.

I was expecting to see only changes inside GetSuggeestedFilename, but now there
are too many changes for rev8:rev10 to make sense.

So a superficial pass of the latest version looks good, but I'm not doing
another detailed review... hence RBSTMP.

[asanka, 2011-08-08 18:40:18]

The only remaining parts of the patch are mostly for moving code around and
changing call sites.  So once I clear the try bots, I'll commit this CL so we
can move forward to the logic refactor CLs.

[commit-bot: I haz the power, 2011-08-09 18:56:48]

Change committed as 96029