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

Issue 11773018: Fix creating target paths in file_util_posix CopyDirectory. (Closed)

Created:
7 years, 11 months ago by aedla
Modified:
7 years, 10 months ago
Reviewers:
Tom Sepez, brettw
CC:
chromium-reviews, erikwright+watch_chromium.org, sail+watch_chromium.org
Visibility:
Public.

Description

Fix creating target paths in file_util_posix CopyDirectory. BUG=167840 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176659 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179590

Patch Set 1 #

Patch Set 2 : add unittest #

Total comments: 1

Patch Set 3 : comment change #

Patch Set 4 : modify CopyDirectoryWithTrailingSeparators to POSIX-only #

Patch Set 5 : support trailing separators in windows file_util::CopyDirectory #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -12 lines) Patch
M base/file_util_posix.cc View 1 2 3 4 1 chunk +8 lines, -8 lines 0 comments Download
M base/file_util_unittest.cc View 1 2 3 4 1 chunk +37 lines, -0 lines 0 comments Download
M base/file_util_win.cc View 1 2 3 4 2 chunks +8 lines, -4 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
aedla
Tom, could you take a look?
7 years, 11 months ago (2013-01-07 10:23:16 UTC) #1
Tom Sepez
We should have brett take a look at this.
7 years, 11 months ago (2013-01-07 18:03:36 UTC) #2
brettw
Sorry, I don't have enough context. The CL description doesn't have any useful info and ...
7 years, 11 months ago (2013-01-07 18:36:31 UTC) #3
Tom Sepez
Sorry about that, Brett. My bad. I've CC'd you on bug 167840 so you should ...
7 years, 11 months ago (2013-01-07 18:40:48 UTC) #4
brettw
I spent a couple of minutes looking at this function and I don't understand how ...
7 years, 11 months ago (2013-01-07 18:49:35 UTC) #5
aedla
Might be helpful for review that webkit/fileapi/file_util_helper.cc CopyOrMoveDirectory() has similar code: while (!(src_file_path_each = file_enum->Next()).empty()) ...
7 years, 11 months ago (2013-01-07 18:55:21 UTC) #6
aedla
I think we could test that CopyDirectory() works if from_path has lots of trailing separators.
7 years, 11 months ago (2013-01-07 19:40:48 UTC) #7
aedla
Added a test, which demonstrates the problem with trailing separators in from_path. For the unfixed ...
7 years, 11 months ago (2013-01-08 13:39:32 UTC) #8
brettw
lgtm https://codereview.chromium.org/11773018/diff/2002/base/file_util_posix.cc File base/file_util_posix.cc (right): https://codereview.chromium.org/11773018/diff/2002/base/file_util_posix.cc#newcode347 base/file_util_posix.cc:347: // the suffix after from_path onto to_path to ...
7 years, 11 months ago (2013-01-09 20:27:48 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aedla@chromium.org/11773018/12001
7 years, 11 months ago (2013-01-14 09:17:37 UTC) #10
commit-bot: I haz the power
Change committed as 176659
7 years, 11 months ago (2013-01-14 12:29:46 UTC) #11
aedla
Commit was reverted. Turns out SHFileOperation on WinXP doesn't like trailing separators. So I think ...
7 years, 11 months ago (2013-01-15 14:43:33 UTC) #12
aedla
Modified the test to be POSIX-only.
7 years, 11 months ago (2013-01-21 08:42:27 UTC) #13
aedla
PTAL @tsepez, brettw: Looks like I can't commit via CQ anymore. Would either of you ...
7 years, 11 months ago (2013-01-21 08:51:17 UTC) #14
aedla
Ah nevermind, I could just reopen the issue.
7 years, 11 months ago (2013-01-21 08:54:41 UTC) #15
brettw
lgtm
7 years, 10 months ago (2013-01-30 00:42:15 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aedla@chromium.org/11773018/31001
7 years, 10 months ago (2013-01-30 08:12:06 UTC) #17
commit-bot: I haz the power
7 years, 10 months ago (2013-01-30 11:38:03 UTC) #18
Message was sent while issue was closed.
Change committed as 179590

Powered by Google App Engine
This is Rietveld 408576698