fileapi: Rename FileSystemMountProvider to FileSystemBackend

fileapi: Rename FileSystemMountProvider to FileSystemBackend

The old name is a misnomer as this class no longer provides mount points!
Per some discussion, FileSystemBackend matches what the class is currently
doing and trying to be.

BUG=257016
TEST=none
[email protected], [email protected], [email protected], [email protected]
# for one-line #include renames in 
chrome/browser/browsing_data/browsing_data_file_system_helper.cc
chrome/browser/extensions/component_loader.cc
chrome/browser/extensions/extension_service.cc

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

[satorux1, 2013-07-04 06:46:36]

kinuko: webkit/... and content/...
vandebo: chrome/browser/media_galleries/...
hidehiko: chrome/browser/chromeos/...

[satorux1, 2013-07-04 06:52:27]

avi: content/public/browser/content_browser_client.h

[kinuko, 2013-07-04 07:12:23]

lgtm, some rants & nits

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_backend.h:130: : public FileSystemBackend {
Ah... I think this one is more like MountPointProvider (or something other than
FileSystemBackend).  Actually this one doesn't need to inherit from
FileSystemBackend at all, and maybe chromeos::FileSystemBackend can just
implement both interface.

And we can likely move this interface out of fileapi layer too...

well, they can be done in a separate patch anyway.

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_context.cc:107:
ScopedVector<FileSystemBackend> additional_providers,
Can you run s/additional_providers/additional_backends/ on this file too?

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_context.cc:132:
RegisterMountPointProvider(isolated_provider_.get());
Hmm... ok we'll do these remaining cleanup in our side.

[hidehiko, 2013-07-04 07:14:40]

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api.cc:370:
fileapi::ExternalFileSystemBackend* external_provider =
s/provider/backend/ ?

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:222:
fileapi::ExternalFileSystemBackend* provider =
s/provider/backend/ ?

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:543:
fileapi::ExternalFileSystemBackend* provider =
ditto.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:190:
fileapi::ExternalFileSystemBackend* external_provider =
s/provider/backend/?

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:632:
fileapi::ExternalFileSystemBackend* external_provider_handler =
ditto.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:259:
fileapi::ExternalFileSystemBackend* external_provider =
s/provider/backend/ ?

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:573:
fileapi::ExternalFileSystemBackend* provider =
ditto.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
File chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:36:
TEST(CrosMountPointProviderTest, DefaultMountPoints) {
Probably, we want to rename CrosMountProviderTest to
ChromeOSFileSystemBackendTest or something? (ChromeOS prefix probably may be
needed as test fixture name is global scope).

Ditto for below tests.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:41:
chromeos::FileSystemBackend provider(
s/provider/backend/

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:71:
chromeos::FileSystemBackend provider(
ditto.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:112:
chromeos::FileSystemBackend provider(
ditto.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:207:
chromeos::FileSystemBackend provider(storage_policy,
ditto.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1138: if (filesystem_context &&
filesystem_context->external_provider()) {
Do we want to rename external_provider to external_backend or something along
with the class name renaming?

[satorux1, 2013-07-04 08:11:32]

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api.cc:370:
fileapi::ExternalFileSystemBackend* external_provider =
On 2013/07/04 07:14:40, hidehiko wrote:
> s/provider/backend/ ?

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:222:
fileapi::ExternalFileSystemBackend* provider =
On 2013/07/04 07:14:40, hidehiko wrote:
> s/provider/backend/ ?

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:543:
fileapi::ExternalFileSystemBackend* provider =
On 2013/07/04 07:14:40, hidehiko wrote:
> ditto.

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:190:
fileapi::ExternalFileSystemBackend* external_provider =
On 2013/07/04 07:14:40, hidehiko wrote:
> s/provider/backend/?

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:632:
fileapi::ExternalFileSystemBackend* external_provider_handler =
On 2013/07/04 07:14:40, hidehiko wrote:
> ditto.

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:259:
fileapi::ExternalFileSystemBackend* external_provider =
On 2013/07/04 07:14:40, hidehiko wrote:
> s/provider/backend/ ?

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:573:
fileapi::ExternalFileSystemBackend* provider =
On 2013/07/04 07:14:40, hidehiko wrote:
> ditto.

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
File chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc
(right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:36:
TEST(CrosMountPointProviderTest, DefaultMountPoints) {
On 2013/07/04 07:14:40, hidehiko wrote:
> Probably, we want to rename CrosMountProviderTest to
> ChromeOSFileSystemBackendTest or something? (ChromeOS prefix probably may be
> needed as test fixture name is global scope).
> 
> Ditto for below tests.

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:41:
chromeos::FileSystemBackend provider(
On 2013/07/04 07:14:40, hidehiko wrote:
> s/provider/backend/

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:71:
chromeos::FileSystemBackend provider(
On 2013/07/04 07:14:40, hidehiko wrote:
> ditto.

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:112:
chromeos::FileSystemBackend provider(
On 2013/07/04 07:14:40, hidehiko wrote:
> ditto.

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/cros_mount_point_provider_unittest.cc:207:
chromeos::FileSystemBackend provider(storage_policy,
On 2013/07/04 07:14:40, hidehiko wrote:
> ditto.

Done.

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/18344013/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1138: if (filesystem_context &&
filesystem_context->external_provider()) {
On 2013/07/04 07:14:40, hidehiko wrote:
> Do we want to rename external_provider to external_backend or something along
> with the class name renaming?

This will probably be gone as part of crbug.com/257279, so i'd keep this as-is

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_backend.h:130: : public FileSystemBackend {
On 2013/07/04 07:12:23, kinuko wrote:
> Ah... I think this one is more like MountPointProvider (or something other
than
> FileSystemBackend).  Actually this one doesn't need to inherit from
> FileSystemBackend at all, and maybe chromeos::FileSystemBackend can just
> implement both interface.
> 
> And we can likely move this interface out of fileapi layer too...
> 
> well, they can be done in a separate patch anyway.

Added a TODO here with the bug you filed.

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_context.cc:107:
ScopedVector<FileSystemBackend> additional_providers,
On 2013/07/04 07:12:23, kinuko wrote:
> Can you run s/additional_providers/additional_backends/ on this file too?

Done.

https://codereview.chromium.org/18344013/diff/4001/webkit/browser/fileapi/fil...
webkit/browser/fileapi/file_system_context.cc:132:
RegisterMountPointProvider(isolated_provider_.get());
On 2013/07/04 07:12:23, kinuko wrote:
> Hmm... ok we'll do these remaining cleanup in our side.

Forgot about them. Renamed.

[hidehiko, 2013-07-04 08:21:12]

The code looks good, but the compile is failing on trybot?

Two nitpicks:

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.h:248:
ScopedVector<fileapi::FileSystemBackend>*
nit: Can be fit in a line?

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chromeos/f...
File chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc (right):

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chromeos/f...
chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc:200:
TEST(FileSystemBackend, GetVirtualPathConflictWithSystemPoints) {
nit: ChromeOSFileSystemBackendTest, too?

[kinuko, 2013-07-04 08:44:35]

I added some more comments for consistency in file_system_context.*. Thanks for
doing this, or feel free to leave some of them as is but with TODO.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:173: FileSystemBackend*
mount_point_provider =
maybe just do s/mount_point_provider/file_system_backend/ on this file

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:83: // Unless a MountPointProvider
is overridden in CreateFileSystemOperation,
MountPointProvider -> FileSystemBackend

hmm... I guess this should be

MountPointProvider is overridden in ->
FileSystemBackend overrides it in

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:94: // ChromeOS (i.e. platforms
that don't use external_mount_point_provider).
Can you remove the sentence after 'On ChromeOS, it will be passed...' (the end
of line 91 - line 94) while you're there?  This sentence doesn't look to apply
any more after hidehiko's refactoring

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:96: // |additional_providers| are
added to the internal provider map
additional_providers -> additional_backends
provider map -> backends map

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:105:
ScopedVector<FileSystemBackend> additional_providers,
additional_providers -> additional_backends

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:134: // Returns the mount point
provider instance for the given |type|.
Returns the file system backend

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:137: FileSystemBackend*
GetMountPointProvider(
Can we rename this to GetMountPointProvider -> GetFileSystemBackend ?

(7 callsites)

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:155: // calling
GetMountPointProvider(kFileSystemTypeExternal).
GetMountPointProvider -> GetFileSystemBackend

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:192: // MountPointProvider for the
URL and call the provider's CreateFileReader.
MountPointProvider -> FileSystemBackend
provider's -> backend's

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:193: // The resolved
MountPointProvider could perform further specialization
ditto (or maybe remove line 193-194, it feels a bit verbose)

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:240: MountPointProviderMap;
MountPointProviderMap -> FileSystemBackendMap

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:245: // For sandbox_provider().
sandbox_provider -> sandbox_backend

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:261: // depending on the filesystem
type pointed by the |url|.
MountPointProvider -> FileSystemBackend (line 258, 260)
provider -> backend (line 258)

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:277: // For initial provider_map
construction. This must be called only from
provider_map -> backend_map

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:279: void
RegisterMountPointProvider(FileSystemBackend* provider);
RegisterBackend

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:282: SandboxFileSystemBackend*
sandbox_provider() const {
Can we rename this too to sandbox_backend()?

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:297: // Registered mount point
providers.
mount point providers -> file system backends

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:301: // of the providers are held
by additional_providers_ or other scoped_ptr
s/providers/backends/g

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:303: MountPointProviderMap
provider_map_;
FileSystemBackendMap backend_map_;

[satorux1, 2013-07-05 04:47:57]

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.h:248:
ScopedVector<fileapi::FileSystemBackend>*
On 2013/07/04 08:21:12, hidehiko wrote:
> nit: Can be fit in a line?

Done.

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chromeos/f...
File chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc (right):

https://codereview.chromium.org/18344013/diff/23001/chrome/browser/chromeos/f...
chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc:200:
TEST(FileSystemBackend, GetVirtualPathConflictWithSystemPoints) {
On 2013/07/04 08:21:12, hidehiko wrote:
> nit: ChromeOSFileSystemBackendTest, too?

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:173: FileSystemBackend*
mount_point_provider =
On 2013/07/04 08:44:35, kinuko wrote:
> maybe just do s/mount_point_provider/file_system_backend/ on this file

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:83: // Unless a MountPointProvider
is overridden in CreateFileSystemOperation,
On 2013/07/04 08:44:35, kinuko wrote:
> MountPointProvider -> FileSystemBackend
> 
> hmm... I guess this should be
> 
> MountPointProvider is overridden in ->
> FileSystemBackend overrides it in

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:94: // ChromeOS (i.e. platforms
that don't use external_mount_point_provider).
On 2013/07/04 08:44:35, kinuko wrote:
> Can you remove the sentence after 'On ChromeOS, it will be passed...' (the end
> of line 91 - line 94) while you're there?  This sentence doesn't look to apply
> any more after hidehiko's refactoring

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:96: // |additional_providers| are
added to the internal provider map
On 2013/07/04 08:44:35, kinuko wrote:
> additional_providers -> additional_backends
> provider map -> backends map

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:105:
ScopedVector<FileSystemBackend> additional_providers,
On 2013/07/04 08:44:35, kinuko wrote:
> additional_providers -> additional_backends

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:134: // Returns the mount point
provider instance for the given |type|.
On 2013/07/04 08:44:35, kinuko wrote:
> Returns the file system backend

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:137: FileSystemBackend*
GetMountPointProvider(
On 2013/07/04 08:44:35, kinuko wrote:
> Can we rename this to GetMountPointProvider -> GetFileSystemBackend ?
> 
> (7 callsites)

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:155: // calling
GetMountPointProvider(kFileSystemTypeExternal).
On 2013/07/04 08:44:35, kinuko wrote:
> GetMountPointProvider -> GetFileSystemBackend

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:192: // MountPointProvider for the
URL and call the provider's CreateFileReader.
On 2013/07/04 08:44:35, kinuko wrote:
> MountPointProvider -> FileSystemBackend
> provider's -> backend's

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:193: // The resolved
MountPointProvider could perform further specialization
On 2013/07/04 08:44:35, kinuko wrote:
> ditto (or maybe remove line 193-194, it feels a bit verbose)

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:240: MountPointProviderMap;
On 2013/07/04 08:44:35, kinuko wrote:
> MountPointProviderMap -> FileSystemBackendMap

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:245: // For sandbox_provider().
On 2013/07/04 08:44:35, kinuko wrote:
> sandbox_provider -> sandbox_backend

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:261: // depending on the filesystem
type pointed by the |url|.
On 2013/07/04 08:44:35, kinuko wrote:
> MountPointProvider -> FileSystemBackend (line 258, 260)
> provider -> backend (line 258)

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:277: // For initial provider_map
construction. This must be called only from
On 2013/07/04 08:44:35, kinuko wrote:
> provider_map -> backend_map

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:279: void
RegisterMountPointProvider(FileSystemBackend* provider);
On 2013/07/04 08:44:35, kinuko wrote:
> RegisterBackend

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:282: SandboxFileSystemBackend*
sandbox_provider() const {
On 2013/07/04 08:44:35, kinuko wrote:
> Can we rename this too to sandbox_backend()?

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:297: // Registered mount point
providers.
On 2013/07/04 08:44:35, kinuko wrote:
> mount point providers -> file system backends

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:301: // of the providers are held
by additional_providers_ or other scoped_ptr
On 2013/07/04 08:44:35, kinuko wrote:
> s/providers/backends/g

Done.

https://codereview.chromium.org/18344013/diff/23001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:303: MountPointProviderMap
provider_map_;
On 2013/07/04 08:44:35, kinuko wrote:
> FileSystemBackendMap backend_map_;

Done.

[satorux1, 2013-07-05 05:02:27]

+thestig for minor changes in chrome/browser:

    chrome/browser/browsing_data/browsing_data_file_system_helper.cc
    chrome/browser/extensions/component_loader.cc
    chrome/browser/extensions/extension_service.cc

please also feel free to review chrome/browser/media_galleries/... if vandebo is
ooo.

+joi for content/public/browser/content_browser_client.h as avi is likely ooo
(holiday in the us)

[kinuko, 2013-07-05 05:11:10]

lgtm, much more complete. Some nits for remnants in comments

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc (right):

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc:277:
GetRootPathCreateAndExamineWithNewProvider) {
NewProvider -> NewBackend

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/syncable_file_system_operation.h (right):

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/syncable_file_system_operation.h:90: // Only
MountPointProviders can create a new operation directly.
MountPointProviders -> FileSystemBackends

[hidehiko, 2013-07-05 05:18:06]

lgtm

https://codereview.chromium.org/18344013/diff/41001/chrome/browser/chromeos/f...
File chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc (right):

https://codereview.chromium.org/18344013/diff/41001/chrome/browser/chromeos/f...
chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc:203:
TEST(FileSystemBackendTest, GetVirtualPathConflictWithSystemPoints) {
nit: Not Done yet?

[satorux1, 2013-07-05 05:25:34]

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc (right):

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc:277:
GetRootPathCreateAndExamineWithNewProvider) {
On 2013/07/05 05:11:11, kinuko wrote:
> NewProvider -> NewBackend

Done.

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/syncable_file_system_operation.h (right):

https://codereview.chromium.org/18344013/diff/36001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/syncable_file_system_operation.h:90: // Only
MountPointProviders can create a new operation directly.
On 2013/07/05 05:11:11, kinuko wrote:
> MountPointProviders -> FileSystemBackends

Done.

https://codereview.chromium.org/18344013/diff/41001/chrome/browser/chromeos/f...
File chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc (right):

https://codereview.chromium.org/18344013/diff/41001/chrome/browser/chromeos/f...
chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc:203:
TEST(FileSystemBackendTest, GetVirtualPathConflictWithSystemPoints) {
On 2013/07/05 05:18:07, hidehiko wrote:
> nit: Not Done yet?

Done.

[Jói, 2013-07-05 13:10:42]

LGTM for //content/public.

[vandebo (ex-Chrome), 2013-07-08 16:04:33]

LGTM

FYI: thestig is OOO this week.

https://codereview.chromium.org/18344013/diff/61001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18344013/diff/61001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:160:
class TestMediaFileSystemBackend
nit: one line.

https://codereview.chromium.org/18344013/diff/61001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:164:
PicasaFileUtil* picasa_file_util)
nit: indent

https://codereview.chromium.org/18344013/diff/61001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:166:
profile_path,
nit: will fit after (

[satorux1, 2013-07-09 01:22:55]

Committed patchset #9 manually as r210480 (presubmit successful).