Cleanup: return value and null-callback fixups on AsyncFileUtil interface

Cleanup: return value and null-callback fixups on AsyncFileUtil interface

- Change return value from bool to void, as handling an error in
  two ways (return value and callback value) adds unnecessary
  complexity, and it's not handled at all in the current code.
- Add a restriction to callbcak: from now callback should never
  be NULL. Also dropping callback.is_null() checks in the implementation.

BUG=241701
TEST=existing tests should pass
R=hidehiko@chromium.org, tzik@chromium.org, vandebo@chromium.org

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

[kinuko, 2013-07-05 05:00:19]

Minor cleanups.

tzik: can you review changes in general
vandebo, hidehiko: can you take a quick look if they look ok from the
implementor's pov?

[hidehiko, 2013-07-05 05:03:23]

lgtm from my side.

[tzik, 2013-07-05 05:07:16]

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:260:
DCHECK(success);
I think this may be hit on shutdown sequence.
Could we just ignore the return value here and elsewhere?

[tzik, 2013-07-05 05:20:14]

lgtm

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:260:
DCHECK(success);
On 2013/07/05 05:07:17, tzik wrote:
> I think this may be hit on shutdown sequence.
> Could we just ignore the return value here and elsewhere?

Per offline discussion, this should be always true even on shutdown sequence on
production code, since this function should be called before channel shutdown
and the task_runner should be alive at the point.

[vandebo (ex-Chrome), 2013-07-08 15:32:34]

LGTM

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:260:
DCHECK(success);
On 2013/07/05 05:20:14, tzik wrote:
> On 2013/07/05 05:07:17, tzik wrote:
> > I think this may be hit on shutdown sequence.
> > Could we just ignore the return value here and elsewhere?
> 
> Per offline discussion, this should be always true even on shutdown sequence
on
> production code, since this function should be called before channel shutdown
> and the task_runner should be alive at the point.

Please add a comment to the FileSystemOperationContext constructor noting that
|task_runner| must outlive the IO thread and that task runners based on the
BrowserThreadImpl worker pool do.

[kinuko, 2013-07-09 04:17:41]

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc
(right):

https://codereview.chromium.org/18580012/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:260:
DCHECK(success);
On 2013/07/08 15:32:34, vandebo wrote:
> On 2013/07/05 05:20:14, tzik wrote:
> > On 2013/07/05 05:07:17, tzik wrote:
> > > I think this may be hit on shutdown sequence.
> > > Could we just ignore the return value here and elsewhere?
> > 
> > Per offline discussion, this should be always true even on shutdown sequence
> on
> > production code, since this function should be called before channel
shutdown
> > and the task_runner should be alive at the point.
> 
> Please add a comment to the FileSystemOperationContext constructor noting that
> |task_runner| must outlive the IO thread and that task runners based on the
> BrowserThreadImpl worker pool do.

Done.

[kinuko, 2013-07-09 08:01:33]

Committed patchset #2 manually as r210510 (presubmit successful).