Bug 24055 – Replace WorkerMessagingProxy with two separate proxy classes.

• Component: WebCore Misc.
• Status: RESOLVED LATER

[Jian Li]

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.

[Jian Li]

Created attachment 27823 [details]
Proposed Patch

[David Levin]

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.

[Jian Li]

Created attachment 27843 [details]
Proposed Patch

[Jian Li]

(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.

[David Levin]

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.

[Alexey Proskuryakov]

Comment on attachment 27843 [details]
Proposed Patch

Clearing review flag to get this out of queue.