Pepper: Use an adaptor to invoke FileSystemOperationRunner::OpenFile

Pepper: Use an adaptor to invoke FileSystemOperationRunner::OpenFile

PepperFileIOHost keeps a weak pointer of FileSystemOperationRunner so when
it goes away it cannot delete the runner as expected by the fileapi
implementation.

This CL adds an adapter class in the middle so that when OpenFile completes
after PepperFileIOHost destruction, the file is bounced back to another
thread to be closed.

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

[rvargas (doing something else), 2014-06-03 17:54:46]

PTAL

[bbudge, 2014-06-03 18:21:52]

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:110:
base::Bind(&FileCloser, Passed(&file)));
Shouldn't we call 'on_close_callback' on this path too?

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:273:
base::Bind(&OpenFileAdapter::DidOpenFile, base::Owned(open_adapter)));
Rather than define a new class, could you could define another static function
that takes the host and callback as params and pass that to the
FileSystemOperationRunner? Like this:

void DidOpenFile(base::weak_ptr<PepperFileIOHost> host,
                 fileapi::FileSystemOperation::OpenFileCallback cb,
                 base::File file,
                 const base::Closure& on_close_callback) {
  if (host) {
    cb.Run(file.Pass(), on_close_callback);
  } else {
    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                            base::Bind(&FileCloser,
                                       Passed(&file)));
  }
}

[rvargas (doing something else), 2014-06-03 23:04:54]

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:110:
base::Bind(&FileCloser, Passed(&file)));
On 2014/06/03 18:21:53, bbudge wrote:
> Shouldn't we call 'on_close_callback' on this path too?

That is an interesting question.

FileSystemOperationRunner::OpenFile() calls
FileSystemOperationImpl::OpenFile() which relies on
AsyncFileUtilAdapter::CreateOrOpen() which uses
RunCreateOrOpenCallback() to return a NULL on_close_callback.

On the other hand, as I said somewhere else, FileSystemOperationRunner has no
documentation about threading at all, so it is not obvious where that callback
should be called if it were not NULL. The normal pattern would be to call it on
the same thread that issued OpenFile(), and that means _this_ thread, which
would result in the DCHECK firing again.

Maybe DCHECKing that the callback is null at this point is good enough?

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:273:
base::Bind(&OpenFileAdapter::DidOpenFile, base::Owned(open_adapter)));
On 2014/06/03 18:21:53, bbudge wrote:
> Rather than define a new class, could you could define another static function
> that takes the host and callback as params and pass that to the
> FileSystemOperationRunner? Like this:
> 
> void DidOpenFile(base::weak_ptr<PepperFileIOHost> host,
>                  fileapi::FileSystemOperation::OpenFileCallback cb,
>                  base::File file,
>                  const base::Closure& on_close_callback) {
>   if (host) {
>     cb.Run(file.Pass(), on_close_callback);
>   } else {
>     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
>                             base::Bind(&FileCloser,
>                                        Passed(&file)));
>   }
> }

Done.

[rvargas (doing something else), 2014-06-03 23:19:58]

thanks

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/306403005/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:110:
base::Bind(&FileCloser, Passed(&file)));
On second thought, now there is code to invoke the callback.

[bbudge, 2014-06-03 23:43:09]

LGTM (one nit). Thanks for doing this!

https://codereview.chromium.org/306403005/diff/50001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/306403005/diff/50001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:99:
base::Bind(&FileCloser, Passed(&file)),
nit: base::Passed

[rvargas (doing something else), 2014-06-04 01:36:12]

Thanks

https://codereview.chromium.org/306403005/diff/50001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/306403005/diff/50001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:99:
base::Bind(&FileCloser, Passed(&file)),
On 2014/06/03 23:43:09, bbudge wrote:
> nit: base::Passed

Done.

[rvargas (doing something else), 2014-06-04 01:36:27]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:38:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/306403005/70001

[commit-bot: I haz the power, 2014-06-04 12:15:14]

Change committed as 274782