Bug 24055 - Replace WorkerMessagingProxy with two separate proxy classes.
Summary: Replace WorkerMessagingProxy with two separate proxy classes.
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 24054
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-19 22:28 PST by Jian Li
Modified: 2009-02-25 02:23 PST (History)
4 users (show)

See Also:


Attachments
Proposed Patch (48.72 KB, patch)
2009-02-19 22:30 PST, Jian Li
no flags Details | Formatted Diff | Diff
Proposed Patch (52.41 KB, patch)
2009-02-20 16:00 PST, Jian Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-02-19 22:28:48 PST
This is the second part of the work in order to split WorkerMessagingProxy into two separate proxy classes. This includes all the necessary changes to plug into two new proxy classes introduced in the first part of the work.
Comment 1 Jian Li 2009-02-19 22:30:46 PST
Created attachment 27823 [details]
Proposed Patch
Comment 2 David Levin 2009-02-20 01:10:09 PST
Comment on attachment 27823 [details]
Proposed Patch

> Index: WebCore/ChangeLog
> +        * GNUmakefile.am:
> +        * WebCore.vcproj/WebCore.vcproj:
> +        * WebCore.xcodeproj/project.pbxproj:
missing some makefiles here.



> Index: WebCore/dom/Worker.cpp

> +void Worker::reportPendingActivity(bool confirmMessage, bool hasPendingActivity)
WebKit eng is trying to move away from bool's to enums.  I think that would be great here b/c I've found places calling this to be unreadable.

Is the initial state of Worker::hasPendingActivity the same as it was before?



> Index: WebCore/dom/WorkerObjectProxy.h

> +#include "ScriptExecutionContext.h"
How is this used in this file?



> Index: WebCore/loader/WorkerThreadableLoader.cpp

> -    if (thisPtr->m_messagingProxy.askedToTerminate())
> +    if (thisPtr->isLoaderAlive())
>          return;
This doesn't look like it serves the same purpose.


> +void WorkerThreadableLoader::MainThreadBridge::postTaskToLoader(PassRefPtr<ScriptExecutionContext::Task>)
> +{
> +    // FIXME: to be implemented.
> +    notImplemented();
> +}
I would hope that we could find a way to keep this working in WebKit at least even if it isn't cross platform (with respect to Chromium) for now.


also:
m_workerContext needs to be deref'ed on the worker context thread in MainThreadBridge::destroy.
Comment 3 Jian Li 2009-02-20 16:00:06 PST
Created attachment 27843 [details]
Proposed Patch
Comment 4 Jian Li 2009-02-20 16:01:36 PST
(In reply to comment #2)
> (From update of attachment 27823 [details] [review])
> > Index: WebCore/ChangeLog
> > +        * GNUmakefile.am:
> > +        * WebCore.vcproj/WebCore.vcproj:
> > +        * WebCore.xcodeproj/project.pbxproj:
> missing some makefiles here.
> 
These makefiles seem not to have all worker related files added so far.
> 
> 
> > Index: WebCore/dom/Worker.cpp
> 
> > +void Worker::reportPendingActivity(bool confirmMessage, bool hasPendingActivity)
> WebKit eng is trying to move away from bool's to enums.  I think that would be
> great here b/c I've found places calling this to be unreadable.
> 
> Is the initial state of Worker::hasPendingActivity the same as it was before?
> 
Yes.
> 
> 
> > Index: WebCore/dom/WorkerObjectProxy.h
> 
> > +#include "ScriptExecutionContext.h"
> How is this used in this file?
> 
Because we need it for enum like MessageDestination and MessageSource.
> 
> 
> > Index: WebCore/loader/WorkerThreadableLoader.cpp
> 
> > -    if (thisPtr->m_messagingProxy.askedToTerminate())
> > +    if (thisPtr->isLoaderAlive())
> >          return;
> This doesn't look like it serves the same purpose.
> 
Fixed.
> 
> > +void WorkerThreadableLoader::MainThreadBridge::postTaskToLoader(PassRefPtr<ScriptExecutionContext::Task>)
> > +{
> > +    // FIXME: to be implemented.
> > +    notImplemented();
> > +}
> I would hope that we could find a way to keep this working in WebKit at least
> even if it isn't cross platform (with respect to Chromium) for now.
> 
Fixed.
> 
> also:
> m_workerContext needs to be deref'ed on the worker context thread in
> MainThreadBridge::destroy.
> 
> 
Fixed.

Comment 5 David Levin 2009-02-20 16:14:42 PST
This looks good to me.

There may be a lifetime issue with m_workerContext in usage in the bridge code, but I'll examine that more later as I get up tests for xhr in workers.  Also as mentioned before I think we can figure out a way to make lifetime easier to understand overall.


> > +void Worker::reportPendingActivity(bool confirmMessage, bool hasPendingActivity)
> WebKit eng is trying to move away from bool's to enums.  I think that would be
> great here b/c I've found places calling this to be unreadable.

It would be good to do this at some point in the future.
Comment 6 Alexey Proskuryakov 2009-02-25 02:23:42 PST
Comment on attachment 27843 [details]
Proposed Patch

Clearing review flag to get this out of queue.