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

Issue 2586863002: Worker: Enable UseCounter for SharedWorkerGlobalScope (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months ago by nhiroki
Modified:
6 months, 1 week ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kinuko+watch, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org, kenjibaheux
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo8c/edit#heading=h.g1fo4m6ayjxf This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, when a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 Review-Url: https://codereview.chromium.org/2586863002 Cr-Commit-Position: refs/heads/master@{#449196} Committed: https://chromium.googlesource.com/chromium/src/+/efd1adc4978287cb9f36a91694fc21c482f996e1

Patch Set 1 #

Patch Set 2 : WIP #

Patch Set 3 : rebase #

Patch Set 4 : rebase #

Patch Set 5 : WIP #

Patch Set 6 : WIP #

Patch Set 7 : remove countDeprecation code path #

Patch Set 8 : fix test failures #

Patch Set 9 : fix win compile failures #

Patch Set 10 : add unit tests #

Patch Set 11 : ready to review #

Total comments: 19

Patch Set 12 : address review comments #

Total comments: 6

Patch Set 13 : rebase #

Patch Set 14 : window.open() #

Patch Set 15 : clean up #

Patch Set 16 : add a missing file #

Patch Set 17 : replace ExecutionContext with ScriptState #

Total comments: 4

Patch Set 18 : address review comments and fix styles #

Patch Set 19 : rename "feature" to "useCounterId" for consistency #

Patch Set 20 : tweak #

Total comments: 2

Patch Set 21 : rename use_counter to used_features #

Patch Set 22 : rebase #

Patch Set 23 : simplify #

Unified diffs Side-by-side diffs Delta from patch set Stats (+353 lines, -31 lines) Patch
M content/browser/shared_worker/shared_worker_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +7 lines, -1 line 0 comments Download
M content/browser/shared_worker/shared_worker_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +18 lines, -1 line 0 comments Download
M content/browser/shared_worker/shared_worker_message_filter.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/shared_worker/shared_worker_service_impl.h View 1 2 3 4 5 6 7 8 9 19 20 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/shared_worker/shared_worker_service_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 19 20 2 chunks +13 lines, -0 lines 0 comments Download
M content/browser/shared_worker/shared_worker_service_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 8 chunks +82 lines, -4 lines 0 comments Download
M content/common/view_messages.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 +13 lines, -1 line 0 comments Download
M content/common/worker_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +8 lines, -0 lines 0 comments Download
M content/renderer/shared_worker/embedded_shared_worker_stub.h View 1 2 3 4 5 6 19 20 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/shared_worker/embedded_shared_worker_stub.cc View 1 2 3 4 5 6 19 20 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/shared_worker/shared_worker_repository.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/shared_worker/websharedworker_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +8 lines, -6 lines 0 comments Download
M content/renderer/shared_worker/websharedworker_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +13 lines, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +11 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter-window.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +11 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +107 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/UseCounter.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, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/UseCounter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/testing/Internals.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/testing/Internals.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/testing/Internals.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/testing/WorkerInternals.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 19 20 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/testing/WorkerInternals.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 19 20 2 chunks +14 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/testing/WorkerInternals.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/SharedWorkerRepositoryClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +10 lines, -8 lines 0 comments Download
M third_party/WebKit/public/web/WebSharedWorkerClient.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebSharedWorkerConnectListener.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 112 (79 generated)
Rick Byers
This seems like a reasonable start, thanks! A few high-level thoughts: 1. There should be ...
7 months, 2 weeks ago (2017-01-02 21:46:40 UTC) #2
Rick Byers
On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote: > This seems like a reasonable ...
7 months, 2 weeks ago (2017-01-02 22:01:45 UTC) #3
Rick Byers
On 2017/01/02 22:01:45, Rick Byers wrote: > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 ...
7 months, 1 week ago (2017-01-06 19:50:03 UTC) #4
nhiroki
Thank you for your early comments and sorry for my delayed responses. I think this ...
6 months, 3 weeks ago (2017-01-24 08:45:47 UTC) #30
nhiroki
+horo, kinuko, can you review this? Thanks.
6 months, 3 weeks ago (2017-01-24 08:46:38 UTC) #36
nhiroki
https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html (right): https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html#newcode71 third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:71: assert_true(isUseCounted(frame3, FetchFeature)); Question for rbyers@: Do iframes in a ...
6 months, 3 weeks ago (2017-01-24 09:54:11 UTC) #37
kinuko
https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_use_counter.cc File content/common/worker_use_counter.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_use_counter.cc#newcode17 content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes = kNumberOfFeatures / kBitsPerByte; Do we ...
6 months, 3 weeks ago (2017-01-24 13:56:31 UTC) #40
nhiroki
Thank you for the comments. Comment reply only; I'll address them tomorrow. https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_use_counter.cc File content/common/worker_use_counter.cc ...
6 months, 3 weeks ago (2017-01-24 15:07:30 UTC) #42
horo
https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared_worker/shared_worker_service_impl.cc File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared_worker/shared_worker_service_impl.cc#newcode353 content/browser/shared_worker/shared_worker_service_impl.cc:353: ScopedWorkerDependencyChecker checker(this); You don't need this? https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared_worker/shared_worker_service_impl.cc#newcode355 content/browser/shared_worker/shared_worker_service_impl.cc:355: FindSharedWorkerHost(filter->render_process_id(), ...
6 months, 3 weeks ago (2017-01-25 03:45:10 UTC) #43
horo
6 months, 3 weeks ago (2017-01-25 03:45:14 UTC) #44
kinuko
https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_use_counter.cc File content/common/worker_use_counter.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_use_counter.cc#newcode17 content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes = kNumberOfFeatures / kBitsPerByte; On 2017/01/24 ...
6 months, 3 weeks ago (2017-01-25 05:12:52 UTC) #45
kinuko
On 2017/01/25 05:12:52, kinuko wrote: > https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_use_counter.cc > File content/common/worker_use_counter.cc (right): > > https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_use_counter.cc#newcode17 > ...
6 months, 3 weeks ago (2017-01-25 07:47:47 UTC) #46
nhiroki
Thanks for your comments. Updated. https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared_worker/shared_worker_service_impl.cc File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared_worker/shared_worker_service_impl.cc#newcode353 content/browser/shared_worker/shared_worker_service_impl.cc:353: ScopedWorkerDependencyChecker checker(this); On 2017/01/25 ...
6 months, 3 weeks ago (2017-01-25 10:16:12 UTC) #49
kinuko
lgtm
6 months, 3 weeks ago (2017-01-25 12:32:41 UTC) #52
horo
lgtm
6 months, 3 weeks ago (2017-01-26 01:52:36 UTC) #53
Rick Byers
On 2017/01/24 08:45:47, nhiroki wrote: > Thank you for your early comments and sorry for ...
6 months, 3 weeks ago (2017-01-26 02:28:06 UTC) #54
Rick Byers
On 2017/01/24 09:54:11, nhiroki wrote: > https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html > File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html > (right): > > https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html#newcode71 ...
6 months, 3 weeks ago (2017-01-26 02:30:35 UTC) #55
Rick Byers
This looks good, thank you! Just a couple issues with the test. https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js File third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js ...
6 months, 3 weeks ago (2017-01-26 03:02:37 UTC) #56
nhiroki
Thanks for your comments. Updated. https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js File third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js (right): https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js#newcode5 third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js:5: fetch('notfound').catch(() => port.postMessage('fetched')); On ...
6 months, 3 weeks ago (2017-01-27 16:58:16 UTC) #63
nhiroki
On 2017/01/26 02:28:06, Rick Byers wrote: > On 2017/01/24 08:45:47, nhiroki wrote: > > Thank ...
6 months, 3 weeks ago (2017-01-27 17:05:04 UTC) #64
nhiroki
On 2017/01/26 02:30:35, Rick Byers wrote: > On 2017/01/24 09:54:11, nhiroki wrote: > > > ...
6 months, 3 weeks ago (2017-01-27 17:06:39 UTC) #65
nhiroki
+dcheng@, can you review following files as an owner? - content/common/view_messages.h - content/common/worker_messages.h - third_party/WebKit/Source/web/* ...
6 months, 3 weeks ago (2017-01-27 17:09:13 UTC) #67
dcheng
https://codereview.chromium.org/2586863002/diff/340001/content/renderer/shared_worker/websharedworker_proxy.h File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2586863002/diff/340001/content/renderer/shared_worker/websharedworker_proxy.h#newcode48 content/renderer/shared_worker/websharedworker_proxy.h:48: void OnWorkerConnected(std::set<uint32_t> features); Nit: const std::set<uint32_t>& https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp ...
6 months, 2 weeks ago (2017-02-03 00:40:41 UTC) #78
nhiroki
Thanks for your comments. Updated. https://codereview.chromium.org/2586863002/diff/340001/content/renderer/shared_worker/websharedworker_proxy.h File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2586863002/diff/340001/content/renderer/shared_worker/websharedworker_proxy.h#newcode48 content/renderer/shared_worker/websharedworker_proxy.h:48: void OnWorkerConnected(std::set<uint32_t> features); On ...
6 months, 2 weeks ago (2017-02-03 17:28:22 UTC) #85
dcheng
LGTM with a naming nit. I know the latest PS renamed feature -> useCounterId, but ...
6 months, 2 weeks ago (2017-02-03 18:27:50 UTC) #86
nhiroki
https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared_worker/shared_worker_host.h File content/browser/shared_worker/shared_worker_host.h (right): https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared_worker/shared_worker_host.h#newcode146 content/browser/shared_worker/shared_worker_host.h:146: std::set<uint32_t> use_counter_; On 2017/02/03 18:27:50, dcheng wrote: > Nit: ...
6 months, 1 week ago (2017-02-06 00:35:57 UTC) #90
nhiroki
Thank you. On 2017/02/03 18:27:50, dcheng wrote: > LGTM with a naming nit. > > ...
6 months, 1 week ago (2017-02-06 00:37:07 UTC) #93
nhiroki
rbyers@, ping :)
6 months, 1 week ago (2017-02-06 00:39:22 UTC) #94
Rick Byers
Shoot, sorry I missed this. The test looks awesome now, thank you! LGTM
6 months, 1 week ago (2017-02-09 01:43:45 UTC) #103
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2586863002/460001
6 months, 1 week ago (2017-02-09 01:54:17 UTC) #106
commit-bot: I haz the power
Committed patchset #23 (id:460001) as https://chromium.googlesource.com/chromium/src/+/efd1adc4978287cb9f36a91694fc21c482f996e1
6 months, 1 week ago (2017-02-09 03:57:41 UTC) #109
darin (slow to review)
It looks like the tests here assume that MessagePort communication shares the same FIFO as ...
6 months, 1 week ago (2017-02-09 20:33:59 UTC) #111
nhiroki
6 months, 1 week ago (2017-02-10 00:26:00 UTC) #112
Message was sent while issue was closed.
On 2017/02/09 20:33:59, darin (slow to review) wrote:
> It looks like the tests here assume that MessagePort communication shares the
> same FIFO as Chrome IPC messages. That happens to be true in the current
> implementation. I'm trying to make it untrue in a new implementation here:
> https://codereview.chromium.org/2422793002
> 
> There are performance benefits to doing so. I'm trying to figure out the best
> way to support the use counter tests here that rely on the use counter IPCs
> happening before MessagePort IPCs. Suggestions welcome :)

Please see: https://codereview.chromium.org/2658603003/#msg97

I'll make a patch for a mechanism to observe UseCounter changes.
Sign in to reply to this message.

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