wstring: make net::GetSuggestedFilename return utf-16

wstring: make net::GetSuggestedFilename return utf-16

We should only use FilePath when working with real paths on disk.

Note:  I made a minor semantic change to downloads -- we no
longer preserve backslashes in server-suggested paths on non-Windows.
I think the previous behavior is crazy but from looking at the
blame I'm the one who wrote it originally, so I don't feel bad
about changing my mind.

BUG=69467
TEST=existing test still passes

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

[Avi (use Gerrit), 2011-01-24 20:09:51]

Code LGTM, but I'm not sure I agree about the idea.

"We should only use FilePath when working with real paths on disk." Why? I
disagree: a suggested filename is still a filename, and it's safer to play the
wide>MB game once in one place than to trust every caller.

[evanm, 2011-01-24 20:21:01]

On 2011/01/24 20:09:51, Avi wrote:
> "We should only use FilePath when working with real paths on disk." Why? I
> disagree: a suggested filename is still a filename, and it's safer to play the
> wide>MB game once in one place than to trust every caller.

Having looked at this in detail in the downloads code (and in the net function I
just changed), attempting to make all the string-munging code correct in the
presence of multiple different possible encodings of the strings is really
error-prone.

In the case of suggesting a filename, we combining something provided by the
remote server (which is unaware of our OS locale) along with a default provided
by us (generally ascii, but conceptually could be Unicode, like the Japanese
equivalent of "Unnamed download").

Using FilePath drags in all the current-OS-locale goop with it.  That
interaction with path bytestrings provided by the remote server necessarily
requires some encoding conversions before we should trust it, so we may as well
round-trip through unicode -- we don't ever want to accidentally plumb in a
server-provided string of unknown-encoding bytes into a filepath of
unknown-encoding bytes.

[Avi (use Gerrit), 2011-01-24 20:25:13]

Yes, we need to worry about this. But FilePath still feels like the right
approach for paths. Can we keep a constructor from Unicode in it? (And BTW, from
the code it seems like we don't even know the encoding from the remote server.)

[evanm, 2011-01-24 20:27:24]

Mark has historically been opposed to a ctor from strings that aren't
in OS-native format...

On Mon, Jan 24, 2011 at 12:25 PM,  <avi@chromium.org> wrote:
> Yes, we need to worry about this. But FilePath still feels like the right
> approach for paths. Can we keep a constructor from Unicode in it? (And BTW,
> from
> the code it seems like we don't even know the encoding from the remote
> server.)
>
> http://codereview.chromium.org/6241001/
>

[Avi (use Gerrit), 2011-01-24 20:31:27]

I still fail to see what that buys us. OK, so we can't have a constructor from
u16. I still don't see why passing around a string16 and appending filename
pieces in that is better than passing around a FilePath and appending filename
pieces with that.

[evanm, 2011-01-24 20:39:24]

On 2011/01/24 20:31:27, Avi wrote:
> I still fail to see what that buys us. OK, so we can't have a constructor from
> u16. I still don't see why passing around a string16 and appending filename
> pieces in that is better than passing around a FilePath and appending filename
> pieces with that.

Just that it's cumbersome to work with FilePaths as if they were strings, and
frequently when working on file names (not paths, just names) we need to do
that.  E.g. some code I just found by looking randomly:


// TODO(estade): improve this to not choke on alternate encodings.
FilePath::StringType StripOrdinalNumber(
    const FilePath::StringType& pure_file_name) {
  FilePath::StringType::size_type r_paren_index =
      pure_file_name.rfind(FILE_PATH_LITERAL(')'));
  FilePath::StringType::size_type l_paren_index =
      pure_file_name.rfind(FILE_PATH_LITERAL('('));
  if (l_paren_index >= r_paren_index)
    return pure_file_name;

  for (FilePath::StringType::size_type i = l_paren_index + 1;
       i != r_paren_index; ++i) {
    if (!IsAsciiDigit(pure_file_name[i]))
      return pure_file_name;


All this FILE_PATH_LITERAL and ::StringType stuff is verbose, making the code
harder to follow and perhaps even buggy.

[Avi (use Gerrit), 2011-01-24 20:53:30]

If it's that bad, OK.