Allow extension extensions to be case-insensitive.

Allow extension extensions to be case-insensitive.

BUG=none
TEST=try to load an extension with a capital letter or two in the extension's extension. It should load.


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

[Erik does not do reviews, 2009-06-23 23:17:52]

http://codereview.chromium.org/147017/diff/1003/1004
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/147017/diff/1003/1004#newcode1510
Line 1510: return StartsWith(extension,
the file extension should always be ASCII safe, right?  Can't we use
LowerCaseEqualsASCII on all platforms?

http://codereview.chromium.org/147017/diff/1003/1005
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/147017/diff/1003/1005#newcode520
Line 520: bool IsExtension(const FilePath& file_name);
Could you move this into Extension?

[Avi (use Gerrit), 2009-06-24 17:33:36]

http://codereview.chromium.org/147017/diff/1003/1004
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/147017/diff/1003/1004#newcode1510
Line 1510: return StartsWith(extension,
On 2009/06/23 23:17:52, Erik Kay wrote:
> the file extension should always be ASCII safe, right?  Can't we use
> LowerCaseEqualsASCII on all platforms?

Because the second parameter of LCEA is always char*, which kEFE isn't on
Windows :( Then again, if we're ifdefing windows we could use a win32
comparison...

http://codereview.chromium.org/147017/diff/1003/1005
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/147017/diff/1003/1005#newcode520
Line 520: bool IsExtension(const FilePath& file_name);
On 2009/06/23 23:17:52, Erik Kay wrote:
> Could you move this into Extension?

Any class in particular?

[Erik does not do reviews, 2009-06-24 17:42:55]

http://codereview.chromium.org/147017/diff/1003/1004
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/147017/diff/1003/1004#newcode1510
Line 1510: return StartsWith(extension,
On 2009/06/24 17:33:36, Avi wrote:
> On 2009/06/23 23:17:52, Erik Kay wrote:
> > the file extension should always be ASCII safe, right?  Can't we use
> > LowerCaseEqualsASCII on all platforms?
> 
> Because the second parameter of LCEA is always char*, which kEFE isn't on
> Windows :( Then again, if we're ifdefing windows we could use a win32
> comparison...

two other options that I like better than this:
- make an ASCII version of this constant in addition to the current FPL
verssion.
- add a FilePath method that does case insensitive extension comparisons like
this so that we don't have to have this kind of code in multiple files.

http://codereview.chromium.org/147017/diff/1003/1005
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/147017/diff/1003/1005#newcode520
Line 520: bool IsExtension(const FilePath& file_name);
On 2009/06/24 17:33:36, Avi wrote:
> On 2009/06/23 23:17:52, Erik Kay wrote:
> > Could you move this into Extension?
> 
> Any class in particular?

chrome/common/extensions/extension.h

[Avi (use Gerrit), 2009-06-25 13:27:21]

erikkay for themes
mark for filepath

[Erik does not do reviews, 2009-06-25 14:08:43]

LGTM

[Mark Mentovai, 2009-06-25 14:22:08]

http://codereview.chromium.org/147017/diff/7/1009
File base/file_path.cc (right):

http://codereview.chromium.org/147017/diff/7/1009#newcode218
Line 218: // Drop the leading period.
I would prefer that we did the FilePathy thing - Extension returns a leading
period, MatchesExtension should accept a leading period.  Here in this function,
we should accept an argument that's either an empty string (to test for
extensionlessness) or a string that begins with a period.

http://codereview.chromium.org/147017/diff/7/1009#newcode225
Line 225: #if defined(OS_WIN)
We have CaseInsensitiveCompare from string_util, it should work.  Why not...

return std::equal(extension.begin(), extension.end(), current.extension.begin(),
CaseInsensitiveCompare<FilePath::CharType>());

[Avi (use Gerrit), 2009-06-25 16:44:14]

http://codereview.chromium.org/147017/diff/7/1009
File base/file_path.cc (right):

http://codereview.chromium.org/147017/diff/7/1009#newcode218
Line 218: // Drop the leading period.
The problem there is getting a constant to pass in that has the leading period.
They're all defined without it. And you can't do string joins in code if your
constant is C++.

http://codereview.chromium.org/147017/diff/7/1009#newcode225
Line 225: #if defined(OS_WIN)
On 2009/06/25 14:22:08, Mark Mentovai wrote:
> We have CaseInsensitiveCompare from string_util, it should work.  Why not...
> 
> return std::equal(extension.begin(), extension.end(),
current.extension.begin(),
> CaseInsensitiveCompare<FilePath::CharType>());

Done.

[Avi (use Gerrit), 2009-06-29 18:09:09]

Mark, ping?

[Avi (use Gerrit), 2009-06-29 19:16:54]

One last look, then.

[Mark Mentovai, 2009-06-29 19:20:17]

LGTM.

Could I trouble you for a quick test in file_path_unittest.cc?  (Feel free to do
it in a follow-up CL.)

[Avi (use Gerrit), 2009-06-29 20:06:19]

On 2009/06/29 19:20:17, Mark Mentovai wrote:
> LGTM.
> 
> Could I trouble you for a quick test in file_path_unittest.cc?  (Feel free to
do
> it in a follow-up CL.)

No trouble, and I'll even do it right in this CL. For your consideration.

[Mark Mentovai, 2009-06-29 20:29:22]

lgtm