Normalize download file name on chromeos

Normalize download file name on chromeos

ChromeOS file manager cannot handle non NFC encoded utf8 file names (because of
file path normalization done in webkit layer), so let's ensure we normalize
downloaded file names.

BUG=chromium-os:26028
TEST=downloaded file with nfd encoded names and made sure they can be handled by file manager

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129128

[asanka, 2012-03-26 20:01:08]

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
chrome/browser/download/chrome_download_manager_delegate.cc:436:
download_util::NormalizeFileNameOnChromeOS(&generated_name);
why not do this in download_util::GenerateFileNameFromRequest() or better yet,
net::GetSuggestedFilename() ?

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
File chrome/browser/download/download_util.h (right):

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
chrome/browser/download/download_util.h:55: void
NormalizeFileNameOnChromeOS(FilePath* file_name);
If this is a common thing that you'll need to do on CrOS, shouldn't this be
somewhere that's not download specific?

[tbarzic, 2012-03-26 20:45:06]

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
File chrome/browser/download/download_util.h (right):

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
chrome/browser/download/download_util.h:55: void
NormalizeFileNameOnChromeOS(FilePath* file_name);
On 2012/03/26 20:01:08, asanka wrote:
> If this is a common thing that you'll need to do on CrOS, shouldn't this be
> somewhere that's not download specific?

I'm not sure if we'll need to do this anywhere else (at least for now).
do you have any suggestions where to put this?

[tbarzic, 2012-03-26 20:59:50]

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
chrome/browser/download/chrome_download_manager_delegate.cc:436:
download_util::NormalizeFileNameOnChromeOS(&generated_name);
On 2012/03/26 20:01:08, asanka wrote:
> why not do this in download_util::GenerateFileNameFromRequest() or better yet,
> net::GetSuggestedFilename() ?

Done.

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
File chrome/browser/download/download_util.h (right):

https://chromiumcodereview.appspot.com/9854011/diff/1006/chrome/browser/downl...
chrome/browser/download/download_util.h:55: void
NormalizeFileNameOnChromeOS(FilePath* file_name);
On 2012/03/26 20:45:06, tbarzic wrote:
> On 2012/03/26 20:01:08, asanka wrote:
> > If this is a common thing that you'll need to do on CrOS, shouldn't this be
> > somewhere that's not download specific?
> 
> I'm not sure if we'll need to do this anywhere else (at least for now).
> do you have any suggestions where to put this?

Done.

[jungshik at Google, 2012-03-26 21:52:46]

LGTM !

[asanka, 2012-03-26 22:08:35]

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
File base/i18n/file_util_icu.cc (right):

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
base/i18n/file_util_icu.cc:203: void NormalizeFileNameOnChromeOS(FilePath*
file_name) {
Could you also toss in a quick unit test in file_util_icu_unittest.cc?

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
File base/i18n/file_util_icu.h (right):

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
base/i18n/file_util_icu.h:38: // If running on ChromeOS, normalizes file name.
// Calculates the canonical file-system representation of |file_name|. Modifies
|file_name| in-place.

The impression I get is that this should only modify the
|file_name->BaseName()|. If so, that should be documented and the implementation
should follow suit.

Given that there's a file_util::NormalizeFilePath(), you might consider calling
this NormalizeFilenameEncoding() or something similar. Let it be a no-op for now
on non-ChromeOS platforms.

[tbarzic, 2012-03-26 22:57:26]

asanka, ptal

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
File base/i18n/file_util_icu.cc (right):

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
base/i18n/file_util_icu.cc:203: void NormalizeFileNameOnChromeOS(FilePath*
file_name) {
On 2012/03/26 22:08:35, asanka wrote:
> Could you also toss in a quick unit test in file_util_icu_unittest.cc?

Done.

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
File base/i18n/file_util_icu.h (right):

https://chromiumcodereview.appspot.com/9854011/diff/11004/base/i18n/file_util...
base/i18n/file_util_icu.h:38: // If running on ChromeOS, normalizes file name.
On 2012/03/26 22:08:35, asanka wrote:
> // Calculates the canonical file-system representation of |file_name|.
Modifies
> |file_name| in-place.
> 
> The impression I get is that this should only modify the
> |file_name->BaseName()|. If so, that should be documented and the
implementation
> should follow suit.
> 
> Given that there's a file_util::NormalizeFilePath(), you might consider
calling
> this NormalizeFilenameEncoding() or something similar. Let it be a no-op for
now
> on non-ChromeOS platforms.

Done.

[asanka, 2012-03-27 01:30:34]

LGTM. Thanks!

https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
File base/i18n/file_util_icu.cc (right):

https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
base/i18n/file_util_icu.cc:211: #endif
Minor nit: Perhaps throw in a NOTIMPLEMENTED() here for other platforms. But I'm
fine with this either way.

https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
File base/i18n/file_util_icu_unittest.cc (right):

https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
base/i18n/file_util_icu_unittest.cc:97: TEST_F(FileUtilICUTest,
NormalizeFileNameEncoding) {
Thanks for adding this!

https://chromiumcodereview.appspot.com/9854011/diff/10011/chrome/browser/down...
File chrome/browser/download/download_file_picker_chromeos.cc (right):

https://chromiumcodereview.appspot.com/9854011/diff/10011/chrome/browser/down...
chrome/browser/download/download_file_picker_chromeos.cc:20: #include
"content/public/browser/download_manager.h"
Warning: These changes might conflict with
http://codereview.chromium.org/9809011/ . You'll want to co-ordinate with
achuith.

Also, his CL adds a save_packge_file_picker_chromeos.cc that might also need the
NormalizeFileNameEncoding() treatment.

[tbarzic, 2012-03-27 01:32:01]

Noted, thanks for review!

On 2012/03/27 01:30:34, asanka wrote:
> LGTM. Thanks!
> 
>
https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
> File base/i18n/file_util_icu.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
> base/i18n/file_util_icu.cc:211: #endif
> Minor nit: Perhaps throw in a NOTIMPLEMENTED() here for other platforms. But
I'm
> fine with this either way.
> 
>
https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
> File base/i18n/file_util_icu_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9854011/diff/10011/base/i18n/file_util...
> base/i18n/file_util_icu_unittest.cc:97: TEST_F(FileUtilICUTest,
> NormalizeFileNameEncoding) {
> Thanks for adding this!
> 
>
https://chromiumcodereview.appspot.com/9854011/diff/10011/chrome/browser/down...
> File chrome/browser/download/download_file_picker_chromeos.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9854011/diff/10011/chrome/browser/down...
> chrome/browser/download/download_file_picker_chromeos.cc:20: #include
> "content/public/browser/download_manager.h"
> Warning: These changes might conflict with
> http://codereview.chromium.org/9809011/ . You'll want to co-ordinate with
> achuith.
> 
> Also, his CL adds a save_packge_file_picker_chromeos.cc that might also need
the
> NormalizeFileNameEncoding() treatment.