Issue 8999017: Add CreateFileSystemOperation() method to FileSystemContext

Issue 8999017: Add CreateFileSystemOperation() method to FileSystemContext (Closed)

Created:
9 years ago by kinuko

Modified:
8 years, 11 months ago

Reviewers:
ericu, satorux1

CC:
chromium-reviews, jstritar+watch_chromium.org, jam, achuith+watch_chromium.org, mihaip+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, Aaron Boodman, rginda+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., pam+watch_chromium.org, kinuko+watch, tzik, kinaba

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Add CreateFileSystemOperation() method to FileSystemContext BUG=none TEST=no functional changes Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117830

Patch Set 1 : '' #

Patch Set 2 : rebased from patch 9016020 #

Total comments: 6

Patch Set 3 : addressed comments #

Patch Set 4 : rebase #

Patch Set 5 : fixed null FSO handling #

Patch Set 6 : clang build fix #

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+235 lines, -87 lines)			Patch
M	content/browser/file_system/file_system_dispatcher_host.h	View		2 chunks	+6 lines, -4 lines	0 comments	Download
M	content/browser/file_system/file_system_dispatcher_host.cc	View	1 2 3 4	8 chunks	+43 lines, -24 lines	0 comments	Download
M	webkit/chromeos/fileapi/cros_mount_point_provider.h	View	1 2 3	1 chunk	+7 lines, -0 lines	0 comments	Download
M	webkit/chromeos/fileapi/cros_mount_point_provider.cc	View	1 2 3	2 chunks	+13 lines, -0 lines	0 comments	Download
M	webkit/fileapi/file_system_context.h	View	1 2 3	3 chunks	+12 lines, -0 lines	0 comments	Download
M	webkit/fileapi/file_system_context.cc	View	1 2 3	3 chunks	+20 lines, -0 lines	0 comments	Download
M	webkit/fileapi/file_system_dir_url_request_job.h	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	webkit/fileapi/file_system_dir_url_request_job.cc	View	1 2 3 4	3 chunks	+17 lines, -7 lines	0 comments	Download
M	webkit/fileapi/file_system_mount_point_provider.h	View	1 2 3	2 chunks	+21 lines, -0 lines	0 comments	Download
M	webkit/fileapi/file_system_operation.h	View	1 2 3	3 chunks	+11 lines, -3 lines	0 comments	Download
M	webkit/fileapi/file_system_operation.cc	View	1 2 3	3 chunks	+17 lines, -13 lines	0 comments	Download
M	webkit/fileapi/file_system_operation_interface.h	View	1 2 3 4 5	2 chunks	+6 lines, -0 lines	0 comments	Download
M	webkit/fileapi/file_system_url_request_job.cc	View	1 2 3 4	2 chunks	+13 lines, -5 lines	0 comments	Download
M	webkit/fileapi/sandbox_mount_point_provider.h	View	1 2 3	1 chunk	+7 lines, -0 lines	0 comments	Download
M	webkit/fileapi/sandbox_mount_point_provider.cc	View	1 2 3	2 chunks	+12 lines, -0 lines	0 comments	Download
M	webkit/tools/test_shell/simple_file_system.h	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	webkit/tools/test_shell/simple_file_system.cc	View	1 2 3	4 chunks	+17 lines, -18 lines	0 comments	Download
M	webkit/tools/test_shell/simple_file_writer.cc	View	1 2 3	5 chunks	+8 lines, -8 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

satorux1

http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/file_system_dispatcher_host.cc File content/browser/file_system/file_system_dispatcher_host.cc (right): http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/file_system_dispatcher_host.cc#newcode316 content/browser/file_system/file_system_dispatcher_host.cc:316: FileSystemOperation* operation = static_cast<FileSystemOperation*>( Is this a down cast? ...

8 years, 12 months ago (2011-12-27 20:55:33 UTC) #1

ericu

+1 to Satoru's comments, but overall it looks good. http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/file_system_dispatcher_host.cc File content/browser/file_system/file_system_dispatcher_host.cc (right): http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/file_system_dispatcher_host.cc#newcode180 content/browser/file_system/file_system_dispatcher_host.cc:180: ...

8 years, 11 months ago (2012-01-10 00:37:04 UTC) #2

kinuko

Thanks for your comments (and sorry for the delay). Updated the change. http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/file_system_dispatcher_host.cc File content/browser/file_system/file_system_dispatcher_host.cc ...

8 years, 11 months ago (2012-01-13 08:28:35 UTC) #3

Thanks for your comments (and sorry for the delay). Updated the change.

http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/...
File content/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/...
content/browser/file_system/file_system_dispatcher_host.cc:180: int request_id,
const GURL& src_path, const GURL& dest_path) {
On 2012/01/10 00:37:04, ericu wrote:
> Where are we going to handle issues involving moves/copies that involve two
> different types of FileSystemOperations?

As we talked before I'm putting it off for now.  I'm vaguely assuming that we
will need to do something like 1) change the per-operation CallbackDispatcher
architecture (e.g. replacing it with per-method regular callbacks) and 2)
implement cross-FSO operations above the individual FSO layer using lower FSO
routines like readDirectory, createOrOpenFile, createDirectory and etc.

(Filed a bug: http://codereview.chromium.org/8999017/)

http://codereview.chromium.org/8999017/diff/8001/content/browser/file_system/...
content/browser/file_system/file_system_dispatcher_host.cc:316:
FileSystemOperation* operation = static_cast<FileSystemOperation*>(
On 2011/12/27 20:55:34, satorux1 wrote:
> Is this a down cast? Can we avoid it? Otherwise, it'd be nice to add some
> comment about why we need to do this here.

Once we support asynchronous uploading for arbitrary filesystem files we should
be able to get rid of this dirty hack.  I've added comments about that and added
AsFileSystemOperation() virtual method for explicit downcasting.

http://codereview.chromium.org/8999017/diff/8001/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/8999017/diff/8001/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider.h:78: virtual
FileSystemOperationInterface* CreateFileSystemOperation(
On 2011/12/27 20:55:34, satorux1 wrote:
> There is also FileSystemContext::CreateFileSystemOperation() with a similar
> signature. Why do we need the two functions?
> 
> Please add comment to the two functions explaining the differences, and why we
> need them.

Added some comments why we have both.  We can get rid of
FileSystemContext::CreateFileSystemOperation as it's just a dispatching layer
but it'll add 4-5 more lines at each callsite, and I thought it might not be
really desirable either.  (Open to objections though)

LGTM too.

Expand Messages | Collapse Messages