Added PlatformFileError param to FileSystemMountPointProvider::CreateFileSystemOperation function.

Added PlatformFileError param to FileSystemMountPointProvider::CreateFileSystemOperation function.

BUG=141617
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156042

[kinuko, 2012-09-11 05:01:39]

lgtm, thanks!!

https://chromiumcodereview.appspot.com/10914188/diff/1/webkit/chromeos/fileap...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10914188/diff/1/webkit/chromeos/fileap...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:229: } else {
nit: no need of else (since we're returning in if)

[kmadhusu, 2012-09-11 15:11:42]

https://chromiumcodereview.appspot.com/10914188/diff/1/webkit/chromeos/fileap...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10914188/diff/1/webkit/chromeos/fileap...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:229: } else {
On 2012/09/11 05:01:39, kinuko wrote:
> nit: no need of else (since we're returning in if)

You had a TODO to handle the not-found case gracefully. Else part corresponds to
that TODO. I am going to commit with this change. If you still feel that this
else part is not required, I can remove it in a separate CL.

[commit-bot: I haz the power, 2012-09-11 15:15:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10914188/1

[kinuko, 2012-09-11 15:32:35]

I'm not suggesting we should ignore the else case... I meant we can omit
the else line but still return null when the previous if wasn't met.
On Sep 12, 2012 12:11 AM, <kmadhusu@chromium.org> wrote:

> Reviewers: kinuko,
>
>
> https://chromiumcodereview.**appspot.com/10914188/diff/1/**
>
webkit/chromeos/fileapi/cros_**mount_point_provider.cc<https://chromiumcodereview.appspot.com/10914188/diff/1/webkit/chromeos/fileapi/cros_mount_point_provider.cc>
> File webkit/chromeos/fileapi/cros_**mount_point_provider.cc (right):
>
> https://chromiumcodereview.**appspot.com/10914188/diff/1/**
>
webkit/chromeos/fileapi/cros_**mount_point_provider.cc#**newcode229<https://chromiumcodereview.appspot.com/10914188/diff/1/webkit/chromeos/fileapi/cros_mount_point_provider.cc#newcode229>
> webkit/chromeos/fileapi/cros_**mount_point_provider.cc:229: } else {
> On 2012/09/11 05:01:39, kinuko wrote:
>
>> nit: no need of else (since we're returning in if)
>>
>
> You had a TODO to handle the not-found case gracefully. Else part
> corresponds to that TODO. I am going to commit with this change. If you
> still feel that this else part is not required, I can remove it in a
> separate CL.
>
> Description:
> Added PlatformFileError param to
> FileSystemMountPointProvider::**CreateFileSystemOperation function.
>
> BUG=141617
> TEST=none
>
> Please review this at
https://chromiumcodereview.**appspot.com/10914188/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M webkit/chromeos/fileapi/cros_**mount_point_provider.h
>   M webkit/chromeos/fileapi/cros_**mount_point_provider.cc
>   M webkit/fileapi/file_system_**context.cc
>   M webkit/fileapi/file_system_**mount_point_provider.h
>   M webkit/fileapi/isolated_mount_**point_provider.h
>   M webkit/fileapi/isolated_mount_**point_provider.cc
>   M webkit/fileapi/sandbox_mount_**point_provider.h
>   M webkit/fileapi/sandbox_mount_**point_provider.cc
>   M webkit/fileapi/test_mount_**point_provider.h
>   M webkit/fileapi/test_mount_**point_provider.cc
>
>
> Index: webkit/chromeos/fileapi/cros_**mount_point_provider.cc
> diff --git a/webkit/chromeos/fileapi/**cros_mount_point_provider.cc
> b/webkit/chromeos/fileapi/**cros_mount_point_provider.cc
> index 79642c4164597b2424345dedc015d8**662e952e6d..**
> 067b310a5ef04ef8f600cd00a89f8f**5fb2498b69 100644
> --- a/webkit/chromeos/fileapi/**cros_mount_point_provider.cc
> +++ b/webkit/chromeos/fileapi/**cros_mount_point_provider.cc
> @@ -218,15 +218,17 @@ FilePath CrosMountPointProvider::**
> GetPathForPermissionsCheck(
>
>  fileapi::FileSystemOperation* CrosMountPointProvider::**
> CreateFileSystemOperation(
>      const fileapi::FileSystemURL& url,
> -    fileapi::FileSystemContext* context) const {
> +    fileapi::FileSystemContext* context,
> +    base::PlatformFileError* error_code) const {
>    if (url.type() == fileapi::kFileSystemTypeDrive) {
>      base::AutoLock locker(mount_point_map_lock_);
>      RemoteProxyMap::const_iterator found = remote_proxy_map_.find(
>          url.filesystem_id());
> -    // TODO(kinuko): we should handle not-found case gracefully.
> -    // http://crbug.com/141617
>      if (found != remote_proxy_map_.end()) {
>        return new chromeos::**RemoteFileSystemOperation(**found->second);
> +    } else {
> +      *error_code = base::PLATFORM_FILE_ERROR_NOT_**FOUND;
> +      return NULL;
>      }
>    }
>
> Index: webkit/chromeos/fileapi/cros_**mount_point_provider.h
> diff --git a/webkit/chromeos/fileapi/**cros_mount_point_provider.h
> b/webkit/chromeos/fileapi/**cros_mount_point_provider.h
> index 2c2edfd36e862184aa8a17c114e368**b76c419d6d..**
> 6678f841dba7fd313d1505325c9c0d**0534e0e91e 100644
> --- a/webkit/chromeos/fileapi/**cros_mount_point_provider.h
> +++ b/webkit/chromeos/fileapi/**cros_mount_point_provider.h
> @@ -62,7 +62,8 @@ class FILEAPI_EXPORT CrosMountPointProvider
>        const OVERRIDE;
>    virtual fileapi::FileSystemOperation* CreateFileSystemOperation(
>        const fileapi::FileSystemURL& url,
> -      fileapi::FileSystemContext* context) const OVERRIDE;
> +      fileapi::FileSystemContext* context,
> +      base::PlatformFileError* error_code) const OVERRIDE;
>    virtual webkit_blob::FileStreamReader* CreateFileStreamReader(
>        const fileapi::FileSystemURL& path,
>        int64 offset,
> Index: webkit/fileapi/file_system_**context.cc
> diff --git a/webkit/fileapi/file_system_**context.cc
> b/webkit/fileapi/file_system_**context.cc
> index 45615152c80da2b3852f35a8d8db43**fba5836bbe..**
> 7bf6399cd3171f5d0e792ed41b340f**baf07b043a 100644
> --- a/webkit/fileapi/file_system_**context.cc
> +++ b/webkit/fileapi/file_system_**context.cc
> @@ -216,9 +216,13 @@ FileSystemOperation* FileSystemContext::**
> CreateFileSystemOperation(
>      return NULL;
>    }
>
> +  PlatformFileError fs_error = base::PLATFORM_FILE_OK;
> +  FileSystemOperation* operation =
> +      mount_point_provider->**CreateFileSystemOperation(url, this,
> &fs_error);
> +
>    if (error_code)
> -    *error_code = base::PLATFORM_FILE_OK;
> -  return mount_point_provider->**CreateFileSystemOperation(url, this);
> +    *error_code = fs_error;
> +  return operation;
>  }
>
>  webkit_blob::FileStreamReader* FileSystemContext::**
> CreateFileStreamReader(
> Index: webkit/fileapi/file_system_**mount_point_provider.h
> diff --git a/webkit/fileapi/file_system_**mount_point_provider.h
> b/webkit/fileapi/file_system_**mount_point_provider.h
> index 5ac21efe1f9b08eb498e5d54710512**9bb112b25b..**
> 1ee7ab467ef0fc0ba4adec306f30b1**c602c68d07 100644
> --- a/webkit/fileapi/file_system_**mount_point_provider.h
> +++ b/webkit/fileapi/file_system_**mount_point_provider.h
> @@ -77,12 +77,14 @@ class FILEAPI_EXPORT FileSystemMountPointProvider {
>
>    // Returns a new instance of the specialized FileSystemOperation for
> this
>    // mount point based on the given triplet of |origin_url|,
> |file_system_type|
> -  // and |virtual_path|.
> +  // and |virtual_path|. On failure to create a file system operation, set
> +  // |error_code| correspondingly.
>    // This method is usually dispatched by
>    // FileSystemContext::**CreateFileSystemOperation.
>    virtual FileSystemOperation* CreateFileSystemOperation(
>        const FileSystemURL& url,
> -      FileSystemContext* context) const = 0;
> +      FileSystemContext* context,
> +      base::PlatformFileError* error_code) const = 0;
>
>    // Creates a new file stream reader for a given filesystem URL |url|
> with an
>    // offset |offset|.
> Index: webkit/fileapi/isolated_mount_**point_provider.cc
> diff --git a/webkit/fileapi/isolated_**mount_point_provider.cc
> b/webkit/fileapi/isolated_**mount_point_provider.cc
> index 47d6311dead3d391a7afd341c334c9**d545a6e3e9..**
> af99b0aaca51a5e76a4aceb134eb14**779d0a9906 100644
> --- a/webkit/fileapi/isolated_**mount_point_provider.cc
> +++ b/webkit/fileapi/isolated_**mount_point_provider.cc
> @@ -126,7 +126,8 @@ FilePath IsolatedMountPointProvider::**
> GetPathForPermissionsCheck(
>
>  FileSystemOperation* IsolatedMountPointProvider::**
> CreateFileSystemOperation(
>      const FileSystemURL& url,
> -    FileSystemContext* context) const {
> +    FileSystemContext* context,
> +    base::PlatformFileError* error_code) const {
>    scoped_ptr<**FileSystemOperationContext> operation_context(
>        new FileSystemOperationContext(**context));
>    if (url.type() == kFileSystemTypeNativeMedia ||
> Index: webkit/fileapi/isolated_mount_**point_provider.h
> diff --git a/webkit/fileapi/isolated_**mount_point_provider.h
> b/webkit/fileapi/isolated_**mount_point_provider.h
> index ee1af87eb54dce59b4827ed0cfcc64**288432e962..**
> aeca147744165e6ea6d8eb9f23b22b**c0bc5e5c7b 100644
> --- a/webkit/fileapi/isolated_**mount_point_provider.h
> +++ b/webkit/fileapi/isolated_**mount_point_provider.h
> @@ -47,7 +47,8 @@ class IsolatedMountPointProvider : public
> FileSystemMountPointProvider {
>        const OVERRIDE;
>    virtual FileSystemOperation* CreateFileSystemOperation(
>        const FileSystemURL& url,
> -      FileSystemContext* context) const OVERRIDE;
> +      FileSystemContext* context,
> +      base::PlatformFileError* error_code) const OVERRIDE;
>    virtual webkit_blob::FileStreamReader* CreateFileStreamReader(
>        const FileSystemURL& url,
>        int64 offset,
> Index: webkit/fileapi/sandbox_mount_**point_provider.cc
> diff --git a/webkit/fileapi/sandbox_**mount_point_provider.cc
> b/webkit/fileapi/sandbox_**mount_point_provider.cc
> index 2f3614def796eb2024001c06f42a6f**12d2802d4d..**
> c02204fa76b6d17c5b0b0e55b5831c**6bf8699ba4 100644
> --- a/webkit/fileapi/sandbox_**mount_point_provider.cc
> +++ b/webkit/fileapi/sandbox_**mount_point_provider.cc
> @@ -448,7 +448,8 @@ FilePath SandboxMountPointProvider::**
> GetPathForPermissionsCheck(
>
>  FileSystemOperation* SandboxMountPointProvider::**
> CreateFileSystemOperation(
>      const FileSystemURL& url,
> -    FileSystemContext* context) const {
> +    FileSystemContext* context,
> +    base::PlatformFileError* error_code) const {
>    scoped_ptr<**FileSystemOperationContext> operation_context(
>        new FileSystemOperationContext(**context));
>
> Index: webkit/fileapi/sandbox_mount_**point_provider.h
> diff --git a/webkit/fileapi/sandbox_**mount_point_provider.h
> b/webkit/fileapi/sandbox_**mount_point_provider.h
> index 74e4d7576c60bfe438c544c7b4f380**6b54dd587c..**
> 3b7f9ca281ac5f59325f2b54617fc5**496b983658 100644
> --- a/webkit/fileapi/sandbox_**mount_point_provider.h
> +++ b/webkit/fileapi/sandbox_**mount_point_provider.h
> @@ -96,7 +96,8 @@ class FILEAPI_EXPORT SandboxMountPointProvider
>        const OVERRIDE;
>    virtual FileSystemOperation* CreateFileSystemOperation(
>        const FileSystemURL& url,
> -      FileSystemContext* context) const OVERRIDE;
> +      FileSystemContext* context,
> +      base::PlatformFileError* error_code) const OVERRIDE;
>    virtual webkit_blob::FileStreamReader* CreateFileStreamReader(
>        const FileSystemURL& url,
>        int64 offset,
> Index: webkit/fileapi/test_mount_**point_provider.cc
> diff --git a/webkit/fileapi/test_mount_**point_provider.cc
> b/webkit/fileapi/test_mount_**point_provider.cc
> index 58f4a472577dfdb94590a748bc4dc0**7d9811ce0a..**
> 33ee811dea33fd791c3934af32d8c4**f39239c1c6 100644
> --- a/webkit/fileapi/test_mount_**point_provider.cc
> +++ b/webkit/fileapi/test_mount_**point_provider.cc
> @@ -123,7 +123,8 @@ FilePath TestMountPointProvider::**
> GetPathForPermissionsCheck(
>
>  FileSystemOperation* TestMountPointProvider::**CreateFileSystemOperation(
>      const FileSystemURL& url,
> -    FileSystemContext* context) const {
> +    FileSystemContext* context,
> +    base::PlatformFileError* error_code) const {
>    scoped_ptr<**FileSystemOperationContext> operation_context(
>        new FileSystemOperationContext(**context));
>    operation_context->set_update_**observers(observers_);
> Index: webkit/fileapi/test_mount_**point_provider.h
> diff --git a/webkit/fileapi/test_mount_**point_provider.h
> b/webkit/fileapi/test_mount_**point_provider.h
> index 6f9b04cbda96c9f4da017c6ce0527e**a4bc018bc1..**
> 7d52e48c413f7043605ebd4c5ab2c2**48361e483a 100644
> --- a/webkit/fileapi/test_mount_**point_provider.h
> +++ b/webkit/fileapi/test_mount_**point_provider.h
> @@ -53,7 +53,8 @@ class FILEAPI_EXPORT_PRIVATE TestMountPointProvider
>        const OVERRIDE;
>    virtual FileSystemOperation* CreateFileSystemOperation(
>        const FileSystemURL& url,
> -      FileSystemContext* context) const OVERRIDE;
> +      FileSystemContext* context,
> +      base::PlatformFileError* error_code) const OVERRIDE;
>    virtual webkit_blob::FileStreamReader* CreateFileStreamReader(
>        const FileSystemURL& url,
>        int64 offset,
>
>
>

[kmadhusu, 2012-09-11 15:38:54]

I understand what you meant. I would like to leave it as it is.

Thanks.

[commit-bot: I haz the power, 2012-09-11 17:38:15]

Change committed as 156042