Initial checkin of the out of process worker implementation....

Initial checkin of the out of process worker implementation.

WebWorkerClient/WebWorker are parallel interfaces of WebCore::{WorkerObjectProxy, WorkerContextProxy} that use Chrome data types.  When WebKit requests a WorkerObjectProxy, we create an instance of WebWorkerClientImpl.  This class creates an object that implements a Chromium version of WorkerObjectProxy (i.e. with Chrome data types) through WebViewDelegate.  That object is a WebWorkerProxy and talks over IPC to a WebWorker object in the worker process.  The WebWorker object creates the actual WebCore::Worker object using another class in glue: WebWorkerImpl.

When the WebCore::Worker object running in the worker process wants to talk back to the code running in the renderer, it talks to WebWorkerImpl which implements WebCore::WorkerObjectProxy.  WebWorkerImpl converts the data types to Chrome compatible ones, and then calls the WebWorkerClient version which does IPC to get to the renderer process.  This ends up at WebWorkerProxy, which calls WebWorkerClientImpl (the original class).

In future changes, sandboxing, multiple worker processes etc will be added.  Note that I also had to make two small changes to WebKit, since WorkerMessagingProxy couldn't be created as is for the nested worker case.  I'll either check it in myself or work with Jian to do so.

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

[jianli, 2009-02-25 21:13:31]

Some comments so far for WebKit side changes.

http://codereview.chromium.org/27157/diff/1/8
File third_party/WebKit/WebCore/dom/WorkerMessagingProxy.cpp (right):

http://codereview.chromium.org/27157/diff/1/8#newcode175
Line 175: // TODO(jabdelmalek): this function needs to be moved into a separate
file that's not included by Chromium.
We're already in the process of wrapping this in ifdef and adding the chromium
version. Please see https://bugs.webkit.org/show_bug.cgi?id=24113.

http://codereview.chromium.org/27157/diff/1/41
File third_party/WebKit/WebCore/dom/WorkerMessagingProxy.h (right):

http://codereview.chromium.org/27157/diff/1/41#newcode50
Line 50: virtual ~WorkerMessagingProxy();
Why do you need to move it to public? If we really want to do this, better
change it upstream.

[jam, 2009-02-25 22:15:51]

http://codereview.chromium.org/27157/diff/1/8
File third_party/WebKit/WebCore/dom/WorkerMessagingProxy.cpp (right):

http://codereview.chromium.org/27157/diff/1/8#newcode175
Line 175: // TODO(jabdelmalek): this function needs to be moved into a separate
file that's not included by Chromium.
On 2009/02/25 21:13:31, jianli wrote:
> We're already in the process of wrapping this in ifdef and adding the chromium
> version. Please see https://bugs.webkit.org/show_bug.cgi?id=24113.

I was just doing this change to allow me to compile (I wasn't aware of other
change, can you cc me on these bugs?).

Looking at the other change, I think we only need the changes to
WorkerMessagingProxy.cpp.  No need to change the Chromium bridge, since
WorkerContextProxy::create will live in glue.

http://codereview.chromium.org/27157/diff/1/41
File third_party/WebKit/WebCore/dom/WorkerMessagingProxy.h (right):

http://codereview.chromium.org/27157/diff/1/41#newcode50
Line 50: virtual ~WorkerMessagingProxy();
On 2009/02/25 21:13:31, jianli wrote:
> Why do you need to move it to public? If we really want to do this, better
> change it upstream.

Strange, it compiles now without this, but yesterday it didn't when it was  in
private.  Reverted the change.

[darin (slow to review), 2009-02-27 16:12:53]

Nice work!  I really like how much simpler this design is than the one we have
for plugins :-)

My main complaint is with the names...  hope you find my suggestions helpful.

http://codereview.chromium.org/27157/diff/1188/507
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/27157/diff/1188/507#newcode127
Line 127:
WorkerService::GetInstance()->OnResourceMessageFilterDestructing(this);
can this just be named something simpler like Shutdown() ?

http://codereview.chromium.org/27157/diff/1188/503
File chrome/browser/worker_process_host.h (right):

http://codereview.chromium.org/27157/diff/1188/503#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
these files should probably live under browser/renderer_host (or maybe there
should be a new browser/worker_host directory--ask brett or ben).

http://codereview.chromium.org/27157/diff/1188/509
File chrome/common/worker_messages_internal.h (right):

http://codereview.chromium.org/27157/diff/1188/509#newcode18
Line 18: /*
maybe add a TODO comment to explain why this is commented out?

http://codereview.chromium.org/27157/diff/1188/533
File chrome/renderer/worker_delegate.h (right):

http://codereview.chromium.org/27157/diff/1188/533#newcode5
Line 5: #ifndef CHROME_RENDERER_WORKER_H_
nit: doesn't match file name

http://codereview.chromium.org/27157/diff/1188/533#newcode16
Line 16: class Message;
nit: no indentation

http://codereview.chromium.org/27157/diff/1188/535
File chrome/worker/worker.cc (right):

http://codereview.chromium.org/27157/diff/1188/535#newcode64
Line 64: WebWorkerContextProxy* proxy = impl_.get();
this is a really nice approach (much simpler than what we have for plugins)!

http://codereview.chromium.org/27157/diff/1188/536
File third_party/WebKit/WebCore/workers/WorkerMessagingProxy.cpp (right):

http://codereview.chromium.org/27157/diff/1188/536#newcode174
Line 174: #if !PLATFORM(CHROMIUM)
OK, please get this upstreamed :-)

alternatively, do we even need to compile this .cpp file?

http://codereview.chromium.org/27157/diff/1188/505
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/27157/diff/1188/505#newcode133
Line 133: virtual WebWorkerContextProxy* CreateWebWorker(WebWorkerObjectProxy*
object) {
Can we use simpler names here?  How about if CreateWebWorker returns a WebWorker
and takes as a parameter a WebWorkerClient?

(Names like WebWorkerContextProxy and WebWorkerObjectProxy are really quite
unintuitive in their meaning.)

Then inside of glue, we could define WebWorkerImpl, which is what
WebWorker::Create would instantiate, allowing someone to create an actual worker
thread.  There would also be a WebWorkerClientImpl, which is what we'd
instantiate to pass to WebView::CreateWebWorker.

The renderer would allocate a WebWorkerProxy that implements WebWorker (just
like your current WorkerDelegate), and in the worker process we'd allocate a
WebWorkerClientProxy that implements WebWorkerClient.

WDYT?

http://codereview.chromium.org/27157/diff/1188/531
File webkit/glue/webworker.cc (right):

http://codereview.chromium.org/27157/diff/1188/531#newcode6
Line 6: #include "config.h"
nit: config.h should always be the first #include (though it probably doesn't
matter in this case)

http://codereview.chromium.org/27157/diff/1188/531#newcode29
Line 29: #if ENABLE(WORKERS)
nit: can this also enclose some of the includes?

http://codereview.chromium.org/27157/diff/1188/531#newcode39
Line 39: class ChromiumWorkerContextProxy : public WorkerContextProxy,
how about Worker{Context,Object}ProxyImpl?  and rename this file to
webworker_impl.cc?  then these classes could also move to webworker_impl.h (just
as we do for other glue implementations).

http://codereview.chromium.org/27157/diff/1188/530
File webkit/glue/webworker.h (right):

http://codereview.chromium.org/27157/diff/1188/530#newcode19
Line 19: WebWorkerContextProxy() { }
nit: no need to write this constructor

http://codereview.chromium.org/27157/diff/1188/530#newcode23
Line 23: const std::string& user_agent,
the original webcore value for this is a WebCore::String, right?  maybe it would
be better to use string16 here since that is a similar string type.

eventually, we'll move these to be WebString (once these interfaces move into
third_party/WebKit/WebKit/chromium/public), and so using string16 will reduce
friction later.

http://codereview.chromium.org/27157/diff/1188/530#newcode30
Line 30: DISALLOW_COPY_AND_ASSIGN(WebWorkerContextProxy);
this is unnecessary since WebWorkerContextProxy is not a concrete class.

http://codereview.chromium.org/27157/diff/1188/530#newcode38
Line 38: WebWorkerObjectProxy() { }
ditto

http://codereview.chromium.org/27157/diff/1188/530#newcode47
Line 47: int destination,
it's not obvious what destination, source, and level mean.  can they be defined?

http://codereview.chromium.org/27157/diff/1188/530#newcode53
Line 53: virtual void ReportPendingActivity(bool has_pending_activity) = 0;
what does pending activity mean?

http://codereview.chromium.org/27157/diff/1188/530#newcode57
Line 57: DISALLOW_COPY_AND_ASSIGN(WebWorkerObjectProxy);
ditto

http://codereview.chromium.org/27157/diff/1188/530#newcode62
Line 62: WebWorkerContextProxy* CreateWebWorkerContext(
nit: how about making this a static 'Create' method on WebWorkerContextProxy?

[jam, 2009-02-28 00:56:51]

http://codereview.chromium.org/27157/diff/1188/507
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/27157/diff/1188/507#newcode127
Line 127:
WorkerService::GetInstance()->OnResourceMessageFilterDestructing(this);
On 2009/02/27 16:12:53, darin wrote:
> can this just be named something simpler like Shutdown() ?

I agree the name is too long.  But Shutdown might confuse some people, since
they might think it means browser shutdown.  How about RendererShutdown?

http://codereview.chromium.org/27157/diff/1188/503
File chrome/browser/worker_process_host.h (right):

http://codereview.chromium.org/27157/diff/1188/503#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/02/27 16:12:53, darin wrote:
> these files should probably live under browser/renderer_host (or maybe there
> should be a new browser/worker_host directory--ask brett or ben).

discussed with ben, moved into browser/worker (no host since this includes
worker_service)

http://codereview.chromium.org/27157/diff/1188/509
File chrome/common/worker_messages_internal.h (right):

http://codereview.chromium.org/27157/diff/1188/509#newcode18
Line 18: /*
On 2009/02/27 16:12:53, darin wrote:
> maybe add a TODO comment to explain why this is commented out?

Done.

http://codereview.chromium.org/27157/diff/1188/533
File chrome/renderer/worker_delegate.h (right):

http://codereview.chromium.org/27157/diff/1188/533#newcode5
Line 5: #ifndef CHROME_RENDERER_WORKER_H_
On 2009/02/27 16:12:53, darin wrote:
> nit: doesn't match file name

Done.

http://codereview.chromium.org/27157/diff/1188/533#newcode16
Line 16: class Message;
On 2009/02/27 16:12:53, darin wrote:
> nit: no indentation

Done.

http://codereview.chromium.org/27157/diff/1188/535
File chrome/worker/worker.cc (right):

http://codereview.chromium.org/27157/diff/1188/535#newcode64
Line 64: WebWorkerContextProxy* proxy = impl_.get();
On 2009/02/27 16:12:53, darin wrote:
> this is a really nice approach (much simpler than what we have for plugins)!

yeah, it works because no intermediate work needs to be done (i.e. duplicating
handles/converting shared memory to HDC)..  But probably some methods can be
simplified to use this approach, and we can merge some classes together.  Future
cleanup :)

http://codereview.chromium.org/27157/diff/1188/536
File third_party/WebKit/WebCore/workers/WorkerMessagingProxy.cpp (right):

http://codereview.chromium.org/27157/diff/1188/536#newcode174
Line 174: #if !PLATFORM(CHROMIUM)
On 2009/02/27 16:12:53, darin wrote:
> OK, please get this upstreamed :-)

Yep Jian made a patch and it landed in r41291.

> 
> alternatively, do we even need to compile this .cpp file?

Yes we do, since we use it when creating a worker in the worker process.

http://codereview.chromium.org/27157/diff/1188/505
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/27157/diff/1188/505#newcode133
Line 133: virtual WebWorkerContextProxy* CreateWebWorker(WebWorkerObjectProxy*
object) {
On 2009/02/27 16:12:53, darin wrote:
> Can we use simpler names here?  How about if CreateWebWorker returns a
WebWorker
> and takes as a parameter a WebWorkerClient?
> 
> (Names like WebWorkerContextProxy and WebWorkerObjectProxy are really quite
> unintuitive in their meaning.)
> 
> Then inside of glue, we could define WebWorkerImpl, which is what
> WebWorker::Create would instantiate, allowing someone to create an actual
worker
> thread.  There would also be a WebWorkerClientImpl, which is what we'd
> instantiate to pass to WebView::CreateWebWorker.
> 
> The renderer would allocate a WebWorkerProxy that implements WebWorker (just
> like your current WorkerDelegate), and in the worker process we'd allocate a
> WebWorkerClientProxy that implements WebWorkerClient.
> 
> WDYT?

def better names than what I had, done.

http://codereview.chromium.org/27157/diff/1188/531
File webkit/glue/webworker.cc (right):

http://codereview.chromium.org/27157/diff/1188/531#newcode6
Line 6: #include "config.h"
On 2009/02/27 16:12:53, darin wrote:
> nit: config.h should always be the first #include (though it probably doesn't
> matter in this case)

Done.

http://codereview.chromium.org/27157/diff/1188/531#newcode29
Line 29: #if ENABLE(WORKERS)
On 2009/02/27 16:12:53, darin wrote:
> nit: can this also enclose some of the includes?

Done.

http://codereview.chromium.org/27157/diff/1188/531#newcode39
Line 39: class ChromiumWorkerContextProxy : public WorkerContextProxy,
On 2009/02/27 16:12:53, darin wrote:
> how about Worker{Context,Object}ProxyImpl?  and rename this file to
> webworker_impl.cc?  then these classes could also move to webworker_impl.h
(just
> as we do for other glue implementations).

Good idea, done.  Renamed to WebWorkerClientImpl to be consistent with
WebWorkerImpl.

http://codereview.chromium.org/27157/diff/1188/530
File webkit/glue/webworker.h (right):

http://codereview.chromium.org/27157/diff/1188/530#newcode19
Line 19: WebWorkerContextProxy() { }
On 2009/02/27 16:12:53, darin wrote:
> nit: no need to write this constructor

Done.

http://codereview.chromium.org/27157/diff/1188/530#newcode23
Line 23: const std::string& user_agent,
On 2009/02/27 16:12:53, darin wrote:
> the original webcore value for this is a WebCore::String, right?  maybe it
would
> be better to use string16 here since that is a similar string type.
> 
> eventually, we'll move these to be WebString (once these interfaces move into
> third_party/WebKit/WebKit/chromium/public), and so using string16 will reduce
> friction later.

Done throughout the 2 interfaces.

http://codereview.chromium.org/27157/diff/1188/530#newcode30
Line 30: DISALLOW_COPY_AND_ASSIGN(WebWorkerContextProxy);
On 2009/02/27 16:12:53, darin wrote:
> this is unnecessary since WebWorkerContextProxy is not a concrete class.

Done.

http://codereview.chromium.org/27157/diff/1188/530#newcode38
Line 38: WebWorkerObjectProxy() { }
On 2009/02/27 16:12:53, darin wrote:
> ditto

Done.

http://codereview.chromium.org/27157/diff/1188/530#newcode47
Line 47: int destination,
On 2009/02/27 16:12:53, darin wrote:
> it's not obvious what destination, source, and level mean.  can they be
defined?

they are actually the int values of enums defined in WebCore.  I didn't want to
duplicate the enums, I think just using the int value and casting when
appropriate will save a lot of duplicate values that need to be updated in sync
with webkit.

http://codereview.chromium.org/27157/diff/1188/530#newcode53
Line 53: virtual void ReportPendingActivity(bool has_pending_activity) = 0;
On 2009/02/27 16:12:53, darin wrote:
> what does pending activity mean?

I'm actually not that familiar with how it's used.  This interface just mirrors
the WebKit one.  Jian can provide more info.

http://codereview.chromium.org/27157/diff/1188/530#newcode57
Line 57: DISALLOW_COPY_AND_ASSIGN(WebWorkerObjectProxy);
On 2009/02/27 16:12:53, darin wrote:
> ditto

Done.

http://codereview.chromium.org/27157/diff/1188/530#newcode62
Line 62: WebWorkerContextProxy* CreateWebWorkerContext(
On 2009/02/27 16:12:53, darin wrote:
> nit: how about making this a static 'Create' method on WebWorkerContextProxy?

Done.