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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 197233008: Add a parameter to FileStreamWriter::CreateForLocalFile to allow creating new (Closed)

Created:
6 years, 9 months ago by ericu
Modified:
6 years, 9 months ago
Reviewers:
kinaba, tzik
CC:
chromium-reviews, vandebo (ex-Chrome), jam, nkostylev+watch_chromium.org, tzik, Lei Zhang, tfarina, Greg Billock, nhiroki, joi+watch-content_chromium.org, tommycli, darin-cc_chromium.org, oshima+watch_chromium.org, kinuko+watch, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, cmumford, jsbell
Visibility:
Public.

Description

Add a parameter to FileStreamWriter::CreateForLocalFile to allow creating new files as well as writing to existing files. See https://codereview.chromium.org/18023022/ for context; this is needed by upcoming IDB Blob support. Tzik, please do primary review for filesystem; Kinaba, please review as ChromeOS owner. BUG=108012 R=kinaba,tzik Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258120

Patch Set 1 #

Patch Set 2 : Add missing file. #

Patch Set 3 : Deal with a TODO #

Patch Set 4 : ChromeOS build fixes #

Patch Set 5 : Name change to avoid windows macro definition #

Patch Set 6 : Merged out #

Total comments: 2

Patch Set 7 : Convert to switch to ensure enum coverage #

Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -31 lines) Patch
M chrome/browser/chromeos/drive/fileapi/webkit_file_stream_writer_impl.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/chromeos/fileapi/file_system_backend.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/media_galleries/fileapi/media_file_system_backend.cc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/fileapi/copy_or_move_operation_delegate_unittest.cc View 1 2 3 4 6 chunks +9 lines, -17 lines 0 comments Download
M webkit/browser/fileapi/file_stream_writer.h View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
M webkit/browser/fileapi/isolated_file_system_backend.cc View 1 2 3 4 1 chunk +6 lines, -2 lines 0 comments Download
M webkit/browser/fileapi/local_file_stream_writer.h View 2 chunks +3 lines, -1 line 0 comments Download
M webkit/browser/fileapi/local_file_stream_writer.cc View 1 2 3 4 5 6 3 chunks +21 lines, -4 lines 0 comments Download
M webkit/browser/fileapi/local_file_stream_writer_unittest.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M webkit/browser/fileapi/sandbox_file_stream_writer.cc View 1 2 3 4 1 chunk +4 lines, -2 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
ericu
Kinaba, Tzik, could you please take a look at this small change? Thanks, Eric
6 years, 9 months ago (2014-03-18 23:38:41 UTC) #1
kinaba
c/b/cros/* lgtm
6 years, 9 months ago (2014-03-19 01:25:21 UTC) #2
tzik
lgtm https://codereview.chromium.org/197233008/diff/100001/webkit/browser/fileapi/local_file_stream_writer.cc File webkit/browser/fileapi/local_file_stream_writer.cc (right): https://codereview.chromium.org/197233008/diff/100001/webkit/browser/fileapi/local_file_stream_writer.cc#newcode105 webkit/browser/fileapi/local_file_stream_writer.cc:105: Could you add DCHECK if open_or_create_ == OPEN_EXISTING_FILE ...
6 years, 9 months ago (2014-03-19 03:34:54 UTC) #3
ericu
https://codereview.chromium.org/197233008/diff/100001/webkit/browser/fileapi/local_file_stream_writer.cc File webkit/browser/fileapi/local_file_stream_writer.cc (right): https://codereview.chromium.org/197233008/diff/100001/webkit/browser/fileapi/local_file_stream_writer.cc#newcode105 webkit/browser/fileapi/local_file_stream_writer.cc:105: On 2014/03/19 03:34:55, tzik wrote: > Could you add ...
6 years, 9 months ago (2014-03-19 17:49:26 UTC) #4
ericu
The CQ bit was checked by ericu@chromium.org
6 years, 9 months ago (2014-03-19 19:16:02 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/197233008/120001
6 years, 9 months ago (2014-03-19 19:17:09 UTC) #6
commit-bot: I haz the power
Change committed as 258120
6 years, 9 months ago (2014-03-19 22:03:22 UTC) #7
pneubeck (no reviews)
6 years, 9 months ago (2014-03-20 08:40:22 UTC) #8
Message was sent while issue was closed. 
A revert of this CL has been created in
https://codereview.chromium.org/206073006/ by pneubeck@chromium.org.

The reason for reverting is: Best guess that this broke
SyncFileSystemApiTest.WriteFileThenGetUsage
on XP Tests(1).

http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds...

If not, we can just re-revert or reland it after a few cycles..

Powered by Google App Engine
This is Rietveld 408576698