Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(56)

Issue 196503005: Make DevTools support for the embedded SharedWorker. (Closed)

Created:
6 years, 9 months ago by horo
Modified:
6 years, 8 months ago
CC:
chromium-reviews, vsevik, jam, yurys, joi+watch-content_chromium.org, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Make DevTools support for the embedded SharedWorker. BUG=327256 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261314

Patch Set 1 : #

Patch Set 2 : rebase #

Patch Set 3 : Introduce SharedWorkerDevToolsManager #

Total comments: 11

Patch Set 4 : incorporated yurys's comment #

Patch Set 5 : rebase #

Patch Set 6 : fix error #

Patch Set 7 : #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Patch Set 10 : add comments #

Total comments: 24

Patch Set 11 : incorporated kinuko's comment #

Total comments: 2

Patch Set 12 : rename Worker*OnUI to NotifyWorker*OnUI #

Total comments: 16

Patch Set 13 : incorporated yurys's comment #

Patch Set 14 : http://crrev.com/213423008 has landed. So I rebased. #

Patch Set 15 : http://crrev.com/214343002 has landed. So I rebased. #

Total comments: 7

Patch Set 16 : introduce WorkerState enum and add test #

Total comments: 24

Patch Set 17 : incorporated kinuko's and kinuko's comments #

Patch Set 18 : fix UAF bug #

Unified diffs Side-by-side diffs Delta from patch set Stats (+659 lines, -515 lines) Patch
A + content/browser/devtools/shared_worker_devtools_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +65 lines, -47 lines 0 comments Download
A + content/browser/devtools/shared_worker_devtools_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +178 lines, -376 lines 0 comments Download
A + content/browser/devtools/shared_worker_devtools_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +230 lines, -43 lines 0 comments Download
M content/browser/devtools/worker_devtools_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +3 lines, -3 lines 0 comments Download
M content/browser/devtools/worker_devtools_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +10 lines, -3 lines 0 comments Download
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 3 chunks +6 lines, -6 lines 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 5 chunks +34 lines, -10 lines 0 comments Download
M content/browser/shared_worker/shared_worker_instance.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +4 lines, -0 lines 0 comments Download
M content/browser/shared_worker/shared_worker_instance.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +14 lines, -2 lines 0 comments Download
M content/browser/shared_worker/shared_worker_service_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +12 lines, -5 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 12 13 14 15 16 7 chunks +84 lines, -16 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 3 chunks +7 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/shared_worker/embedded_shared_worker_stub.h View 1 2 3 4 5 6 7 8 9 10 11 12 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 7 8 9 10 11 12 1 chunk +7 lines, -4 lines 0 comments Download

Messages

Total messages: 40 (0 generated)
horo
kinuko@ Could you please review?
6 years, 9 months ago (2014-03-12 06:48:29 UTC) #1
pfeldman
I was hoping that we would not need worker manager operating on the IO thread ...
6 years, 9 months ago (2014-03-12 08:05:44 UTC) #2
horo
On 2014/03/12 08:05:44, pfeldman wrote: > I was hoping that we would not need worker ...
6 years, 9 months ago (2014-03-12 13:37:47 UTC) #3
horo
pfeldman@ I'm planning to introduce SharedWorkerDehvToolsManager which lives in UI thread. Please see "Patch Set ...
6 years, 9 months ago (2014-03-13 08:07:26 UTC) #4
yurys
https://codereview.chromium.org/196503005/diff/100001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/100001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode162 content/browser/devtools/shared_worker_devtools_manager.cc:162: return; Looks like you need to remove the id ...
6 years, 9 months ago (2014-03-13 09:06:51 UTC) #5
horo
Thank you very much for your review. https://codereview.chromium.org/196503005/diff/100001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/100001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode162 content/browser/devtools/shared_worker_devtools_manager.cc:162: return; On ...
6 years, 9 months ago (2014-03-14 08:53:19 UTC) #6
horo
yurys@ Ping?
6 years, 9 months ago (2014-03-20 08:40:50 UTC) #7
kinuko
https://codereview.chromium.org/196503005/diff/240001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/240001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode9 content/browser/devtools/shared_worker_devtools_manager.cc:9: #include "base/memory/singleton.h" nit: we already have this in .h ...
6 years, 9 months ago (2014-03-24 12:24:01 UTC) #8
horo
https://codereview.chromium.org/196503005/diff/240001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/240001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode9 content/browser/devtools/shared_worker_devtools_manager.cc:9: #include "base/memory/singleton.h" On 2014/03/24 12:24:02, kinuko wrote: > nit: ...
6 years, 9 months ago (2014-03-25 06:28:17 UTC) #9
kinuko
lgtm https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_worker/shared_worker_host.cc File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_worker/shared_worker_host.cc#newcode32 content/browser/shared_worker/shared_worker_host.cc:32: void WorkerCreatedOnUI(int worker_process_id, (minor nit) I prefer naming ...
6 years, 9 months ago (2014-03-25 13:08:03 UTC) #10
horo
https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_worker/shared_worker_host.cc File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_worker/shared_worker_host.cc#newcode32 content/browser/shared_worker/shared_worker_host.cc:32: void WorkerCreatedOnUI(int worker_process_id, On 2014/03/25 13:08:04, kinuko wrote: > ...
6 years, 9 months ago (2014-03-26 01:55:14 UTC) #11
horo
6 years, 9 months ago (2014-03-26 01:55:16 UTC) #12
horo
yurys@chromium.org Could you please review this CL? I want to land this CL before the ...
6 years, 9 months ago (2014-03-26 06:23:54 UTC) #13
yurys
https://codereview.chromium.org/196503005/diff/100001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/100001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode259 content/browser/devtools/shared_worker_devtools_manager.cc:259: if (url.GetOrigin() != it->second->worker_url.GetOrigin()) On 2014/03/14 08:53:20, horo wrote: ...
6 years, 9 months ago (2014-03-26 13:11:32 UTC) #14
alecflett
yurys: ping. I'm kind of blocked on this CL as well.. I'm doing the same ...
6 years, 9 months ago (2014-03-28 17:21:00 UTC) #15
pfeldman
@alecflett: i can see comment #14 with yurys's comments. or is there anything else you ...
6 years, 9 months ago (2014-03-28 17:24:56 UTC) #16
horo
alecflett@ I'm sorry to have kept you waiting. yurys@ I changed this cl. * Stop ...
6 years, 9 months ago (2014-03-29 06:53:38 UTC) #17
yurys
https://codereview.chromium.org/196503005/diff/570001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/570001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode121 content/browser/devtools/shared_worker_devtools_manager.cc:121: const SharedWorkerInstance instance_; Is it OK to store it ...
6 years, 8 months ago (2014-03-31 13:48:23 UTC) #18
horo
https://codereview.chromium.org/196503005/diff/570001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/570001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode121 content/browser/devtools/shared_worker_devtools_manager.cc:121: const SharedWorkerInstance instance_; On 2014/03/31 13:48:23, yurys wrote: > ...
6 years, 8 months ago (2014-04-01 08:34:08 UTC) #19
yurys
LGTM! https://codereview.chromium.org/196503005/diff/570001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/570001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode235 content/browser/devtools/shared_worker_devtools_manager.cc:235: for (WorkerInfoMap::iterator paused_it = paused_workers_.begin(); On 2014/04/01 08:34:09, ...
6 years, 8 months ago (2014-04-01 09:01:44 UTC) #20
kinuko
Some nits https://codereview.chromium.org/196503005/diff/590001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/590001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode40 content/browser/devtools/shared_worker_devtools_manager.cc:40: SharedWorkerDevToolsAgentHost(WorkerId worker_id) nit: explicit https://codereview.chromium.org/196503005/diff/590001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode75 content/browser/devtools/shared_worker_devtools_manager.cc:75: host->AddRoute(worker_id_.second, ...
6 years, 8 months ago (2014-04-01 16:01:05 UTC) #21
horo
https://codereview.chromium.org/196503005/diff/590001/content/browser/devtools/shared_worker_devtools_manager.cc File content/browser/devtools/shared_worker_devtools_manager.cc (right): https://codereview.chromium.org/196503005/diff/590001/content/browser/devtools/shared_worker_devtools_manager.cc#newcode40 content/browser/devtools/shared_worker_devtools_manager.cc:40: SharedWorkerDevToolsAgentHost(WorkerId worker_id) On 2014/04/01 16:01:05, kinuko wrote: > nit: ...
6 years, 8 months ago (2014-04-02 04:31:57 UTC) #22
horo
jochen@ Could you please review content/renderer/render_thread_impl.cc in this cl? Thank you.
6 years, 8 months ago (2014-04-02 06:18:47 UTC) #23
jochen (gone - plz use gerrit)
lgtm
6 years, 8 months ago (2014-04-02 09:27:07 UTC) #24
horo
The CQ bit was checked by horo@chromium.org
6 years, 8 months ago (2014-04-02 13:36:38 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001
6 years, 8 months ago (2014-04-02 13:37:11 UTC) #26
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-02 18:15:22 UTC) #27
commit-bot: I haz the power
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_aosp&number=60432
6 years, 8 months ago (2014-04-02 18:15:22 UTC) #28
horo
The CQ bit was checked by horo@chromium.org
6 years, 8 months ago (2014-04-02 22:12:12 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001
6 years, 8 months ago (2014-04-02 22:14:58 UTC) #30
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-03 03:22:58 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
6 years, 8 months ago (2014-04-03 03:22:59 UTC) #32
horo
The CQ bit was checked by horo@chromium.org
6 years, 8 months ago (2014-04-03 04:04:13 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001
6 years, 8 months ago (2014-04-03 04:05:18 UTC) #34
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-03 06:47:15 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
6 years, 8 months ago (2014-04-03 06:47:16 UTC) #36
horo
The CQ bit was checked by horo@chromium.org
6 years, 8 months ago (2014-04-03 07:11:48 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001
6 years, 8 months ago (2014-04-03 07:12:22 UTC) #38
commit-bot: I haz the power
Change committed as 261314
6 years, 8 months ago (2014-04-03 07:55:23 UTC) #39
horo
6 years, 8 months ago (2014-04-03 08:52:06 UTC) #40
Message was sent while issue was closed.
A revert of this CL has been created in
https://codereview.chromium.org/223583002/ by horo@chromium.org.

The reason for reverting is: buildbot failure in Chromium Memory on Linux
ASan+LSan Tests

SharedWorkerDevToolsManagerTest.AttachTest test causes memory leaks..

Powered by Google App Engine
This is Rietveld 408576698