Add Worker support for FileSystem API.

Add Worker support for FileSystem API.

(corresponds to https://bugs.webkit.org/show_bug.cgi?id=45808)

No support for shared workers yet.

BUG=32277
TEST=none; layout tests will be added later.

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

[ericu, 2010-09-16 20:17:29]

I'm no expert on this area--I'm more learning from it than reviewing it--but it
looks fine to me.  Please wait for a more-knowledgeable review before
submitting.

[michaeln, 2010-09-16 20:29:07]

nice job navigating the  bewildering worker world!

http://codereview.chromium.org/3394003/diff/4001/24
File chrome/common/file_system/open_file_system_dispatcher.cc (right):

http://codereview.chromium.org/3394003/diff/4001/24#newcode22
chrome/common/file_system/open_file_system_dispatcher.cc:22: : callbacks(c) { }
nit: indent 2 more spaces, no need for a space between {}'s

http://codereview.chromium.org/3394003/diff/4001/24#newcode30
chrome/common/file_system/open_file_system_dispatcher.cc:30:
OpenFileSystemDispatcher::OpenFileSystemDispatcher() {
are the empty ctor/dtor's needed?

http://codereview.chromium.org/3394003/diff/4001/25
File chrome/common/file_system/open_file_system_dispatcher.h (right):

http://codereview.chromium.org/3394003/diff/4001/25#newcode30
chrome/common/file_system/open_file_system_dispatcher.h:30: WebKit::WebFrame*
frame,
Since this is used for both workers and documents, can this be a const GURL&
security_origin param instead of a 'frame' ptr?

http://codereview.chromium.org/3394003/diff/4001/32
File chrome/worker/webworkerclient_proxy.h (right):

http://codereview.chromium.org/3394003/diff/4001/32#newcode86
chrome/worker/webworkerclient_proxy.h:86: virtual void
openFileSystem(WebKit::WebFrame* frame,
Does 'frame' really belong in this API?

I know its in the allowDatabase call too and that one seems out of place too.
What this method ultimately needs to know is the security origin and I think
that value can be determined w/o any additional params in the worker case.

Unlike WebFrameClient (where multiple frames refer to the same client) there is
a one-to-one relationship between WebWorkerClient and a WebWorker. You can see
this is implied by the interface because the 'client' methods don't have a
'worker' arg. So, the worker calling openFileSystem is implied by that
relationship and we can know the security origin w/o being told in the API call.

We can know the origin based on the 'script_url' passed into
WebWorkerBase::CreateWorkerContext.

[kinuko, 2010-09-17 00:19:53]

http://codereview.chromium.org/3394003/diff/4001/24
File chrome/common/file_system/open_file_system_dispatcher.cc (right):

http://codereview.chromium.org/3394003/diff/4001/24#newcode22
chrome/common/file_system/open_file_system_dispatcher.cc:22: : callbacks(c) { }
On 2010/09/16 20:29:07, michaeln wrote:
> nit: indent 2 more spaces, no need for a space between {}'s

Done.

http://codereview.chromium.org/3394003/diff/4001/24#newcode30
chrome/common/file_system/open_file_system_dispatcher.cc:30:
OpenFileSystemDispatcher::OpenFileSystemDispatcher() {
On 2010/09/16 20:29:07, michaeln wrote:
> are the empty ctor/dtor's needed?

Seems like I need them in .cpp files to get it compiling with IDMap (with
IDMapOwnPointer) of PendingRequest that is defined in .cpp.

http://codereview.chromium.org/3394003/diff/4001/25
File chrome/common/file_system/open_file_system_dispatcher.h (right):

http://codereview.chromium.org/3394003/diff/4001/25#newcode30
chrome/common/file_system/open_file_system_dispatcher.h:30: WebKit::WebFrame*
frame,
On 2010/09/16 20:29:07, michaeln wrote:
> Since this is used for both workers and documents, can this be a const GURL&
> security_origin param instead of a 'frame' ptr?

Done.

http://codereview.chromium.org/3394003/diff/4001/32
File chrome/worker/webworkerclient_proxy.h (right):

http://codereview.chromium.org/3394003/diff/4001/32#newcode86
chrome/worker/webworkerclient_proxy.h:86: virtual void
openFileSystem(WebKit::WebFrame* frame,
On 2010/09/16 20:29:07, michaeln wrote:
> Does 'frame' really belong in this API?

Removed.  (So did the same in the corresponding WebKit patch.)

It's great to know that we can get rid of the frame parameter here.

[michaeln, 2010-09-17 00:51:56]

http://codereview.chromium.org/3394003/diff/4001/24
File chrome/common/file_system/open_file_system_dispatcher.cc (right):

http://codereview.chromium.org/3394003/diff/4001/24#newcode30
chrome/common/file_system/open_file_system_dispatcher.cc:30:
OpenFileSystemDispatcher::OpenFileSystemDispatcher() {
On 2010/09/17 00:19:53, Kinuko Yasuda wrote:
> On 2010/09/16 20:29:07, michaeln wrote:
> > are the empty ctor/dtor's needed?
> 
> Seems like I need them in .cpp files to get it compiling with IDMap (with
> IDMapOwnPointer) of PendingRequest that is defined in .cpp.

ah... got it!

http://codereview.chromium.org/3394003/diff/11001/12006#newcode13
chrome/common/file_system/open_file_system_dispatcher.cc:13: #include
"third_party/WebKit/WebKit/chromium/public/WebSecurityOrigin.h"
no longer needed, WebFrame and WebSecurityOrigin?

http://codereview.chromium.org/3394003/diff/11001/12006#newcode18
chrome/common/file_system/open_file_system_dispatcher.cc:18: using
WebKit::WebSecurityOrigin;
ditto

http://codereview.chromium.org/3394003/diff/11001/12006#newcode64
chrome/common/file_system/open_file_system_dispatcher.cc:64: 
i guess this is one of those place where dumi wants chrome types to avoid type
conversion thrashing... seems like a reasonable request

http://codereview.chromium.org/3394003/diff/11001/12007
File chrome/common/file_system/open_file_system_dispatcher.h (right):

http://codereview.chromium.org/3394003/diff/11001/12007#newcode11
chrome/common/file_system/open_file_system_dispatcher.h:11: #include
"googleurl/src/gurl.h"
looks like this could be forward declared in the .h and included in the .cc

http://codereview.chromium.org/3394003/diff/11001/12007#newcode35
chrome/common/file_system/open_file_system_dispatcher.h:35: void
OnOpenFileSystemRequestComplete(
maybe put this method in the private section

http://codereview.chromium.org/3394003/diff/11001/12015
File chrome/worker/webworker_stub_base.h (right):

http://codereview.chromium.org/3394003/diff/11001/12015#newcode46
chrome/worker/webworker_stub_base.h:46: scoped_ptr<OpenFileSystemDispatcher>
open_file_system_dispatcher_;
I'm wondering if you had considered making one instance of this dispatcher per
process instead of one per worker/document? Many of our other 'dispatchers' are
one per process (see WorkerThread.h and ChildThread.h). Maybe even just add
support for this openFileSystem/completed method pair to FileSystemDispatcher
(which is already wired into ChildThread).

I see it does contain some state about pending request per context, maybe that's
why you went with a one per context instead of one per process?

[kinuko, 2010-09-17 04:38:01]

On 2010/09/17 00:51:56, michaeln wrote:
> http://codereview.chromium.org/3394003/diff/11001/12007#newcode35
> chrome/common/file_system/open_file_system_dispatcher.h:35: void
> OnOpenFileSystemRequestComplete(
> maybe put this method in the private section
> 
> http://codereview.chromium.org/3394003/diff/11001/12015
> File chrome/worker/webworker_stub_base.h (right):
> 
> http://codereview.chromium.org/3394003/diff/11001/12015#newcode46
> chrome/worker/webworker_stub_base.h:46: scoped_ptr<OpenFileSystemDispatcher>
> open_file_system_dispatcher_;
> I'm wondering if you had considered making one instance of this dispatcher per
> process instead of one per worker/document? Many of our other 'dispatchers'
are
> one per process (see WorkerThread.h and ChildThread.h). Maybe even just add
> support for this openFileSystem/completed method pair to FileSystemDispatcher
> (which is already wired into ChildThread).
> 
> I see it does contain some state about pending request per context, maybe
that's
> why you went with a one per context instead of one per process?

Yes I wondered if I should merge the dispatcher into FileSystemDispatcher too.
I wasn't sure (or had no guts to examine) if having one integrated per-process
dispatcher would have no problem - but now that you say so I made that change.

[michaeln, 2010-09-17 20:39:11]

http://codereview.chromium.org/3394003/diff/27001/28007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3394003/diff/27001/28007#newcode683
chrome/renderer/render_view.cc:683: if
(ChildThread::current()->file_system_dispatcher()->OnMessageReceived(
Is this call in RenderView actually needed?

I would have thought that messages would have been plumbed to the
file_system_dispatcher via ChildThread::OnMessageReceived well in advance of
them coming thru here.

void ChildThread::OnMessageReceived(const IPC::Message& msg) {
  // Resource responses are sent to the resource dispatcher.
  if (resource_dispatcher_->OnMessageReceived(msg))
    return;
  if (socket_stream_dispatcher_->OnMessageReceived(msg))
    return;
  if (file_system_dispatcher_->OnMessageReceived(msg))
    return;

http://codereview.chromium.org/3394003/diff/27001/28010
File chrome/worker/websharedworker_stub.h (right):

http://codereview.chromium.org/3394003/diff/27001/28010#newcode33
chrome/worker/websharedworker_stub.h:33: // Invoked when the
WebWorkerClientProxy is shutting down.
unrelated: this comment about shutting down look very out of place where it is,
maybe delete it?

[kinuko, 2010-09-17 21:00:55]

http://codereview.chromium.org/3394003/diff/27001/28007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3394003/diff/27001/28007#newcode683
chrome/renderer/render_view.cc:683: if
(ChildThread::current()->file_system_dispatcher()->OnMessageReceived(
On 2010/09/17 20:39:11, michaeln wrote:
> Is this call in RenderView actually needed?

Good point, I don't think so, I had forgotten to check/remove.

[kinuko, 2010-09-17 21:42:30]

http://codereview.chromium.org/3394003/diff/27001/28007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3394003/diff/27001/28007#newcode683
chrome/renderer/render_view.cc:683: if
(ChildThread::current()->file_system_dispatcher()->OnMessageReceived(
On 2010/09/17 21:00:56, Kinuko Yasuda wrote:
> On 2010/09/17 20:39:11, michaeln wrote:
> > Is this call in RenderView actually needed?
> 
> Good point, I don't think so, I had forgotten to check/remove.

Removed.

http://codereview.chromium.org/3394003/diff/27001/28010
File chrome/worker/websharedworker_stub.h (right):

http://codereview.chromium.org/3394003/diff/27001/28010#newcode33
chrome/worker/websharedworker_stub.h:33: // Invoked when the
WebWorkerClientProxy is shutting down.
On 2010/09/17 20:39:11, michaeln wrote:
> unrelated: this comment about shutting down look very out of place where it
is,
> maybe delete it?

Done.

[michaeln, 2010-09-17 22:22:15]

LGTM

[kinuko, 2010-09-23 04:00:12]

On 2010/09/17 22:22:15, michaeln wrote:
> LGTM

(+dumi)
Updated to use the newly added callback dispatcher for openFileSystem.
This patch would need another review now -- Dumi, could you take a look at it?
(You could only focus on the files that have diffs from the previous one, and
common/file_system/webfilesystem_callback_dispatcher.{cc,h}.)

[dumi, 2010-09-23 04:44:22]

http://codereview.chromium.org/3394003/diff/17002/36012
File chrome/common/file_system/file_system_dispatcher.cc (right):

http://codereview.chromium.org/3394003/diff/17002/36012#newcode44
chrome/common/file_system/file_system_dispatcher.cc:44:
ViewHostMsg_OpenFileSystemRequest_Params params;
is 'routing_id' the routing ID of the renderer process? if so, then we should
declare ViewHostMsg_OpenFileSystemRequest as a ROUTED message (that will
automatically add a routing_id parameter, as the first parameter of this
message). also, do we need ViewHostMsg_OpenFileSystemRequest_Params? can we
change this message to:

IPC_MESSAGE_ROUTED4(ViewHostMsg_OpenFileSystemRequest,
                    int /* request_id */,
                    GURL /* origin */,
                    fileapi::FileSystemType /* type */,
                    long long /* size */)

and add some code to handle fileapi::FileSystemType if needed?

[kinuko, 2010-09-24 00:32:42]

http://codereview.chromium.org/3394003/diff/17002/36012
File chrome/common/file_system/file_system_dispatcher.cc (right):

http://codereview.chromium.org/3394003/diff/17002/36012#newcode44
chrome/common/file_system/file_system_dispatcher.cc:44:
ViewHostMsg_OpenFileSystemRequest_Params params;
On 2010/09/23 04:44:22, dumi wrote:
> is 'routing_id' the routing ID of the renderer process? if so, then we should
> declare ViewHostMsg_OpenFileSystemRequest as a ROUTED message (that will
> automatically add a routing_id parameter, as the first parameter of this
> message).

Imo this message is meant to be a control message just telling the peer its
routing_id but not a routed message.
...but it turned out that with the latest changes we no longer need the
routing_id in its peer either, so I just removed it.

> also, do we need ViewHostMsg_OpenFileSystemRequest_Params? can we
> change this message to:
> and add some code to handle fileapi::FileSystemType if needed?

Done.

[dumi, 2010-09-25 00:20:09]

LGTM