Added lifecycle management and sharing support for SharedWorkers. SharedWorkers

Added lifecycle management and sharing support for SharedWorkers. SharedWorkers
can now outlive their parent pages and can be shared by multiple instances
across multiple tabs.

BUG=26233
TEST=ui tests

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

[Andrew T Wilson (Slow), 2009-11-11 20:15:00]

I'm including SharedWorkerRepository.cpp in this patch for completeness - note
that this file has sense been upstreamed so I'll check it in to the WebKit repo
instead.

[Andrew T Wilson (Slow), 2009-11-11 20:15:18]

Reviewers: John Abd-El-Malek,

Message:
I'm including SharedWorkerRepository.cpp in this patch for completeness -  
note
that this file has sense been upstreamed so I'll check it in to the WebKit  
repo
instead.

Description:
Added lifecycle management and sharing support for SharedWorkers.  
SharedWorkers
can now outlive their parent pages and can be shared by multiple instances
across multiple tabs.

BUG=26233
TEST=ui tests

Please review this at http://codereview.chromium.org/390017

Affected files:
   M chrome/browser/renderer_host/resource_message_filter.h
   M chrome/browser/renderer_host/resource_message_filter.cc
   M chrome/browser/worker_host/worker_process_host.h
   M chrome/browser/worker_host/worker_process_host.cc
   M chrome/browser/worker_host/worker_service.h
   M chrome/browser/worker_host/worker_service.cc
   M chrome/common/render_messages_internal.h
   M chrome/common/worker_messages_internal.h
   M chrome/renderer/render_view.cc
   M chrome/renderer/websharedworker_proxy.h
   M chrome/renderer/websharedworker_proxy.cc
   M chrome/renderer/websharedworkerrepository_impl.h
   M chrome/renderer/websharedworkerrepository_impl.cc
   M chrome/renderer/webworker_base.h
   M chrome/renderer/webworker_base.cc
   M chrome/renderer/webworker_proxy.h
   M chrome/renderer/webworker_proxy.cc
   M chrome/worker/websharedworker_stub.h
   M chrome/worker/websharedworker_stub.cc
   M chrome/worker/webworkerclient_proxy.cc
   M chrome/worker/worker_uitest.cc
   M webkit/api/src/SharedWorkerRepository.cpp
   A  
webkit/data/layout_tests/platform/chromium-win/LayoutTests/fast/workers/shared-worker-exception-expected.txt

[jam, 2009-11-11 22:18:54]

http://codereview.chromium.org/390017/diff/3001/3002
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/390017/diff/3001/3002#newcode685
Line 685: if (is_shared)
I don't understand why forwarding a message needs to be different?  i.e. why
does url+name need to be used for routing messages to shared workers.  Can't
WorkerProcessHost just use the routing id of the message as the signal to which
worker instance this is destined to, just like we do for dedicated workers? 
Doing so would simply a bunch of these codepaths.

http://codereview.chromium.org/390017/diff/3001/3006
File chrome/browser/worker_host/worker_service.cc (right):

http://codereview.chromium.org/390017/diff/3001/3006#newcode154
Line 154: *url_mismatch = false;
nit: need brace brackets

http://codereview.chromium.org/390017/diff/3001/3006#newcode188
Line 188: iter = pending_shared_workers_.erase(iter);
nit: here and below, should use brace brackets to match existing style

http://codereview.chromium.org/390017/diff/3001/3010
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/390017/diff/3001/3010#newcode1816
Line 1816: else
nit: need brace brackets for multi-line statements

http://codereview.chromium.org/390017/diff/3001/3015
File chrome/renderer/webworker_base.cc (right):

http://codereview.chromium.org/390017/diff/3001/3015#newcode74
Line 74: // (meaning we've sent the WorkerMsg_StartWorkerContext already).
update the comment as well

http://codereview.chromium.org/390017/diff/3001/3019
File chrome/worker/websharedworker_stub.cc (right):

http://codereview.chromium.org/390017/diff/3001/3019#newcode41
Line 41: if (!started_)
nit: the convention is usually to early return when something has already been
done

[Andrew T Wilson (Slow), 2009-11-11 23:34:51]

http://codereview.chromium.org/390017/diff/3001/3002
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/390017/diff/3001/3002#newcode685
Line 685: if (is_shared)
The reason is that many different renderers can talk to the same SharedWorker,
and there's only a single WorkerInstance, and hence only a single renderer
routing id.

To route via routing IDs, we would have to allocate render-side routing IDs for
all of these clients and track multiple render-side routing IDs in each
WorkerInstance, which is a much more invasive change and isn't necessary when we
aren't creating a new worker.

Instead, we just route messages to shared workers via url + name - much simpler,
and allows us to only use the routing ID during worker creation and lets us
leverage the lookup code we have to write anyhow.

http://codereview.chromium.org/390017/diff/3001/3006
File chrome/browser/worker_host/worker_service.cc (right):

http://codereview.chromium.org/390017/diff/3001/3006#newcode154
Line 154: *url_mismatch = false;
On 2009/11/11 22:18:54, John Abd-El-Malek wrote:
> nit: need brace brackets
Sigh, going back and forth between google3 and webkit makes my brain spin.

[Andrew T Wilson (Slow), 2009-11-12 07:44:40]

OK, ready for another round.

Per our recent discussion, I changed how shared worker messages are forwarded
(each WorkerInstance manages a set of route_ids now).

I also got rid of WorkerInstance.sender_id and changed all the uses to pass a
real Sender instead of the id.

[jam, 2009-11-12 20:11:23]

lgtm with below

http://codereview.chromium.org/390017/diff/1049/1052
File chrome/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/390017/diff/1049/1052#newcode183
Line 183: // being sent to the shared worker (ignored for dedicated workers).
nit: the comment says it's ignored for dedicated workers, but it's used above
for FilterMessage.

http://codereview.chromium.org/390017/diff/1049/1052#newcode204
Line 204: OnForwardToWorker)
can you add the new messages that you receive in RMF here as well?  it's a small
bit of work to make the code still handle nested workers in case we use it in
the future

http://codereview.chromium.org/390017/diff/1049/1052#newcode383
Line 383: // Walk all instances and remove the document from their document set
very nitty nit: period at end of comment

http://codereview.chromium.org/390017/diff/1049/1052#newcode444
Line 444: document_set.erase(i);
this isn't safe, need to increment i only if you don't erase, otherwise i =
document_set.erase(i).  same for line 466

http://codereview.chromium.org/390017/diff/1049/1053
File chrome/browser/worker_host/worker_process_host.h (right):

http://codereview.chromium.org/390017/diff/1049/1053#newcode20
Line 20: class WorkerInstance {
nit: If this is a class, then it's against the coding guidelines to have all the
member variables public (and without a "_" suffix).  Either keep it as a struct,
or add getters/setters.

http://codereview.chromium.org/390017/diff/1049/1053#newcode24
Line 24: // Checks if this WorkerInstance matches the passed url/name params
nit: just to make it clear to someone reading the code, can you add a small
comment that the methods below apply only to shared workers?

http://codereview.chromium.org/390017/diff/1049/1054
File chrome/browser/worker_host/worker_service.cc (right):

http://codereview.chromium.org/390017/diff/1049/1054#newcode84
Line 84: // tried to start up the worker simultaneously)
nit: period

http://codereview.chromium.org/390017/diff/1049/1054#newcode157
Line 157: if (found_instance) {
nit: chrome style is to not use brace brackets for one line statements

http://codereview.chromium.org/390017/diff/1049/1054#newcode174
Line 174: for (WorkerProcessHost::Instances::iterator i =
queued_workers_.begin();
nit: in the loop above and below, you use "iter", and here you use "i".  perhaps
use the same everywhere for consistency?

http://codereview.chromium.org/390017/diff/1049/1061
File chrome/renderer/websharedworkerrepository_impl.cc (right):

http://codereview.chromium.org/390017/diff/1049/1061#newcode20
Line 20: RenderThread::current()->Send(new
ViewHostMsg_DocumentDetached(document));
nit: if you make this ChildThread::, this file can be reused in the worker
process if we allow workers to create other workers