Bug 26126 – Refactor methods of WorkerMessagingProxy used to talk to main-thread loader into new interface.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Titov]

This is a part of work to enable implementation of XHR in workers in Chromium, and can be used by nested workers implementation as well. It introduces additional WorkerLoaderProxy.

We have WorkerObjectProxy and WorkerContextProxy already, however, Workers not only talk back and forth with WorkerObject but they also need to talk to the loading code that normally lives on a main thread. In addition, in multi-process browsers, the 'loading context' can be a distinct instance from 'Worker object' and can live in different processes. 

WorkerThreadableLoader::MainThreadBridge used to use methods on WorkerMessagingProxy directly to schedule loading tasks on main thread and get the results back on worker thread. The new abstract interface WorkerLoadignProxy gets these methods so WorkerMessagingProxy can implement them, as well as Chromium, since it doesn't have WorkerMessagingProxy but rather has a set of IPC proxies on its own.

WorkerThreadableLoader::MainThreadBridge now uses an instance of WorkerLoaderProxy to talk to the DocumentThreadaableLoader. Some FIXMEs were removed.

A small note about removing a check for "askedToTerminate" from WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader: David Levin and I looked at it together and we don't see danger in removing this - so no need to add this method on new WorkerLoaderProxy. The absence of this check only avoids creating the main thread loader in some small number of cases cases when the worker is being terminated from the parent side but the tasks 'crossed in queues'. The loader will still be destroyed as a result of WorkerContext destruction, so it seems to be an optimization for a pretty narrow case.

[Dmitry Titov]

Created attachment 30850 [details]
Proposed patch

[David Levin]

Comment on attachment 30850 [details]
Proposed patch

A few issues to fix up, so r- for this version of the patch.

> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj
> index a2742ac..2996d35 100644
> --- a/WebCore/WebCore.xcodeproj/project.pbxproj
> +++ b/WebCore/WebCore.xcodeproj/project.pbxproj
> @@ -208,6 +208,7 @@
>  		188604B30F2E654A000B6443 /* DOMTimer.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 188604B10F2E654A000B6443 /* DOMTimer.cpp */; };
>  		188604B40F2E654A000B6443 /* DOMTimer.h in Headers */ = {isa = PBXBuildFile; fileRef = 188604B20F2E654A000B6443 /* DOMTimer.h */; };
> +		18F831B80FD48C7800D8C56B /* WorkerLoaderProxy.h in Headers */ = {isa = PBXBuildFile; fileRef = 18F831B70FD48C7800D8C56B /* WorkerLoaderProxy.h */; };

Is this out of order?


> diff --git a/WebCore/loader/WorkerThreadableLoader.cpp b/WebCore/loader/WorkerThreadableLoader.cpp
>  

Is it possible to remove the #include "WorkerMessagingProxy.h"?


> -WorkerThreadableLoader::MainThreadBridge::MainThreadBridge(PassRefPtr<ThreadableLoaderClientWrapper> workerClientWrapper, WorkerMessagingProxy& messagingProxy, const String& taskMode,
> +WorkerThreadableLoader::MainThreadBridge::MainThreadBridge(PassRefPtr<ThreadableLoaderClientWrapper> workerClientWrapper, WorkerLoaderProxy* loaderProxy, const String& taskMode,

Ideally loaderProxy would be a & since it can never be 0.



> diff --git a/WebCore/workers/WorkerLoaderProxy.h b/WebCore/workers/WorkerLoaderProxy.h
> +
> +        virtual ~WorkerLoaderProxy() {}
Nice to have a space in { }.

> +        // Postst callbacks from loading code to the WorkerContext. The 'mode' is used to differentiate

Postst spelling.

[Dmitry Titov]

(In reply to comment #2)
> (From update of attachment 30850 [details] [review])
> > +		18F831B80FD48C7800D8C56B /* WorkerLoaderProxy.h in Headers */ = {isa = PBXBuildFile; fileRef = 18F831B70FD48C7800D8C56B /* WorkerLoaderProxy.h */; };
> 
> Is this out of order?

Seems ok in the xcode itself (the file is in right alphabetical order in the Sources). 

> > diff --git a/WebCore/loader/WorkerThreadableLoader.cpp b/WebCore/loader/WorkerThreadableLoader.cpp
> Is it possible to remove the #include "WorkerMessagingProxy.h"?

Yes, removed. Instead, included WorkerLoaderProxy.h

> > +WorkerThreadableLoader::MainThreadBridge::MainThreadBridge(PassRefPtr<ThreadableLoaderClientWrapper> workerClientWrapper, WorkerLoaderProxy* loaderProxy, const String& taskMode,
> 
> Ideally loaderProxy would be a & since it can never be 0.

Done, also changed WorkerThread::workerLoaderProxy and WorkerThread::workerObjectProxy to return &, accordingly.

> > diff --git a/WebCore/workers/WorkerLoaderProxy.h b/WebCore/workers/WorkerLoaderProxy.h
> > +
> > +        virtual ~WorkerLoaderProxy() {}
> Nice to have a space in { }.

Done.

> > +        // Postst callbacks from loading code to the WorkerContext. The 'mode' is used to differentiate
> 
> Postst spelling.

Fixed.

[Dmitry Titov]

Created attachment 31077 [details]
Updated according to review comments.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/44515