FilePath: Remove most of ToWStringHack, adding a LossyDisplayName()

FilePath: Remove much of ToWStringHack, adding a LossyDisplayName()

The reason we don't want a free conversion between FilePaths and Unicode
is that it can be lossy.  But when displaying a string to the user,
we're ok if it's lossy when we have no other option.

This change introduces a LossyDisplayName() method that returns a
string16, and converts many of the users of ToWStringHack to use it.

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

[Evan Martin, 2011-02-01 22:29:23]

We discussed this earlier.  I'd appreciate your feedback.

I was hoping I could do a ninja strike and remove ToWStringHack in one go, but
this change is too big already and some of the conversions are already shady. 
If you dislike any of the changes I've done here, point them out and I'll pull
them out into a separate change; there are too many instances for me to not have
an interim period where both ToWStringHack and LossyDisplayName are available.
:\

[Evan Martin, 2011-02-01 22:31:23]

http://codereview.chromium.org/6246036/diff/1/chrome/browser/chrome_plugin_ho...
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/chrome_plugin_ho...
chrome/browser/chrome_plugin_host.cc:470:
path.Append(chrome::kChromePluginDataDirname).LossyDisplayName());
In retrospect, as a way of asserting this is Windows-only, I should probably
just use .value().

[Mark Mentovai, 2011-02-01 22:42:44]

I’m not able to complete the review right now because codereview is so bad.

Here’s the last comment I was working on:

16To8(LossyDisplayName) is frustrating. On the Mac, it’s zero conversions for
the price of two. On Linux, you probably still need to involve that native
multibyte conversion.

I’m see a lot of 16ToWide(LossyDisplayName) too. Also frustrating.

Is it worth having LossyDisplayNameUTF8 and LossyDisplayNameW to avoid
conversions when possible?

http://codereview.chromium.org/6246036/diff/1/base/file_path.cc
File base/file_path.cc (right):

http://codereview.chromium.org/6246036/diff/1/base/file_path.cc#newcode520
base/file_path.cc:520: return WideToUTF16(base::SysNativeMBToWide(path_));
If on Mac, you can just do an 8-to-16 conversion.

http://codereview.chromium.org/6246036/diff/1/chrome/browser/chrome_plugin_ho...
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/chrome_plugin_ho...
chrome/browser/chrome_plugin_host.cc:470:
path.Append(chrome::kChromePluginDataDirname).LossyDisplayName());
Evan Martin wrote:
> In retrospect, as a way of asserting this is Windows-only, I should probably
> just use .value().

In platform-specific code, that’s OK.

http://codereview.chromium.org/6246036/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:1895: std::string path_str =
UTF16ToUTF8(extension_path.LossyDisplayName());
16To8(LossyDisplayName) is frustrating.

I’m also seeing 16ToWide(LossyDisplayName) a bunch in this CL. Also frustrating.

[Avi (use Gerrit), 2011-02-01 22:46:25]

I'm not entirely comfortable here. LossyDisplayName is supposed to be for
display but that's not how it's used in quite a few cases. Even if I leave out
all the tests (which I'm OK with) it's frequent.

http://codereview.chromium.org/6246036/diff/1/chrome/browser/chrome_plugin_ho...
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/chrome_plugin_ho...
chrome/browser/chrome_plugin_host.cc:470:
path.Append(chrome::kChromePluginDataDirname).LossyDisplayName());
That sounds good.

http://codereview.chromium.org/6246036/diff/1/chrome/browser/spellcheck_host.cc
File chrome/browser/spellcheck_host.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/spellcheck_host....
chrome/browser/spellcheck_host.cc:237:
bdict_file_path_.BaseName().LossyDisplayName())));
Whoa. LossyDisplayName is for display, not as a general UTF16 output. I'd rather
ifdef here than do that.

http://codereview.chromium.org/6246036/diff/1/webkit/database/database_tracke...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/6246036/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:723:
ASCIIToUTF16(kExtensionOriginIdentifierPrefix), true)) {
Another one I'm not thrilled with.

[Mark Mentovai, 2011-02-01 22:54:55]

I guess we’re hoping that each new hack is a hack in fewer places than what it
replaces.

http://codereview.chromium.org/6246036/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:1895: std::string path_str =
UTF16ToUTF8(extension_path.LossyDisplayName());
16To8(LossyDisplayName) is frustrating. On the Mac, it’s zero conversions for
the price of two. On Linux, you probably still need to involve that native
multibyte conversion.

I’m see a lot of 16ToWide(LossyDisplayName) too. Also frustrating.

Is it worth having LossyDisplayNameUTF8 and LossyDisplayNameW to avoid
conversions when possible?

http://codereview.chromium.org/6246036/diff/1/chrome/browser/metrics/metrics_...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/metrics/metrics_...
chrome/browser/metrics/metrics_log.cc:286:
CreateBase64Hash(UTF16ToUTF8(iter->path.BaseName().LossyDisplayName())));
Watch the 80.

This is not for display and is thus controversial.

http://codereview.chromium.org/6246036/diff/1/chrome/browser/safe_browsing/fi...
File chrome/browser/safe_browsing/filter_false_positive_perftest.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/safe_browsing/fi...
chrome/browser/safe_browsing/filter_false_positive_perftest.cc:244:
WideToASCII(url_file.value())
This is substantially uglier than what it replaces.

http://codereview.chromium.org/6246036/diff/1/chrome/browser/ui/browser_init.cc
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/browser/ui/browser_init....
chrome/browser/ui/browser_init.cc:968: FilePath param = FilePath(params[i]);
Is this an abuse of FilePath?

http://codereview.chromium.org/6246036/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:471: if
(!GenerateId(UTF16ToUTF8(new_path.LossyDisplayName()), &id))
Not for display?

http://codereview.chromium.org/6246036/diff/1/chrome/test/plugin/pdf_browsert...
File chrome/test/plugin/pdf_browsertest.cc (right):

http://codereview.chromium.org/6246036/diff/1/chrome/test/plugin/pdf_browsert...
chrome/test/plugin/pdf_browsertest.cc:308:
UTF16ToASCII(file_path.BaseName().LossyDisplayName());
To ASCII? Hmm.

[Evan Martin, 2011-02-04 19:04:30]

In the interest of making progress on this, I reverted (or at least attempted to
revert) all of the bits of this that might be controversial, only leaving
instances where the conversion is (hopefully) obviously ok.  If these LGTY then
I will work on the more controversial corners in a followup.

[Evan Martin, 2011-02-04 19:07:07]

Oh, and to answer Mark's questions:

16To8(displayname) does seem a bit silly on OSX because of the
double-conversion, but maybe it'll make you feel better to consider that on
Linux we *always* need to convert because nothing uses UTF-16 there.  Most
instances of 16To8 on a display name are in the extensions code, which really
should have been 16 anyway... a change for the future.

16ToWide() are all temporary, as the intent is to remove all Wide()s from the
codebase.  They are easy to spot by grepping for Wide().  Avi has started
tackling views (which is where the majority of these come from).

[Evan Martin, 2011-02-04 19:10:05]

On 2011/02/04 19:07:07, Evan Martin wrote:
> 16To8(displayname) does seem a bit silly on OSX because of the
> double-conversion

In Haskell you can actually annotate functions to indicate to the compiler legal
transformations for optimization purposes.  E.g. map(f, map(g, list)) == map(f
compose g, list)  (as long as g has no side effects).  This allows you to
express code in inefficient but logical ways and trust that the compiler can
fold them.  Kinda like how inlining works in C++ but it works even for functions
that aren't inline.

[Mark Mentovai, 2011-02-04 19:17:28]

evan@chromium.org wrote:
> Oh, and to answer Mark's questions:
>
> 16To8(displayname) does seem a bit silly on OSX because of the
> double-conversion, but maybe it'll make you feel better to consider that on
> Linux we *always* need to convert because nothing uses UTF-16 there.

Righto.

Doesn’t make me feel any better about doing UTF-8 to UTF-16 to UTF-8
when we know it’s actually UTF-8 on either end of the trip.

[Avi (use Gerrit), 2011-02-04 19:24:52]

Almost scandal-free.

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:373: //
|expected_title_in_finished| need to be checked.
Nitpicky of me: Fix the name of the variable in this comment to match the one it
actually has.

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:316:
extension_path.BaseName().LossyDisplayName());
So this will DCHECK when a user puts a stupidly-named directory in his
extensions dir? Not fond of that. Need moar paranoia.

[Mark Mentovai, 2011-02-04 19:26:58]

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
chrome/browser/download/download_item.cc:490: if (path.find(query) !=
std::wstring::npos)
string16::find doesn’t seem like the right way to search or compare UTF-16 text.

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1896: std::string path_str =
UTF16ToUTF8(extension_path.LossyDisplayName());
:( Still controversial?

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:431:
UTF16ToUTF8(messages_path.LossyDisplayName()).c_str());
:(

[evanm, 2011-02-04 19:37:11]

New version up, also with a Mac compile fix.

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:373: //
|expected_title_in_finished| need to be checked.
On 2011/02/04 19:24:53, Avi wrote:
> Nitpicky of me: Fix the name of the variable in this comment to match the one
it
> actually has.

Done.

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
chrome/browser/download/download_item.cc:490: if (path.find(query) !=
std::wstring::npos)
On 2011/02/04 19:26:58, Mark Mentovai wrote:
> string16::find doesn’t seem like the right way to search or compare UTF-16
text.

I guess for strict substring it's ok, because extension bytes never match
non-extension bytes?  (... here, search for "string searches" on
http://en.wikipedia.org/wiki/UTF-16/UCS-2 )

In any case, I am not changing the logic here.  Though I might as well fix this
to use string16::npos while I'm here.

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1896: std::string path_str =
UTF16ToUTF8(extension_path.LossyDisplayName());
On 2011/02/04 19:26:58, Mark Mentovai wrote:
> :( Still controversial?

This code is all weird ("wrong").  We have a user-reported message below that is
not localized, and then we stuff a path in there and just hope that UTF-8 makes
it through the rest of the code paths ok.  I've spoken with Aaron about this; we
need to make all the extension error-reporting APIs use string16, at which point
this double-conversion will become the necessary single-conversion.

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:316:
extension_path.BaseName().LossyDisplayName());
On 2011/02/04 19:24:53, Avi wrote:
> So this will DCHECK when a user puts a stupidly-named directory in his
> extensions dir? Not fond of that. Need moar paranoia.

I guess DCHECKing is OK, since it is breaking an assertion that might be
interesting at development-time.  And the code below handles the bad path for
non-debug builds.

[Avi (use Gerrit), 2011-02-04 19:42:32]

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
chrome/browser/download/download_item.cc:490: if (path.find(query) !=
std::wstring::npos)
For strict substring, this is valid.

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1896: std::string path_str =
UTF16ToUTF8(extension_path.LossyDisplayName());
I see no filed bug nor reference to it.

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:316:
extension_path.BaseName().LossyDisplayName());
I think I have a different expectation of what a DCHECK is for than you do.

I thought a DCHECK was to catch programmer error. If the code is processing
random crap under the user's control, it's OK to complain in a log, but I don't
think it's OK to allow bad user data to cause a DCHECK.

[Mark Mentovai, 2011-02-04 19:49:32]

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
chrome/browser/download/download_item.cc:490: if (path.find(query) !=
std::wstring::npos)
evanm wrote:
> I guess for strict substring it's ok, because extension bytes never match
> non-extension bytes?  (... here, search for "string searches" on
> http://en.wikipedia.org/wiki/UTF-16/UCS-2 )

I think that discussion only considers whether UTF-16 surrogates pose a problem
for string searches. They don’t. But Unicode’s full of other surprises. What
about composite characters? Are the strings already canonical? Should 0065 0301
compare equal to 00e9?

http://en.wikipedia.org/wiki/Unicode#Ready-made_versus_composite_characters

http://en.wikipedia.org/wiki/Canonical_equivalence

[evanm, 2011-02-04 19:58:49]

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/download/down...
chrome/browser/download/download_item.cc:490: if (path.find(query) !=
std::wstring::npos)
On 2011/02/04 19:49:32, Mark Mentovai wrote:
> evanm wrote:
> > I guess for strict substring it's ok, because extension bytes never match
> > non-extension bytes?  (... here, search for "string searches" on
> > http://en.wikipedia.org/wiki/UTF-16/UCS-2 )
> 
> I think that discussion only considers whether UTF-16 surrogates pose a
problem
> for string searches. They don’t. But Unicode’s full of other surprises. What
> about composite characters? Are the strings already canonical? Should 0065
0301
> compare equal to 00e9?

Good point.  Filed a bug, added a link.

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1896: std::string path_str =
UTF16ToUTF8(extension_path.LossyDisplayName());
On 2011/02/04 19:42:32, Avi wrote:
> I see no filed bug nor reference to it.

I filed 71980.  Are you saying you want me to tag this with a reference to that
bug?  My recollection from looking at the extensions code before is that this
occurs all over the extensions code...

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/6246036/diff/1044/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:316:
extension_path.BaseName().LossyDisplayName());
On 2011/02/04 19:42:32, Avi wrote:
> I thought a DCHECK was to catch programmer error. If the code is processing
> random crap under the user's control, it's OK to complain in a log, but I
don't
> think it's OK to allow bad user data to cause a DCHECK.

OK, I wrote some more thorough checking.

[Avi (use Gerrit), 2011-02-04 20:02:01]

On 2011/02/04 19:58:49, evanm wrote:
> I filed 71980.  Are you saying you want me to tag this with a reference to
that
> bug?  My recollection from looking at the extensions code before is that this
> occurs all over the extensions code...

Hm. I'll take that argument.

> OK, I wrote some more thorough checking.

LGTM

[Mark Mentovai, 2011-02-04 20:20:54]

Let’s call this LGTM and move on to scarier things.