Bug 26126

Summary: Refactor methods of WorkerMessagingProxy used to talk to main-thread loader into new interface.
Product: WebKit Reporter: Dmitry Titov <dimich>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, jianli, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
levin: review-
Updated according to review comments. levin: review+

Dmitry Titov
Reported 2009-06-01 18:15:33 PDT
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.
Attachments
Proposed patch (28.21 KB, patch)
2009-06-01 18:48 PDT, Dmitry Titov
levin: review-
Updated according to review comments. (33.70 KB, patch)
2009-06-08 18:37 PDT, Dmitry Titov
levin: review+
Dmitry Titov
Comment 1 2009-06-01 18:48:37 PDT
Created attachment 30850 [details] Proposed patch
David Levin
Comment 2 2009-06-08 17:32:19 PDT
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
Comment 3 2009-06-08 18:37:01 PDT
(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
Comment 4 2009-06-08 18:37:40 PDT
Created attachment 31077 [details] Updated according to review comments.
Dmitry Titov
Comment 5 2009-06-08 20:16:54 PDT
Note You need to log in before you can comment on or make changes to this bug.