Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(984)

Issue 869823003: Update ReplaceIllegalCharactersInPath to handle quirks in HFS+ and VFS (Closed)

Created:
5 years, 11 months ago by asanka
Modified:
5 years, 10 months ago
Reviewers:
jungshik at Google
CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Update ReplaceIllegalCharactersInPath to handle quirks in HFS+ and VFAT This change: - Re-introduces U+200C and U+200D as illegal characters since these are ignored on HFS+ and can interfere with filename sanitization. All code points in the Cf general category are now considered illegal in a filename. - Leading and trailing WSpace and '.' characters are now considered illegal. - Due to being confused for short names on VFAT filesystems, the tilde ('~') is now considered illegal. - Prior to this change, only ASCII whitespace were trimmed from filenames on Mac OSX. All UTF-8 encoded WSpace characters are now handled on Mac OSX. - Instead of trimming leading and trailing whitespace, they are now replaced by the replacement character. Trimming could cause a previously hidden extension or basename to be exposed. BUG=444102 BUG=446538 Committed: https://crrev.com/92a354fca8d118ad1be38612f85aa71a9a25303b Cr-Commit-Position: refs/heads/master@{#314041}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 2

Patch Set 4 : Remove unnecessary #if def #

Unified diffs Side-by-side diffs Delta from patch set Stats (+695 lines, -757 lines) Patch
M base/i18n/file_util_icu.h View 1 1 chunk +15 lines, -5 lines 0 comments Download
M base/i18n/file_util_icu.cc View 1 2 3 3 chunks +45 lines, -35 lines 0 comments Download
M base/i18n/file_util_icu_unittest.cc View 3 chunks +52 lines, -28 lines 0 comments Download
M content/browser/download/save_package.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M content/browser/download/save_package_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M net/base/filename_util_unittest.cc View 1 2 2 chunks +581 lines, -687 lines 0 comments Download

Messages

Total messages: 9 (2 generated)
asanka
PTAL?
5 years, 11 months ago (2015-01-23 21:33:47 UTC) #2
jungshik at Google
On 2015/01/23 21:33:47, asanka wrote: > PTAL? Sorry for the delay (after an office move, ...
5 years, 10 months ago (2015-01-28 21:43:24 UTC) #3
jungshik at Google
LGTM (sorry for the delay). https://codereview.chromium.org/869823003/diff/40001/base/i18n/file_util_icu.cc File base/i18n/file_util_icu.cc (right): https://codereview.chromium.org/869823003/diff/40001/base/i18n/file_util_icu.cc#newcode87 base/i18n/file_util_icu.cc:87: UNICODE_STRING_SIMPLE("[[:WSpace:][.]]"), ends_status)); You don't ...
5 years, 10 months ago (2015-01-30 23:13:25 UTC) #4
asanka
Thanks! https://codereview.chromium.org/869823003/diff/40001/base/i18n/file_util_icu.cc File base/i18n/file_util_icu.cc (right): https://codereview.chromium.org/869823003/diff/40001/base/i18n/file_util_icu.cc#newcode87 base/i18n/file_util_icu.cc:87: UNICODE_STRING_SIMPLE("[[:WSpace:][.]]"), ends_status)); On 2015/01/30 at 23:13:25, Jungshik at ...
5 years, 10 months ago (2015-01-30 23:36:48 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/869823003/60001
5 years, 10 months ago (2015-01-30 23:37:33 UTC) #7
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 10 months ago (2015-01-31 00:38:30 UTC) #8
commit-bot: I haz the power
5 years, 10 months ago (2015-01-31 00:39:19 UTC) #9
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/92a354fca8d118ad1be38612f85aa71a9a25303b
Cr-Commit-Position: refs/heads/master@{#314041}

Powered by Google App Engine
This is Rietveld 408576698