Move webworker{client}_impl.{h,cc} into webkit/api/src

Move webworker{client}_impl.{h,cc} into webkit/api/src

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30478

[yaar, 2009-10-28 01:40:16]

Enjoy!

[darin (slow to review), 2009-10-28 04:07:37]

Thanks for the patch!

http://codereview.chromium.org/337057/diff/1/2
File webkit/api/src/EmptyWebFrameClientImpl.h (right):

http://codereview.chromium.org/337057/diff/1/2#newcode40
Line 40: class EmptyWebFrameClient : public WebFrameClient {
I should have mentioned this before, but my plan has been to move these default
implementations to WebFrameClient.  I only resisted this so far as a means to
help me catch errors while things were moving around so much.

But, once we upstream the API to svn.webkit.org, we'll need to make sure that
all callbacks have default implementations so that we can support using a newer
WebKit with an older Chromium.

If you want to do this step as a follow-up CL, that's OK too.

http://codereview.chromium.org/337057/diff/1/3
File webkit/api/src/WebWorkerClientImpl.cpp (right):

http://codereview.chromium.org/337057/diff/1/3#newcode64
Line 64: #include "webkit/glue/webkitclient_impl.h"
is this webkitclient_impl.h header really required?  i do not plan to move that
into webkit/api/src.

http://codereview.chromium.org/337057/diff/1/3#newcode105
Line 105: NOTREACHED();
please use ASSERT_NOT_REACHED instead.

http://codereview.chromium.org/337057/diff/1/3#newcode111
Line 111: currentContext->workerContext()->thread());
nit: strange indentation here

http://codereview.chromium.org/337057/diff/1/3#newcode113
Line 113: &thread->workerObjectProxy();
nit: looks like this should not have been wrapped to the next line

http://codereview.chromium.org/337057/diff/1/3#newcode128
Line 128: , m_workerThreadId(WTF::currentThread()) {
opening bracket should be on the next line

http://codereview.chromium.org/337057/diff/1/3#newcode147
Line 147: if (!WTF::isMainThread()) {
nit: it is commonplace to just leave off the WTF:: prefix.

http://codereview.chromium.org/337057/diff/1/3#newcode172
Line 172: WTF::PassRefPtr<SerializedScriptValue> message,
ditto, remove the WTF:: prefix.  here and elsewhere in this file.

http://codereview.chromium.org/337057/diff/1/4
File webkit/api/src/WebWorkerClientImpl.h (right):

http://codereview.chromium.org/337057/diff/1/4#newcode36
Line 36: #include "webkit/api/public/WebWorkerClient.h"
nit: i'm guessing you needed a path here since webkit/glue files are still
including this file.  in that case, please do what i've been doing elsewhere. 
make the path be "../public/WebWorkerClient.h", and put a FIXME comment above it
about removing the path once all consumes have been moved out of webkit/glue.

http://codereview.chromium.org/337057/diff/1/4#newcode54
Line 54: ,  public WebWorkerClient {
nit: extra space after the comma

http://codereview.chromium.org/337057/diff/1/4#newcode140
Line 140: WTF::RefPtr<WebCore::ScriptExecutionContext> m_scriptExecutionContext;
nit: we usually leave off WTF:: in implementation header files since that is
common practice in WebCore.

http://codereview.chromium.org/337057/diff/1/5
File webkit/api/src/WebWorkerImpl.cpp (right):

http://codereview.chromium.org/337057/diff/1/5#newcode60
Line 60: #include "webkit/glue/webpreferences.h"
we should find a way to avoid depending on webpreferences.h.    this doesn't
have to block this CL.  it looks like there may be some complications.

http://codereview.chromium.org/337057/diff/1/5#newcode63
Line 63: using namespace WTF;
the WTF namespace is already automatically imported.

http://codereview.chromium.org/337057/diff/1/5#newcode81
Line 81: static WorkerWebFrameClient* GetSharedInstance()
use camelCase...  just call it "sharedInstance"

http://codereview.chromium.org/337057/diff/1/5#newcode93
Line 93: WebWorker* WebWorker::create(WebWorkerClient* client) {
nit: opening bracket on the next line

http://codereview.chromium.org/337057/diff/1/5#newcode231
Line 231: static_cast<ScriptExecutionContext::Task*>(param);
nit: indent by 4

http://codereview.chromium.org/337057/diff/1/5#newcode377
Line 377: WebWorker* WebWorker::create(WebWorkerClient* client) {
nit: opening bracket on next line

http://codereview.chromium.org/337057/diff/1/6
File webkit/api/src/WebWorkerImpl.h (right):

http://codereview.chromium.org/337057/diff/1/6#newcode78
Line 78: WTF::PassRefPtr<WebCore::ScriptExecutionContext::Task>, const
WebCore::String& mode);
same nit about WTF::

[yaar, 2009-10-28 16:56:30]

Fixed. I opened issues for subsequent CLs.

[jam, 2009-10-28 17:33:02]

http://codereview.chromium.org/337057/diff/5001/5003
File webkit/api/src/WebWorkerClientImpl.cpp (right):

http://codereview.chromium.org/337057/diff/5001/5003#newcode85
Line 85: //  FIXME: not supported in chromium.
Can you elaborate more in this comment: i.e. 

FIXME: pass in a flag to indicate whether multiple workers can run in one
process.

[yaar, 2009-10-28 18:05:37]

How about:
#ifdef ENABLE(WEB_WORKERS_SHARE_PROCESS)
    // Special behavior if multiple workers per process are enabled.
    // Note that v8 doesn't more than one workers per process.
    if (!worker->scriptExecutionContext()->isDocument())
        return new WorkerMessagingProxy(worker);
#endif


On Wed, Oct 28, 2009 at 10:33 AM, <jam@chromium.org> wrote:

>
> http://codereview.chromium.org/337057/diff/5001/5003
>
> File webkit/api/src/WebWorkerClientImpl.cpp (right):
>
> http://codereview.chromium.org/337057/diff/5001/5003#newcode85
> Line 85: //  FIXME: not supported in chromium.
> Can you elaborate more in this comment: i.e.
>
> FIXME: pass in a flag to indicate whether multiple workers can run in
> one process.
>
>
> http://codereview.chromium.org/337057
>

[yaar, 2009-10-28 21:34:55]

Added a comment that John requested, and eliminated webpreferences dependency.

[jam, 2009-10-28 21:46:13]

On 2009/10/28 18:05:37, yaar wrote:
> How about:
> #ifdef ENABLE(WEB_WORKERS_SHARE_PROCESS)
>     // Special behavior if multiple workers per process are enabled.
>     // Note that v8 doesn't more than one workers per process.
>     if (!worker->scriptExecutionContext()->isDocument())
>         return new WorkerMessagingProxy(worker);
> #endif

This is all run-time now, and we'll want to maintain that.  So adding an ifdef
makes it look like compile time is ok.  Can you change it back to just a
comment?  Of course, actually passing in the boolean in this change would be the
best :)

> >
> >
> > http://codereview.chromium.org/337057
> >
>

[darin (slow to review), 2009-10-28 23:51:25]

LGTM