Update callers of CreateFileSystemOperation so more detailed error codes can be returned. Where app…

Update callers of CreateFileSystemOperation so more detailed error codes can be returned. Where applicable, convert net errors to base platform errors.

BUG=141617
TBR=tony@chromium.org

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

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

[tzik, 2012-09-06 05:59:07]

http://codereview.chromium.org/10920087/diff/1/webkit/fileapi/file_system_con...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10920087/diff/1/webkit/fileapi/file_system_con...
webkit/fileapi/file_system_context.cc:198: }
Could you insert below?
if (error_code)
  *error_code = base::PLATFORM_FILE_OK;

[calvinlo, 2012-09-06 07:14:35]

On 2012/09/06 05:59:07, tzik wrote:
>
http://codereview.chromium.org/10920087/diff/1/webkit/fileapi/file_system_con...
> File webkit/fileapi/file_system_context.cc (right):
> 
>
http://codereview.chromium.org/10920087/diff/1/webkit/fileapi/file_system_con...
> webkit/fileapi/file_system_context.cc:198: }
> Could you insert below?
> if (error_code)
>   *error_code = base::PLATFORM_FILE_OK;

Thanks for finding the bug. I think this will work now, rerunning the tests.
Will reupload CL soon.

[tzik, 2012-09-06 08:13:32]

lgtm

[kinuko, 2012-09-06 09:15:10]

http://codereview.chromium.org/10920087/diff/6001/content/browser/fileapi/fil...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10920087/diff/6001/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:502:
operation->SyncGetPlatformPath(url, platform_path);
Can you change this to check the return value as well?

if (operation)
  operation->SyncGet...

http://codereview.chromium.org/10920087/diff/6001/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:801: DCHECK(operation);
This needs to check the return value as well. Yes this means we need to change
all callsites as well... maybe we can do like following here:

  if (error != base::PLATFORM_FILE_OK) {
    Send(new FileSystemMsg_DidFail(request_id, error));
    return NULL;
  }
  operations_.AddWithID(...)
  ...

And in the callsite:

  FileSystemOperation* operation = GetNewOperation();
  if (operation)
    operation->Foo(...);

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:208: 
nit: please remove this empty line

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_dir_url_request_job.cc (right):

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:83: FileSystemOperation*
operation = GetNewOperation();
Please let GetNewOperation return error code too

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:86: net::ERR_INVALID_URL));
...and return the error code here (via PlatformFileErrorToNetError)

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:127:
GetNewOperation()->ReadDirectory(
ditto.  We need to fail (with NotifyDone) if the operation is null.

[calvinlo, 2012-09-06 11:19:32]

Finished all fixes from Kinuko's review

http://codereview.chromium.org/10920087/diff/6001/content/browser/fileapi/fil...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10920087/diff/6001/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:502:
operation->SyncGetPlatformPath(url, platform_path);
On 2012/09/06 09:15:10, kinuko wrote:
> Can you change this to check the return value as well?
> 
> if (operation)
>   operation->SyncGet...

Done.

http://codereview.chromium.org/10920087/diff/6001/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:801: DCHECK(operation);
On 2012/09/06 09:15:10, kinuko wrote:
> This needs to check the return value as well. Yes this means we need to change
> all callsites as well... maybe we can do like following here:
> 
>   if (error != base::PLATFORM_FILE_OK) {
>     Send(new FileSystemMsg_DidFail(request_id, error));
>     return NULL;
>   }
>   operations_.AddWithID(...)
>   ...
> 
> And in the callsite:
> 
>   FileSystemOperation* operation = GetNewOperation();
>   if (operation)
>     operation->Foo(...);

Done, although I did it as:
if (!operation)
  return;
operation->Foo(...);

is that ok or is your method the preferred one.

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:208: 
On 2012/09/06 09:15:10, kinuko wrote:
> nit: please remove this empty line

Done.

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_dir_url_request_job.cc (right):

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:83: FileSystemOperation*
operation = GetNewOperation();
On 2012/09/06 09:15:10, kinuko wrote:
> Please let GetNewOperation return error code too

Done.

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:86: net::ERR_INVALID_URL));
On 2012/09/06 09:15:10, kinuko wrote:
> ...and return the error code here (via PlatformFileErrorToNetError)

Done.

http://codereview.chromium.org/10920087/diff/6001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:127:
GetNewOperation()->ReadDirectory(
On 2012/09/06 09:15:10, kinuko wrote:
> ditto.  We need to fail (with NotifyDone) if the operation is null.

Done.

[kinuko, 2012-09-06 11:45:54]

lgtm, thanks!

[calvinlo, 2012-09-06 12:15:39]

On 2012/09/06 11:45:54, kinuko wrote:
> lgtm, thanks!

Hello Stig, can you please take a look at the change in
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc. 

Hi William, I also made a small change to net/base/net_errors.cc that requires
approval. 

Thanks guys.

[Lei Zhang, 2012-09-06 18:23:21]

chrome/ bit lgtm

[willchan no longer on Chromium, 2012-09-06 18:33:37]

lgtm

http://codereview.chromium.org/10920087/diff/2017/net/base/net_errors.cc
File net/base/net_errors.cc (right):

http://codereview.chromium.org/10920087/diff/2017/net/base/net_errors.cc#newc...
net/base/net_errors.cc:54: case base::PLATFORM_FILE_ERROR_INVALID_URL:
I'm surprised by this PLATFORM_FILE_ERROR_INVALID_URL, looks like a layering
violation to me. That said, I don't believe that was introduced here, so this
change is fine.

[commit-bot: I haz the power, 2012-09-07 04:17:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10920087/2017

[commit-bot: I haz the power, 2012-09-07 04:17:14]

Presubmit check for 10920087-2017 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 1.7s to calculate.

[calvinlo, 2012-09-07 04:23:22]

Added TBR=
tony@, can you please check webkit/tools/test_shell/*

[commit-bot: I haz the power, 2012-09-07 04:23:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10920087/2017

[commit-bot: I haz the power, 2012-09-07 13:05:39]

Change committed as 155377

[commit-bot: I haz the power, 2012-09-10 03:50:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10920087/4003

[commit-bot: I haz the power, 2012-09-10 04:20:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10920087/4003

[commit-bot: I haz the power, 2012-09-10 04:22:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10920087/7031

[commit-bot: I haz the power, 2012-09-10 06:24:48]

Change committed as 155671