WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
26126
Refactor methods of WorkerMessagingProxy used to talk to main-thread loader into new interface.
https://bugs.webkit.org/show_bug.cgi?id=26126
Summary
Refactor methods of WorkerMessagingProxy used to talk to main-thread loader i...
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-
Details
Formatted Diff
Diff
Updated according to review comments.
(33.70 KB, patch)
2009-06-08 18:37 PDT
,
Dmitry Titov
levin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Landed:
http://trac.webkit.org/changeset/44515
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug