Use asynchronized api for file system request. [blink] (3/4)

1. allowFileSystem() function clean up in blink. Please refer CLs:
   https://codereview.chromium.org/289793002/ and
   https://codereview.chromium.org/273513005/
2. Use asynchronized api for file system request.

Initial patch.

BUG=343382
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174764

[Fady Samuel, 2014-05-13 17:23:55]

This seems reasonable to me. lgtm. Please get another reviewer to look at this.
kinuko@ perhaps. Thanks!

[Xi Han, 2014-05-13 17:26:35]

Please take a loot at look at the changes. Thanks a lot.

[Xi Han, 2014-05-15 19:26:31]

I update this CL according to the rename issue in previous 2 patches:
https://codereview.chromium.org/289793002/ and 
https://codereview.chromium.org/273513005/

Please take another look at the changes. Thank you very much.

[kinuko, 2014-05-16 10:47:04]

https://codereview.chromium.org/277353002/diff/60001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/60001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:71: if (!client() ||
!client()->requestFileSystemAccessSync(context)) {
Why we call Sync one here?  resolveURL could be called either in Worker or in
DOMWindow, should we have two versions for this one too?

https://codereview.chromium.org/277353002/diff/60001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:95:
bind(&LocalFileSystem::fileSystemAllowedCallbacks, this, context, type,
callbacks),
I haven't looked deeper into the bind implementation, but by taking a very rough
look at how ParamStorageTraits<> works don't we need to pass context as
PassRefPtr<> so that we can be sure the ptr's referenced while async call's
ongoing?

https://codereview.chromium.org/277353002/diff/60001/public/web/WebWorkerPerm...
File public/web/WebWorkerPermissionClientProxy.h (left):

https://codereview.chromium.org/277353002/diff/60001/public/web/WebWorkerPerm...
public/web/WebWorkerPermissionClientProxy.h:66: virtual void
requestFileSystemAccessAsync(const WebPermissionCallbacks& callbacks)
Why do we add this in the other CL and then remove it here?  Am I missing some
multi-sided patch dependencies outside this patch?

[Xi Han, 2014-05-16 19:48:44]

Update the implementations in LocalFileSystem. I keep all the function
definitions unchanged, but update their implementations to support both sync and
async apis. Please take another look. 

Thanks a lot!

https://codereview.chromium.org/277353002/diff/60001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/60001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:71: if (!client() ||
!client()->requestFileSystemAccessSync(context)) {

Great catch! The deleteFileSystem is the same case. I have updated them both.


On 2014/05/16 10:47:05, kinuko wrote:
> Why we call Sync one here?  resolveURL could be called either in Worker or in
> DOMWindow, should we have two versions for this one too?

https://codereview.chromium.org/277353002/diff/60001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:95:
bind(&LocalFileSystem::fileSystemAllowedCallbacks, this, context, type,
callbacks),
On 2014/05/16 10:47:05, kinuko wrote:
> I haven't looked deeper into the bind implementation, but by taking a very
rough
> look at how ParamStorageTraits<> works don't we need to pass context as
> PassRefPtr<> so that we can be sure the ptr's referenced while async call's
> ongoing?

Done.

https://codereview.chromium.org/277353002/diff/60001/public/web/WebWorkerPerm...
File public/web/WebWorkerPermissionClientProxy.h (left):

https://codereview.chromium.org/277353002/diff/60001/public/web/WebWorkerPerm...
public/web/WebWorkerPermissionClientProxy.h:66: virtual void
requestFileSystemAccessAsync(const WebPermissionCallbacks& callbacks)
In the other CL, this function is renamed from requestFileSystemAccess to
requestFileSystemAccessAsync. Because it is still used in
LocalFileSystemClient.cpp, so I didn't remove it there but remove it in this CL.


On 2014/05/16 10:47:05, kinuko wrote:
> Why do we add this in the other CL and then remove it here?  Am I missing some
> multi-sided patch dependencies outside this patch?

[Xi Han, 2014-05-18 02:21:37]

This is the 3rd CL, please take a look at it. Thanks a lot!

[kinuko, 2014-05-19 09:02:46]

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:87:
bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr,
callbacks)));
I think these patterns can be probably implemented using common internal method
which takes ExecutionContext and two closures (for allow and denied cases).

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.h:54: void
deleteFileSystem(PassRefPtr<ExecutionContext>, FileSystemType,
PassOwnPtr<AsyncFileSystemCallbacks>);
nit: it doesn't look really necessary to change these method signatures

[jochen (gone - plz use gerrit), 2014-05-19 13:22:52]

Source/web and public lgtm

[Xi Han, 2014-05-20 16:13:31]

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:87:
bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr,
callbacks)));
On 2014/05/19 09:02:46, kinuko wrote:
> I think these patterns can be probably implemented using common internal
method
> which takes ExecutionContext and two closures (for allow and denied cases).

Done.

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/277353002/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.h:54: void
deleteFileSystem(PassRefPtr<ExecutionContext>, FileSystemType,
PassOwnPtr<AsyncFileSystemCallbacks>);
On 2014/05/19 09:02:46, kinuko wrote:
> nit: it doesn't look really necessary to change these method signatures

Done.

https://codereview.chromium.org/277353002/diff/90001/public/web/WebPermission...
File public/web/WebPermissionClient.h (right):

https://codereview.chromium.org/277353002/diff/90001/public/web/WebPermission...
public/web/WebPermissionClient.h:52: virtual void
requestFileSystemAccessAsync(const WebPermissionCallbacks& callbacks) {
WebPermissionCallbacks permissionCallbacks(callbacks);
permissionCallbacks.doAllow(); }
Add default implementation here.

[Fady Samuel, 2014-05-20 21:02:36]

This looks reasonable thanks! LGTM again. Please get kinuko@ to review this
though. She might have more informed feedback than me.

[Fady Samuel, 2014-05-20 21:02:36]

This looks reasonable thanks! LGTM again. Please get kinuko@ to review this
though. She might have more informed feedback than me.

[kinuko, 2014-05-21 07:24:24]

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:84:
requestFileSystemAccesssAsyncInternal(contextPtr,
bind(&LocalFileSystem::resolveURLInternal, this, fileSystemURL, callbacks),
bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr,
callbacks));
Um, looking this closely actually this wouldn't work really well (as well as
your previous patch wouldn't either).

callbacks's given as PassOwnPtr, so one of the bind() will nullify it before the
other bind()'s evaluated.

I think you'll need to have a wrapper RefCounted class for
AsyncFileSystemCallbacks, and pass the wrapper to these internal methods instead
of the AsyncFileSystemCallbacks itself.

Also I think you can include the line 76-83 in the common method, so that the
body of each public method (resolveURL, requestFileSystem, deleteFileSystem)
would just contain single method call (+ creating the wrapper callback object
etc)

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.h:70: OwnPtr<FileSystemClient>
m_client;
I think this one should be in the private: section (at the very end of this
class definition)

[Xi Han, 2014-05-21 19:50:04]

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:84:
requestFileSystemAccesssAsyncInternal(contextPtr,
bind(&LocalFileSystem::resolveURLInternal, this, fileSystemURL, callbacks),
bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr,
callbacks));
On 2014/05/21 07:24:24, kinuko wrote:
> Um, looking this closely actually this wouldn't work really well (as well as
> your previous patch wouldn't either).
> 
> callbacks's given as PassOwnPtr, so one of the bind() will nullify it before
the
> other bind()'s evaluated.
> 
> I think you'll need to have a wrapper RefCounted class for
> AsyncFileSystemCallbacks, and pass the wrapper to these internal methods
instead
> of the AsyncFileSystemCallbacks itself.
> 
> Also I think you can include the line 76-83 in the common method, so that the
> body of each public method (resolveURL, requestFileSystem, deleteFileSystem)
> would just contain single method call (+ creating the wrapper callback object
> etc)

1. Great catch! I introduced a wrapper RefCounted structure for
AsyncFileSystemCallbacks.

2. I tried to puts lines 76-83 into a common method, but the code looks quite
complicated. This is because each public method (resolveURL, requestFileSystem)
calls different internal function (see line 81, 100). If we put them together,
we need to pass in a bind for these internal functions as well. This makes the
code become too complicated to read. It is ideal to have clean code, but it is
not right to make simple functions become so complicated. How about just leave
the code as it is now?

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.h:70: OwnPtr<FileSystemClient>
m_client;
On 2014/05/21 07:24:24, kinuko wrote:
> I think this one should be in the private: section (at the very end of this
> class definition)

Done.

[kinuko, 2014-05-22 10:37:42]

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:84:
requestFileSystemAccesssAsyncInternal(contextPtr,
bind(&LocalFileSystem::resolveURLInternal, this, fileSystemURL, callbacks),
bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr,
callbacks));
On 2014/05/21 19:50:05, hanxi wrote:
> 2. I tried to puts lines 76-83 into a common method, but the code looks quite
> complicated. This is because each public method (resolveURL,
requestFileSystem)
> calls different internal function (see line 81, 100). If we put them together,
> we need to pass in a bind for these internal functions as well. This makes the
> code become too complicated to read. It is ideal to have clean code, but it is
> not right to make simple functions become so complicated. How about just leave
> the code as it is now? 

Can't the common method do just like:

{
    if (!client()) {
        denied();
        return;
    }
    if (!context->isDocument()) {
        if (!client()->requestFileSystemAccessSync(context)) {
            denied();
            return;
        }
        allowed();
        return;
    }
    client()->requestFileSystemAccessAsync(context,
PermissionCallbacks::create(allowed, denied));
}

?

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
Source/modules/filesystem/LocalFileSystem.cpp:146:
PassRefPtr<RefCountedFileSystemCallbacks> callbacks) {
Please break the line before '{' (here and below. please utilize git cl
presubmit / git cl format)

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
Source/modules/filesystem/LocalFileSystem.h:46: struct
RefCountedFileSystemCallbacks : RefCounted<RefCountedFileSystemCallbacks> {
This can be hidden in .cpp file.

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
Source/modules/filesystem/LocalFileSystem.h:49:
PassOwnPtr<AsyncFileSystemCallbacks> callbacks;
Usually we don't use PassOwnPtr for storage type

[kinuko, 2014-05-22 10:43:23]

On 2014/05/22 10:37:42, kinuko wrote:
> Can't the common method do just like:
> 
> {
>     if (!client()) {
>         denied();
>         return;
>     }
>     if (!context->isDocument()) {
>         if (!client()->requestFileSystemAccessSync(context)) {
>             denied();
>             return;
>         }
>         allowed();
>         return;
>     }
>     client()->requestFileSystemAccessAsync(context,
> PermissionCallbacks::create(allowed, denied));
> }
> 
> ?

Fyi, here's a rough patch about what I meant:
https://codereview.chromium.org/290733012/

[kinuko, 2014-05-22 10:43:41]

On 2014/05/22 10:43:23, kinuko wrote:
> On 2014/05/22 10:37:42, kinuko wrote:
> > Can't the common method do just like:
> > 
> > {
> >     if (!client()) {
> >         denied();
> >         return;
> >     }
> >     if (!context->isDocument()) {
> >         if (!client()->requestFileSystemAccessSync(context)) {
> >             denied();
> >             return;
> >         }
> >         allowed();
> >         return;
> >     }
> >     client()->requestFileSystemAccessAsync(context,
> > PermissionCallbacks::create(allowed, denied));
> > }
> > 
> > ?
> 
> Fyi, here's a rough patch for what I meant:
> https://codereview.chromium.org/290733012/

[Xi Han, 2014-05-22 14:21:41]

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/90001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:84:
requestFileSystemAccesssAsyncInternal(contextPtr,
bind(&LocalFileSystem::resolveURLInternal, this, fileSystemURL, callbacks),
bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr,
callbacks));
On 2014/05/22 10:37:42, kinuko wrote:
> On 2014/05/21 19:50:05, hanxi wrote:
> > 2. I tried to puts lines 76-83 into a common method, but the code looks
quite
> > complicated. This is because each public method (resolveURL,
> requestFileSystem)
> > calls different internal function (see line 81, 100). If we put them
together,
> > we need to pass in a bind for these internal functions as well. This makes
the
> > code become too complicated to read. It is ideal to have clean code, but it
is
> > not right to make simple functions become so complicated. How about just
leave
> > the code as it is now? 
> 
> Can't the common method do just like:
> 
> {
>     if (!client()) {
>         denied();
>         return;
>     }
>     if (!context->isDocument()) {
>         if (!client()->requestFileSystemAccessSync(context)) {
>             denied();
>             return;
>         }
>         allowed();
>         return;
>     }
>     client()->requestFileSystemAccessAsync(context,
> PermissionCallbacks::create(allowed, denied));
> }
> 
> ?

Thank you for the correction and the rough patch. The code looks really neat and
clean now. Sorry about my comment, I realized that I accidentally introduced one
more layer of call which caused the code ugly.

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
Source/modules/filesystem/LocalFileSystem.cpp:146:
PassRefPtr<RefCountedFileSystemCallbacks> callbacks) {
On 2014/05/22 10:37:42, kinuko wrote:
> Please break the line before '{' (here and below. please utilize git cl
> presubmit / git cl format)

Done.

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
Source/modules/filesystem/LocalFileSystem.h:46: struct
RefCountedFileSystemCallbacks : RefCounted<RefCountedFileSystemCallbacks> {
On 2014/05/22 10:37:42, kinuko wrote:
> This can be hidden in .cpp file.

Done.

https://codereview.chromium.org/277353002/diff/120001/Source/modules/filesyst...
Source/modules/filesystem/LocalFileSystem.h:49:
PassOwnPtr<AsyncFileSystemCallbacks> callbacks;
On 2014/05/22 10:37:42, kinuko wrote:
> Usually we don't use PassOwnPtr for storage type

Oh, I see, great catch, thanks.

[kinuko, 2014-05-23 02:14:43]

lgtm

[Xi Han, 2014-05-24 12:14:05]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-05-24 12:14:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/277353002/150001

[commit-bot: I haz the power, 2014-05-24 13:35:00]

Change committed as 174764