Bug fix for 10876, file extension handling refactoring

Fix for bug 10876 that resulted in some refactoring:

The bug originates from extensions being treated case sensitive on Windows and Mac OSX, where they shouldn't be. 

Therefore I added generic static methods to FilePath to compare strings in the same way the file system does, and changed the relevant parts of the code to make use of them.
I tested the methods under Windows and Mac OS X. I also wrote a basic version for Linux/Posix that behaves the same way as the original code, so there should at least be no regression.

Also, while fixing this I found some confusion in the code about whether extensions are used with or without leading dot. For this reason I changed some functions that were taking an extension as parameter to instead take the whole file path. This makes calling these functions easier and the caller doesn't need to know whether the extension is supposed to be with or without dot.

In the same vein, I split DownloadManager::IsExecutable into IsExecutableFile, where one again passes in the whole file and doesn't have to worry about getting the extension right, and IsExecutableExtension, which corresponds to the original functionality. Ideally only the former method should be public, but that again would have required further code scrubbing that was (even more) outside of the original bug fix.

Finally, fixed a wrong comment in the file path tests.


BUG=10876
TEST=FilePathTest.MatchesExtension, .CompareIgnoreCase


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

[Roland, 2009-07-17 08:37:49]

"Simple" bug fix for bug #10876 that resulted in a bit of file extension
handling refactoring.

(updated patch set to pacify lint.)

Cheers,

Roland

[Erik does not do reviews, 2009-07-17 16:18:35]

I added markmentovai to the review and cc'd avi, both of whom will almost
certainly have comments on much of this.

I tend to think that the model of pulling out the extension and doing
comparisons puts extra logic burden on the caller and instead the comparison
logic should be in FilePath.

http://codereview.chromium.org/149796/diff/1020/24
File base/file_path.h (right):

http://codereview.chromium.org/149796/diff/1020/24#newcode174
Line 174: StringType Extension(bool include_dot = true) const;
style guide doesn't allow default arguments.  Create a second function.

http://codereview.chromium.org/149796/diff/1020/24#newcode181
Line 181: StringType CanonicalizedExtension(bool include_dot = true) const;
ditto

http://codereview.chromium.org/149796/diff/1020/24#newcode205
Line 205: bool MatchesExtension(const StringType& extension) const;
This function was added recently to handle the cases sensitivity comparison that
you were trying to solve above.  Isn't it enough?

http://codereview.chromium.org/149796/diff/1020/28
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/149796/diff/1020/28#newcode1223
Line 1223: FilePath::StringType extension = path.CanonicalizedExtension(false);
why canonicalize here?  Shouldn't it do this in IsExecutableExtension?

http://codereview.chromium.org/149796/diff/1020/28#newcode1236
Line 1236: FilePath::StringType extension = path.CanonicalizedExtension(false);
same here

http://codereview.chromium.org/149796/diff/1020/28#newcode1281
Line 1281: const FilePath::StringType& extension) const {
it seems like this method could be rewritten better using MatchesExtension (or
adding a static variant that compares two extensions)

http://codereview.chromium.org/149796/diff/1020/26
File chrome/browser/icon_manager_win.cc (right):

http://codereview.chromium.org/149796/diff/1020/26#newcode11
Line 11: if (extension != L".exe" && extension != L".dll" && extension !=
L".ico")
use MatchesExtension instead

http://codereview.chromium.org/149796/diff/1020/20
File net/base/filter.cc (right):

http://codereview.chromium.org/149796/diff/1020/20#newcode112
Line 112: if (0 == extension.compare(FILE_PATH_LITERAL(".gz")) ||
these should use MatchesExtension instead.  since this pattern of multiple
compares seems common, perhaps we could add a version of MatchesExtension that
takes a list of extensions.

[Avi (use Gerrit), 2009-07-17 16:52:00]

I'm ambivalent. I really do like the ability to ask for the extension with no
period (goodness knows that the first thing I always have to do is strip the
period in every caller). I'm less enthused by the idea of "canonical extension".
As was asked earlier, how doesn't "matches extension" work for you?

http://codereview.chromium.org/149796/diff/1020/23
File base/file_path.cc (right):

http://codereview.chromium.org/149796/diff/1020/23#newcode358
Line 358: DCHECK(extension.empty() || extension[0] == kExtensionSeparator);
Really like this DCHECK.

http://codereview.chromium.org/149796/diff/1020/23#newcode376
Line 376: current_extension.begin());
Er, I wrote this code specifically to work on all platforms; ifdefing it is
really unnecessary. Just because the Mac can have a case-sensitive filesystem
doesn't affect whether extensions are or are not matched case-insensitively.
Those are orthogonal issues. (BTW, they are matched case-insensitively. Always.)

[Mark Mentovai, 2009-07-17 16:55:15]

I haven't looked at the change yet but I have read the description.

Doesn't MatchesExtension handle what you need?

[Avi (use Gerrit), 2009-07-17 17:02:37]

http://codereview.chromium.org/149796/diff/1020/28
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/149796/diff/1020/28#newcode844
Line 844: ShouldOpenFileBasedOnExtension(download->full_path()))
This code has changed since you touched it; the original code is buggy. The
auto-open list has no leading periods; make sure you don't reintroduce the bug.

[Roland, 2009-07-21 05:59:51]

Thanks all for the detailed feedback!

As for the main question regarding MatchesExtension: The issue described in bug
#10876 stems from DownloadManager::ShouldOpenFileExtension() using the std::set
find() method to look for the extension in auto_open_. That was the main thing I
wanted to address with CanonicalizedExtension(). When making those changes I
also found a few other issues that I corrected along the way (the mentioned
leading dot issue with auto_open_, and ShouldOpenFileExtension treating the
extension as a file path).

Now, I could rewrite the code to use MatchesExtension(), or using a
case-insensitive comparison functor for the auto_open_ set itself (and by
analogy also the exe_types_ set). 
However I think that the contents of those sets should also be stored in a
canonicalized way (hence CanonicalizedExtension()).

As it seems that a public CanonicalizedExtension() method doesn't have too many
fans here ;), I'd suggest rewriting those parts of the code to use a
case-insensitive comparison functor, and also rewriting
OpenFilesBasedOnExtension to make extensions lower-case before storing them.

As for my other changes to method signatures (taking full paths instead of
extensions): I still think they are worthwhile, as they eliminate the need to
know whether an extension should have a dot or not, and reducing the caller's
burden to extract the extension just for calling the method.

Do you guys have any different opinions or suggestions?

[Roland, 2009-07-21 06:27:52]

On 2009/07/17 16:52:00, Avi wrote:
> http://codereview.chromium.org/149796/diff/1020/23#newcode376
> Line 376: current_extension.begin());
> Er, I wrote this code specifically to work on all platforms; ifdefing it is
> really unnecessary. Just because the Mac can have a case-sensitive filesystem
> doesn't affect whether extensions are or are not matched case-insensitively.
> Those are orthogonal issues. (BTW, they are matched case-insensitively.
Always.)

I was actually thinking mainly of Linux when I added the #ifdef, Mac was more of
an afterthought (I realize the way I wrote it doesn't make that clear :p). 

I.e., do we treat Linux as case-insensitive as well?

[Roland, 2009-07-21 08:34:04]

One further suggestion: I wonder if it wouldn't be better to have a separate
'FileExtension' class that takes care of all the mentioned issues (case, dot/no
dot) and wraps file extensions in the same way that FilePath does for whole
paths.

In case you're in favor of implementing such a class: should that be a sub-class
of FilePath (less refactoring), a separate class, but dependent on FilePath
(similar), or a wholly separate class (more refactoring: e.g.,
FilePath::StringType would need to become a common type).

[Mark Mentovai, 2009-07-21 15:06:43]

Roland wrote:
> One further suggestion: I wonder if it wouldn't be better to have a separate
> 'FileExtension' class that takes care of all the mentioned issues (case,
dot/no
> dot) and wraps file extensions in the same way that FilePath does for whole
> paths.

I think that might be an excessive abstraction.

[Roland, 2009-08-06 09:12:59]

Removed the contentious parts of my patch and only fixed those parts of the code
immediately pertinent to the issue.

However, I believe that there are still issues regarding case handling when
comparing file extensions. I added comments and unit test cases to document
them. The latter are commented out, as they are failing.

[Avi (use Gerrit), 2009-08-07 19:10:45]

http://codereview.chromium.org/149796/diff/3001/3007
File base/file_path.cc (right):

http://codereview.chromium.org/149796/diff/3001/3007#newcode350
Line 350: // Also, the comparison should probably be case-sensitive for LINUX.
Should it? Find a Linux expert to answer the question.

http://codereview.chromium.org/149796/diff/3001/3009
File base/string_util.h (right):

http://codereview.chromium.org/149796/diff/3001/3009#newcode546
Line 546: // FIXME: This doesn't work correct in all cases, esp. UTF-8
You can't use "FIXME". You're allowed "TODO()" with a name in there. Preferably
yours.

http://codereview.chromium.org/149796/diff/3001/3003
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/149796/diff/3001/3003#newcode1290
Line 1290: if (open && !executable)
No. As commented above (on the old line 868 and removed in this file revision),
the auto-open list is dot-less. You fail to remove the dot, and things won't
match. (I made the same mistake and broke auto-opening, so that's why I see it.)

http://codereview.chromium.org/149796/diff/3001/3003#newcode1318
Line 1318: if (auto_open_.find(extension) != auto_open_.end() ||
Same as above. The auto-open list is dotless. You can't change it (ever) as
that's a saved user preference.

http://codereview.chromium.org/149796/diff/3001/3003#newcode1379
Line 1379: ascii_extension.erase(0, 1);
:\ Either the parameter always has a dot or it doesn't. I don't like code that
has to guess.

[Evan Stade, 2009-08-07 19:54:14]

http://codereview.chromium.org/149796/diff/3001/3007
File base/file_path.cc (right):

http://codereview.chromium.org/149796/diff/3001/3007#newcode350
Line 350: // Also, the comparison should probably be case-sensitive for LINUX.
On 2009/08/07 19:10:45, Avi wrote:
> Should it? Find a Linux expert to answer the question.

Files are case-sensitive of course but if you just want to ask "is this a jpeg?"
then you probably don't care if its jpeg or JPEG. So I'd say the way we use it,
it should be case-insensitive.

[Erik does not do reviews, 2009-08-14 00:14:44]

http://codereview.chromium.org/149796/diff/3001/3006
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/149796/diff/3001/3006#newcode727
Line 727: // FIXME: The following should pass, but currently don't in the
as with Avi's other comment, use TODO(rolandsteiner).  Also, you should have a
bug # for this issue, and refer to it in the TODO.

http://codereview.chromium.org/149796/diff/3001/3003
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/149796/diff/3001/3003#newcode1268
Line 1268: void DownloadManager::OpenFilesBasedOnExtension(
This implementation looks identical to ShouldOpen... except for the end. 
Shouldn't it just call ShouldOpen... instead of reimplementing the same logic?

[Roland, 2009-08-14 05:18:32]

On 2009/08/07 19:10:45, Avi wrote:
> http://codereview.chromium.org/149796/diff/3001/3003#newcode1290
> Line 1290: if (open && !executable)
> No. As commented above (on the old line 868 and removed in this file
revision),
> the auto-open list is dot-less. You fail to remove the dot, and things won't
> match. (I made the same mistake and broke auto-opening, so that's why I see
it.)

ARGH! You're right of course. Dunno how that happened, esp. after 1.) you
explicitly cautioned against this before and 2.) I did notice and fix the
original bug in my original patch! >_<;

Will fix this (again) with my next patch set.

> http://codereview.chromium.org/149796/diff/3001/3003#newcode1379
> Line 1379: ascii_extension.erase(0, 1);
> :\ Either the parameter always has a dot or it doesn't. I don't like code that
> has to guess.

The question is, where does the extension come from? If it's from the
Extension() method, then it will have a leading dot. If it comes from somewhere
else, e.g., the auto open list, it may not. So this query is there to keep the
method generic and easy to use for the caller.

It's also not the only place where such a query happens (e.g., ReplaceExtension,
amongst others). Cleaning this up may be worthwhile, but out of the scope of
this bug, IMHO.

On 2009/08/07 19:54:14, Evan Stade wrote:
> http://codereview.chromium.org/149796/diff/3001/3007#newcode350
> Line 350: // Also, the comparison should probably be case-sensitive for LINUX.
> On 2009/08/07 19:10:45, Avi wrote:
> > Should it? Find a Linux expert to answer the question.
> 
> Files are case-sensitive of course but if you just want to ask "is this a
jpeg?"
> then you probably don't care if its jpeg or JPEG. So I'd say the way we use
it,
> it should be case-insensitive.

Sounds good (and makes things easier in any case), but I still wonder whether
there can never be other use cases for this method where - on Linux - case would
matter. That is, IMHO the function should make this explicit, e.g., by being
named MatchesExtensionCaseInsensitive (?).

[Roland, 2009-08-20 12:33:01]

New revision: added static utility methods to FilePath that compare strings the
same way as the file system does, and changed the appropriate methods to use
them. 

Changed the auto_open_ set to use a comparator functor that is case insensitive
based on those methods.

Added unit tests for the new methods.

Tested on Windows and Mac OSX, Linux uses fallback functions that should behave
like the original code (so at least no regression should occur).

[jungshik at Google, 2009-09-02 17:34:51]

http://codereview.chromium.org/149796/diff/5001/5003
File base/file_path.cc (right):

http://codereview.chromium.org/149796/diff/5001/5003#newcode437
Line 437: wchar_t c2 = (wchar_t)LOWORD(::CharUpperW((LPWSTR)MAKELONG(*i2, 0)));
drive-in comment: I wonder what happens if you pass along eszett (small letter
sharp s : U+00DF) to CharUpperW. The single character uppercase counterpart was
not introduced until Unicode 5.1. 

Moreover, your code cannot deal with filenames containing non-BMP  characters (>
U+FFFF represented in two wchar_t's on Windows). Can you just use
CharUpperBuffer or CharUpper after copying or LCMapString? Hmm, that will make
this slower only to deal with extremely rare cases (deseret alphabet). It's also
possible that Windows file system itself does not deal with them 'correctly'(?).


Perhaps, you can just put TODO comment (to add a slower path when there's a
non-BMP character in either name) for now as long as the way German eszett is
dealt with matches what Windows does. 

BTW, the MSDN says that it does not do anything special under Turkish locale
(which is good for us).

[Roland, 2009-09-03 01:54:30]

Well, I followed the article in
http://msdn.microsoft.com/library/en-us/winui/winui/windowsuserinterface/reso...
.

It mentions CharUpper as well as CharUpperBuff and LCMapString as adequate
functions to mimic the OS' behavior when it comes to comparing filenames, and
since I didn't want to allocate and de-allocate 2 extra buffers just for
comparison, I went with CharUpper.

[Roland, 2009-09-04 05:24:38]

A gentle review-wanted-ping... ^_-

Cheers,

Roland

[jungshik at Google, 2009-09-04 21:42:12]

On 2009/09/03 01:54:30, Roland wrote:
> Well, I followed the article in
>
http://msdn.microsoft.com/library/en-us/winui/winui/windowsuserinterface/reso...

Well, that does not guarantee that it does what the file system does. Can you
try experimenting with a filename containing eszett or add it as a part of the
test? 


> .
> 
> It mentions CharUpper as well as CharUpperBuff and LCMapString as adequate
> functions to mimic the OS' behavior when it comes to comparing filenames, and
> since I didn't want to allocate and de-allocate 2 extra buffers just for
> comparison, I went with CharUpper.

[jungshik at Google, 2009-09-04 21:46:40]

http://codereview.chromium.org/149796/diff/5001/5002
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/149796/diff/5001/5002#newcode811
Line 811: TEST_F(FilePathTest, CompareIgnoreCase) {
I guess this and other compare tests should have tests for 'unnormalized' file
names for Mac OS X? OS X file system internally uses NFD, but the name
comparison should succeed regardless of the normalization status of file names.

[Roland, 2009-09-10 10:03:26]

On 2009/09/04 21:42:12, Jungshik Shin wrote:
> Well, that does not guarantee that it does what the file system does. Can you
> try experimenting with a filename containing eszett or add it as a part of the
> test? 

I looked at the Eszett issue: AFAICT, vanilla Vista doesn't even have a glyph
for upper case Eszett. Furthermore, according to the unicode standard,
upper-casing lower case Eszett is "SS", *not* upper case Eszett. 
In any case, Explorer doesn't complain about 3 Files with the same name and
"ss", <lower eszett> and <upper eszett> as Extension.

The unittests I added for eszett reflect this. 


On 2009/09/04 21:46:40, Jungshik Shin wrote:
> http://codereview.chromium.org/149796/diff/5001/5002
> File base/file_path_unittest.cc (right):
> 
> http://codereview.chromium.org/149796/diff/5001/5002#newcode811
> Line 811: TEST_F(FilePathTest, CompareIgnoreCase) {
> I guess this and other compare tests should have tests for 'unnormalized' file
> names for Mac OS X? OS X file system internally uses NFD, but the name
> comparison should succeed regardless of the normalization status of file
names.

Yes, they should, and boy was that not easy to implement! @_@ (the unicode/text
encoding services had a bad tendency to just throw error codes on me, see also
the code comments).

Hopefully the way I implemented it now is water tight (There's also a fall-back
to the previous version in case the system throws an error). I also added unit
tests for this on Mac OSX only.

[Roland, 2009-09-28 03:01:02]

Review requested! ^_-

Cheers,

Roland

[Avi (use Gerrit), 2009-09-30 16:20:58]

I'm OK with the changes in the download manager, but have issues with the
changes to file_path. Make sure to get Mark's OK with those changes before
committing.

http://codereview.chromium.org/149796/diff/11008/10004
File base/file_path.cc (right):

http://codereview.chromium.org/149796/diff/11008/10004#newcode366
Line 366: // case insensitively.
Didn't we answer this question? Our linux guys said yes.

http://codereview.chromium.org/149796/diff/11008/10004#newcode465
Line 465: // cf.
http://developer.apple.com/mac/library/technotes/tn/tn1150.html#UnicodeSubtle...
You're using the TEC? That's incredibly ancient; please don't do that. You can
construct CFStrings from byte encodings using CFString APIs. Please do so.

[Roland, 2009-10-01 03:06:23]

On 2009/09/30 16:20:58, Avi wrote:
> I'm OK with the changes in the download manager, but have issues with the
> changes to file_path. Make sure to get Mark's OK with those changes before
> committing.

I'll try to grab him on IRC, but due to the different time zones this may be
difficult. Could you please also ping him to have a look?

> http://codereview.chromium.org/149796/diff/11008/10004#newcode366
> Line 366: // case insensitively.
> Didn't we answer this question? Our linux guys said yes.

This is more or less just a leftover from previous versions. And I agree that
from a UI perspective it makes sense to handle things case insensitively.
The reason I left it in is because I wonder if there can really never be a
(non-UI) case where we need to do things the file system way. 
(However, since such a need apparently hasn't surfaced so far, I guess I'm just
over-thinking things. ^_^;)
 
> http://codereview.chromium.org/149796/diff/11008/10004#newcode465
> Line 465: // cf.
>
http://developer.apple.com/mac/library/technotes/tn/tn1150.html#UnicodeSubtle...
> You're using the TEC? That's incredibly ancient; please don't do that. You can
> construct CFStrings from byte encodings using CFString APIs. Please do so.

First let me say that I'm far from an expert in OS X programming (just started
with these things, really), so it's not unlikely that I missed some APIs!

The problem here is not just byte encodings, but to get the specific UTF-8
decomposition that HFS uses (and even more so something that actually works, and
not just returns a non-descriptive error... :p). I haven't found a suitable API
for that in CFString. CFStringNormalize comes close, but that only allows the
standard de/composition forms. (Also, while the API may be ancient, the relevant
parts of HFS likely don't change, so it seems to me that that may well be close
to what the file system actually uses.)

[jungshik at Google, 2009-10-01 05:51:13]

On 2009/09/10 10:03:26, Roland wrote:
> On 2009/09/04 21:42:12, Jungshik Shin wrote:
> > Well, that does not guarantee that it does what the file system does. Can
you
> > try experimenting with a filename containing eszett or add it as a part of
the
> > test? 
> 
> I looked at the Eszett issue: AFAICT, vanilla Vista doesn't even have a glyph
> for upper case Eszett. Furthermore, according to the unicode standard,
> upper-casing lower case Eszett is "SS", *not* upper case Eszett. 
> In any case, Explorer doesn't complain about 3 Files with the same name and
> "ss", <lower eszett> and <upper eszett> as Extension.


Gee. You got me wrong (I realized that I was not very explicit about that).
Sure, the uppercase of U+00DF (eszett) is 'SS'. That's why I wondered how using
a single-char-in-single-char-out version of Uppercase could work. 


> The unittests I added for eszett reflect this. 


Did I miss it? You didn't add a test comparing a file with U+00DF with a file
with 'SS'? Whether it's regarded as the same file or not by Windows, you have to
have a test, don't you? 

> 
> On 2009/09/04 21:46:40, Jungshik Shin wrote:
> > http://codereview.chromium.org/149796/diff/5001/5002
> > File base/file_path_unittest.cc (right):
> > 
> > http://codereview.chromium.org/149796/diff/5001/5002#newcode811
> > Line 811: TEST_F(FilePathTest, CompareIgnoreCase) {
> > I guess this and other compare tests should have tests for 'unnormalized'
file
> > names for Mac OS X? OS X file system internally uses NFD, but the name
> > comparison should succeed regardless of the normalization status of file
> names.
> 
> Yes, they should, and boy was that not easy to implement! @_@ (the
unicode/text
> encoding services had a bad tendency to just throw error codes on me, see also
> the code comments).
> 
> Hopefully the way I implemented it now is water tight (There's also a
fall-back
> to the previous version in case the system throws an error). I also added unit
> tests for this on Mac OSX only.

[jungshik at Google, 2009-10-01 05:51:42]

http://codereview.chromium.org/149796/diff/11008/10003
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/149796/diff/11008/10003#newcode846
Line 846: // Note that uc(<lowercase eszett>) => "SS", NOT <uppercase eszett>!
So, does the Windows consider a file with eszett the same as a file with 'SS'? 
Whether it does or not, you need a test for that.

[Roland, 2009-10-06 05:29:41]

On Thu, Oct 1, 2009 at 1:56 PM, Mark Mentovai <mark@chromium.org> wrote:
> Try it out, but I bet CFStringGetFileSystemRepresentation does what you need.

Nah, can't use that - too obvious! >_< (Ok, I feel officially dumb now for
missing that! @_@;)


On 2009/10/01 05:51:13, Jungshik Shin wrote:
> Sure, the uppercase of U+00DF (eszett) is 'SS'. That's why I wondered how
using
> a single-char-in-single-char-out version of Uppercase could work. 

Well, it doesn't convert <eszett> to 'SS', but neither does the Windows file
system, nor does HFS.

On 2009/10/01 05:51:42, Jungshik Shin wrote:
> Whether it does or not, you need a test for that.

Yes, I misunderstood you before. I added the missing test, and it did show up an
issue in my prior implementation: I used CFStringCompare to compare the
decomposed strings, which DOES convert <eszett> -> 'SS'. (It also likes to crash
on the file system representation strings :p)

As this is not what the file system does I instead now added an adapted version
of the 'official' comparison routine (see the same tech note I linked in the
comments in various places). This adds considerably to the code (and there I
thought the Windows version was cumbersome :p), but should be the most correct,
as well as a bit faster.

@Mark: now please don't tell me there's a CFStringCompareFileNamesForDummies
that I could have used instead all along... ^_-

[Mark Mentovai, 2009-10-06 16:10:03]

Fortunately(?) for you, I don't know of any system-provided function that
directly implements FastUnicodeCompare.

http://codereview.chromium.org/149796/diff/17002/18018
File base/file_path.cc (right):

http://codereview.chromium.org/149796/diff/17002/18018#newcode883
Line 883: CFStringRef cfstring1 =
Leak.  Use scoped_cftyperef.  Same for cfstring2.

http://codereview.chromium.org/149796/diff/17002/18018#newcode885
Line 885: (const UInt8*) str1.c_str(),
Prefer C++-style casts.

http://codereview.chromium.org/149796/diff/17002/18018#newcode897
Line 897: CFIndex bufferLength1 =
Style dictates buffer_length, not bufferLength.

http://codereview.chromium.org/149796/diff/17002/18018#newcode902
Line 902: scoped_array<char> buffer1(new char[bufferLength1]);
scoped_array is a pet peeve of mine, because it's really just a vector. 
Provided that length > 0, this is just a std::vector<char>(length).  You can
access the underlying storage with &v[0].

http://codereview.chromium.org/149796/diff/17002/18018#newcode910
Line 910: bufferLength2);
When you write "a a b b c c d d," you've written too much.

You should extract
CFStringCreateWithBytesNoCopy/CFStringGetMaximumSizeOfFileSystemRepresentation/CFStringGetFileSystemRepresentation
into its own static utility function.  It should probably return a StringType.

http://codereview.chromium.org/149796/diff/17002/18018#newcode920
Line 920: //
http://developer.apple.com/mac/library/technotes/tn/tn1150.html#UnicodeSubtle...
You can use #StringComparisonAlgorithm here, it's more specific.

http://codereview.chromium.org/149796/diff/17002/18018#newcode925
Line 925: int index1 = 0;
I recommend:

 - breaking everything from here on down into its own static utility function to
approximate FastUnicodeCompare, and
 - going the rest of the way to convert the function to our style.  The big
differences between our style and Apple's is that we name_variables_like_this
insteadOfThis, and that we don't declare variables until their first use.

http://codereview.chromium.org/149796/diff/17002/18018#newcode968
Line 968: return (codepoint1 < codepoint2) ? -1 : +1;
The + is extraneous and makes me think of floating-point numbers (even though
there's no good reason for it).  ? -1 : 1 is a common enough idiom that I'd just
use that.

http://codereview.chromium.org/149796/diff/17002/18018#newcode971
Line 971: #else
For a long #ifdef like this one, you should put comments on your #elses and
#endifs.

http://codereview.chromium.org/149796/diff/17002/18019
File base/file_path.h (right):

http://codereview.chromium.org/149796/diff/17002/18019#newcode263
Line 263: static int CompareIgnoreCase(const StringType& str1,
I like that you've kept these static, to discourage direct use on FilePaths.

[Roland, 2009-10-07 07:57:32]

Uploaded a new version based on Mark's comments.

On 2009/10/06 16:10:03, Mark Mentovai wrote:
> http://codereview.chromium.org/149796/diff/17002/18018#newcode902
> Line 902: scoped_array<char> buffer1(new char[bufferLength1]);
> scoped_array is a pet peeve of mine, because it's really just a vector. 
> Provided that length > 0, this is just a std::vector<char>(length).  You can
> access the underlying storage with &v[0].

I added 2 helper methods CreateHFSDecomposedForm() and FastUnicodeCompare() as
suggested, which indeed makes things much cleaner.

In my first version of the rewrite CreateHFSDecomposedForm() used a string to
hold the converted result and had CFStringGetFileSystemRepresentation() write
directly into the underlying buffer. However, basic_string stores the current
string length explicitely, so result.length() would still be 0 after the
operation. vector<char> would also suffer from the same problem with size().

Getting a wrong result from length() or size() would probably be confusing and a
potential source of bugs down the road. OTOH I wanted to avoid needlessly
copying the contents of the result buffer within CreateHFSDecomposedForm(), or
for the return value. 

Therefore I rewrote the code to use a simple new[] allocated buffer instead and
return a pointer. However, this brought me back to using scoped_array as the
most convenient solution for life-time management of the result buffer within
CompareIgnoreCase().

If you still have issues with the use of scoped_array, I can of course change
the code to something different (although I believe the current version should
be rather efficient).

[Mark Mentovai, 2009-10-07 14:14:01]

It sounds like you want to use WriteInto from base/string_util.h.  You can call
WriteInto on a std::string with the maximum size, and then resize the string
with the result of strlen when you're done.

[Roland, 2009-10-08 06:32:23]

On 2009/10/07 14:14:01, Mark Mentovai wrote:
> It sounds like you want to use WriteInto from base/string_util.h.  You can
call
> WriteInto on a std::string with the maximum size, and then resize the string
> with the result of strlen when you're done.

Yes, that is similar to my first version. But as the comment to WriteInto points
out, this also adds some basically unnecessary overhead by filling the string
with 0s.

However, it also DOES allow for a cleaner interface, so I uploaded a new version
of the implementation. 

I didn't use WriteInto directly however, since it requires a resize afterwards
that needs explaining anyway, so I just use the 2 lines (reserve + resize) as I
feel that makes it clearer what happens.

Since the interface is now "representable" I also expose FastUnicodeCompare()
and GetHFSDecomposedForm() in the header file.

Finally, I also removed a potential signed shift bug in FastUnicodeCompare() and
cleaned up the variable naming a bit.

[Mark Mentovai, 2009-10-08 13:52:59]

The FilePath stuff LGTM with these changes.

(It looks like a lot of comments but it really isn't.)

http://codereview.chromium.org/149796/diff/24006/24008
File base/file_path.cc (right):

http://codereview.chromium.org/149796/diff/24006/24008#newcode8
Line 8: #include <CoreFoundation/CFString.h>
You don't need this.  CoreServices.h should get you all of the CFString
declarations you need.

http://codereview.chromium.org/149796/diff/24006/24008#newcode22
Line 22: #include "base/scoped_ptr.h"
No longer needed.

http://codereview.chromium.org/149796/diff/24006/24008#newcode454
Line 454: return +1;
+1 comment again here and on line 457

http://codereview.chromium.org/149796/diff/24006/24008#newcode496
Line 496: UInt16 lower_case_table[] = {
const?

http://codereview.chromium.org/149796/diff/24006/24008#newcode497
Line 497: // High-byte indices ( == 0 iff no case mapping and no ignorables )
Make this table conform to our coding style by giving it a 2-space indent
instead of 4.

http://codereview.chromium.org/149796/diff/24006/24008#newcode901
Line 901: // comparison, we just use that vaue and flag it with DCHECK.
vaue = value

http://codereview.chromium.org/149796/diff/24006/24008#newcode916
Line 916: // comparison, we just use that vaue and flag it with DCHECK.
Same.

I feel like this block and the one above ought to be combined, since they're
exactly the same.

http://codereview.chromium.org/149796/diff/24006/24008#newcode983
Line 983: // GetHFSDecomposedForm() returns and empty string in an error case.
and -> an

http://codereview.chromium.org/149796/diff/24006/24009
File base/file_path.h (right):

http://codereview.chromium.org/149796/diff/24006/24009#newcode286
Line 286: static int FastUnicodeCompare(const StringType& string1,
Since you're making this public, you should put HFS in the name.

http://codereview.chromium.org/149796/diff/24006/24007
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/149796/diff/24006/24007#newcode811
Line 811: 
We should add straight ASCII test cases here to see if .txt matches foo.TXT,
foo.TxT, and that sort of thing.

http://codereview.chromium.org/149796/diff/24006/24007#newcode874
Line 874: { { FPL("foo"),                          FPL("bar") },                
+1},
Pluses?

http://codereview.chromium.org/149796/diff/24006/24007#newcode934
Line 934: if (result < 0)
I find this curious.  Doesn't the interface already guarantee -1, 0, 1, and not
just negative, 0, positive?  I guess it doesn't, because in the POSIX-not-Mac
case, you're just using strcasecmp, which doesn't provide this guarantee.  After
reading the Windows and Mac code, this surprised me.  You should:

 - Add a small comment in file_path.h, either "returns -1, 0, or 1" or "returns
a negative integer, 0, or a positive integer", depending on what you choose.
 - Optionally transform negatives to -1 and positives to 1 where you need to use
strcasecmp or other functions that might not have already restricted return
values to these three possibilities.

[Roland, 2009-10-09 05:21:41]

Uploaded a cleaned up patch set with the requested changes:

.) eradicated the remaining '+1's

.) moved reading of the next non-ignorable codepoint to a separate 'inline'
function (within the anonymous namespace).

.) added test cases to the MatchesExtension unit test

.) Changed the code to always return -1, 0 or 1. Added a comment to file_path.h.
Removed the separate check from the unit test.

.) Corrected the rest: const lower_case_table, comment typos, changed method
name to 'HFSFastUnicodeCompare', removed unneeded #include-s

[Mark Mentovai, 2009-10-09 05:33:23]

The FilePath portions LGTM.  I think you've done a great job here.

http://codereview.chromium.org/149796/diff/24035/24038
File base/file_path.h (right):

http://codereview.chromium.org/149796/diff/24035/24038#newcode282
Line 282: static StringType GetHFSDecomposedForm(const FilePath::StringType&
string);
FilePath:: is unnecessary on FilePath::StringType because you're inside |class
FilePath|.

[Mark Mentovai, 2009-10-24 16:01:03]

This hasn't been checked in yet.  Is it still waiting for more reviews?  Are we
going to do this or are we abandoning the approach?

[Roland Steiner, 2009-10-26 04:28:41]

I think I have all the LGMT's necessary, but I was waiting to get my chromium
account set up rather than bothering people to check it in for me.

As I'm a bit busy today otherwise, I intended to check it in tomorrow.


On 2009/10/24 16:01:03, Mark Mentovai wrote:
> This hasn't been checked in yet.  Is it still waiting for more reviews?  Are
we
> going to do this or are we abandoning the approach?

[Roland Steiner, 2009-10-27 05:25:10]

Committed as rev. 30168

[Roland, 2009-10-27 13:40:31]

Unfortunately, there was some bit rot: it seems ICU was yanked from under base/,
so I need one more code review after all, to check that I adapted the code
correctly:

.) file_path.cc: changed U8_NEXT -> CBU8_NEXT
.) changed include file "unicode/utf8.h" -> "base/third_party/icu/icu_utf.h"

[Roland, 2009-10-28 05:41:34]

> On Tue, Oct 27, 2009 at 10:44 PM, Mark Mentovai <mark@chromium.org> wrote:
> LGTM

committed (again :p) as r30323