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

Issue 390017: Added lifecycle management and sharing support for SharedWorkers. SharedWorkers (Closed)

Created:
11 years ago by Andrew T Wilson (Slow)
Modified:
9 years, 6 months ago
Reviewers:
jam
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

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 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31868

Patch Set 1 #

Patch Set 2 : Moved log statement inside if statement #

Total comments: 8

Patch Set 3 : Review feedback #

Patch Set 4 : Changed shared worker messages to use a routing id #

Patch Set 5 : Self-reviewed code again, cleaned up some style nits #

Patch Set 6 : Changed WebWorkerBase not not call a virtual function from the destructor #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+552 lines, -77 lines) Patch
M chrome/browser/renderer_host/resource_message_filter.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 1 2 3 2 chunks +22 lines, -3 lines 0 comments Download
M chrome/browser/worker_host/worker_process_host.h View 1 2 3 4 3 chunks +50 lines, -5 lines 2 comments Download
M chrome/browser/worker_host/worker_process_host.cc View 1 2 3 4 6 chunks +140 lines, -12 lines 4 comments Download
M chrome/browser/worker_host/worker_service.h View 1 2 3 3 chunks +35 lines, -3 lines 0 comments Download
M chrome/browser/worker_host/worker_service.cc View 1 2 3 4 9 chunks +195 lines, -16 lines 3 comments Download
M chrome/common/render_messages_internal.h View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
M chrome/common/worker_messages_internal.h View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/renderer/render_view.cc View 1 2 3 4 1 chunk +12 lines, -6 lines 0 comments Download
M chrome/renderer/websharedworker_proxy.h View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/renderer/websharedworker_proxy.cc View 1 2 3 4 4 chunks +5 lines, -5 lines 0 comments Download
M chrome/renderer/websharedworkerrepository_impl.h View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/renderer/websharedworkerrepository_impl.cc View 1 chunk +12 lines, -9 lines 1 comment Download
M chrome/renderer/webworker_base.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/webworker_proxy.h View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/renderer/webworker_proxy.cc View 1 2 3 4 2 chunks +7 lines, -4 lines 0 comments Download
M chrome/worker/websharedworker_stub.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/worker/websharedworker_stub.cc View 1 2 3 4 3 chunks +9 lines, -1 line 0 comments Download
M chrome/worker/webworkerclient_proxy.cc View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M chrome/worker/worker_uitest.cc View 1 chunk +6 lines, -6 lines 0 comments Download
M webkit/api/src/SharedWorkerRepository.cpp View 1 chunk +3 lines, -0 lines 0 comments Download
A webkit/data/layout_tests/platform/chromium-win/LayoutTests/fast/workers/shared-worker-exception-expected.txt View 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
Andrew T Wilson (Slow)
I'm including SharedWorkerRepository.cpp in this patch for completeness - note that this file has sense ...
11 years ago (2009-11-11 20:15:00 UTC) #1
Andrew T Wilson (Slow)
Reviewers: John Abd-El-Malek, Message: I'm including SharedWorkerRepository.cpp in this patch for completeness - note that ...
11 years ago (2009-11-11 20:15:18 UTC) #2
jam
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 ...
11 years ago (2009-11-11 22:18:54 UTC) #3
Andrew T Wilson (Slow)
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 ...
11 years ago (2009-11-11 23:34:51 UTC) #4
Andrew T Wilson (Slow)
OK, ready for another round. Per our recent discussion, I changed how shared worker messages ...
11 years ago (2009-11-12 07:44:40 UTC) #5
jam
11 years ago (2009-11-12 20:11:23 UTC) #6
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

Powered by Google App Engine
This is Rietveld 408576698