Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(36)

Issue 27157: Initial checkin of the out of process worker implementation.... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 2 months ago by jam
Modified:
5 years, 12 months ago
CC:
chromium-reviews_googlegroups.com, jianli
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : compile on linux #

Patch Set 5 : '' #

Patch Set 6 : fix routing problem #

Patch Set 7 : fix routing problem #

Patch Set 8 : fix crash #

Patch Set 9 : use the right stack size for v8 #

Total comments: 38

Patch Set 10 : '' #

Patch Set 11 : separate WebWorkerClient into its own header #

Patch Set 12 : added missing includes #

Patch Set 13 : '' #

Patch Set 14 : fix compile problem on linux #

Patch Set 15 : sync #

Patch Set 16 : fix compile when workers aren't enabled #

Patch Set 17 : add xcodeproj changes #

Patch Set 18 : use gyp instead of xcodeproj #

Patch Set 19 : '' #

Patch Set 20 : '' #

Patch Set 21 : '' #

Patch Set 22 : '' #

Patch Set 23 : '' #

Patch Set 24 : '' #

Patch Set 25 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1757 lines, -64 lines) Patch
M chrome/SConscript View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/app/chrome_dll_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/browser.scons View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/browser.vcproj View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +20 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 4 chunks +17 lines, -0 lines 0 comments Download
A chrome/browser/worker_host/worker_process_host.h View 19 20 1 chunk +56 lines, -0 lines 0 comments Download
A chrome/browser/worker_host/worker_process_host.cc View 19 20 1 chunk +99 lines, -0 lines 0 comments Download
A chrome/browser/worker_host/worker_service.h View 19 20 1 chunk +52 lines, -0 lines 0 comments Download
A chrome/browser/worker_host/worker_service.cc View 19 20 1 chunk +73 lines, -0 lines 0 comments Download
M chrome/chrome.gyp View 18 19 20 21 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/chrome.sln View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 4 chunks +16 lines, -0 lines 0 comments Download
M chrome/common/child_process_host.cc View 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/common/child_thread.h View 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/child_thread.cc View 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +4 lines, -1 line 0 comments Download
M chrome/common/common.scons View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/common.vcproj View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/common/ipc_message_utils.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 7 chunks +95 lines, -56 lines 0 comments Download
M chrome/common/render_messages_internal.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +12 lines, -0 lines 0 comments Download
A chrome/common/worker_messages.h View 1 chunk +21 lines, -0 lines 0 comments Download
A chrome/common/worker_messages_internal.h View 1 2 3 4 5 6 7 8 9 1 chunk +73 lines, -0 lines 0 comments Download
M chrome/renderer/render_thread.cc View 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/renderer/render_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/render_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/renderer/renderer.scons View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/renderer/renderer.vcproj View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +8 lines, -0 lines 0 comments Download
A chrome/renderer/webworker_proxy.h View 1 chunk +56 lines, -0 lines 0 comments Download
A chrome/renderer/webworker_proxy.cc View 10 1 chunk +92 lines, -0 lines 0 comments Download
A chrome/worker/webworkerclient_proxy.h View 10 1 chunk +68 lines, -0 lines 0 comments Download
A chrome/worker/webworkerclient_proxy.cc View 1 chunk +95 lines, -0 lines 0 comments Download
A chrome/worker/worker.scons View 1 2 3 4 5 6 7 8 9 1 chunk +102 lines, -0 lines 0 comments Download
A chrome/worker/worker.vcproj View 1 2 3 4 5 6 7 8 9 1 chunk +171 lines, -0 lines 0 comments Download
A chrome/worker/worker_main.cc View 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/worker/worker_process.h View 1 chunk +28 lines, -0 lines 0 comments Download
A chrome/worker/worker_process.cc View 1 chunk +14 lines, -0 lines 0 comments Download
A chrome/worker/worker_thread.h View 1 chunk +31 lines, -0 lines 0 comments Download
A chrome/worker/worker_thread.cc View 1 2 3 4 5 6 7 8 9 1 chunk +26 lines, -0 lines 0 comments Download
M third_party/WebKit/WebCore/workers/WorkerMessagingProxy.cpp View 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +0 lines, -1 line 0 comments Download
M webkit/glue/glue.vcproj View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +24 lines, -0 lines 0 comments Download
M webkit/glue/webview_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +7 lines, -0 lines 0 comments Download
A webkit/glue/webworker.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +31 lines, -0 lines 0 comments Download
A webkit/glue/webworker_impl.h View 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +57 lines, -0 lines 0 comments Download
A webkit/glue/webworker_impl.cc View 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +91 lines, -0 lines 0 comments Download
A webkit/glue/webworkerclient.h View 11 12 13 14 15 16 17 18 19 20 1 chunk +36 lines, -0 lines 0 comments Download
A webkit/glue/webworkerclient_impl.h View 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +61 lines, -0 lines 0 comments Download
A webkit/glue/webworkerclient_impl.cc View 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +128 lines, -0 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 5 (0 generated)
jam
8 years, 2 months ago (2009-02-25 20:47:04 UTC) #1
jianli
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: ...
8 years, 2 months ago (2009-02-25 21:13:31 UTC) #2
jam
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 ...
8 years, 2 months ago (2009-02-25 22:15:51 UTC) #3
darin (slow to review)
Nice work! I really like how much simpler this design is than the one we ...
8 years, 2 months ago (2009-02-27 16:12:53 UTC) #4
jam
8 years, 2 months ago (2009-02-28 00:56:51 UTC) #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06