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

Issue 2848653003: Add a DevTools command to create an isolated world for a given frame (Closed)

Created:
3 years, 7 months ago by alex clarke (OOO till 29th)
Modified:
3 years, 7 months ago
Reviewers:
caseq, Sami, pfeldman
CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, blink-reviews-bindings_chromium.org, kozyatinskiy+blink_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a DevTools command to create an isolated world for a given frame DevTools already has the ability to run script in any execution context, it' just can't by itself create an isolated world. This patch adds that ability. We believe this will be useful for Headless chrome users who whish to manipulate the dom. They can already do this via devtools protocol but running a script in an isolated world should be easier to work with. BUG=546953 Review-Url: https://codereview.chromium.org/2848653003 Cr-Commit-Position: refs/heads/master@{#470870} Committed: https://chromium.googlesource.com/chromium/src/+/fa3fdf20cea4dbedd74476a40649e708ab1827a0

Patch Set 1 #

Total comments: 6

Patch Set 2 : Add missing expects #

Total comments: 1

Patch Set 3 : Changed so we don't pass in the id #

Total comments: 3

Patch Set 4 : Change the way isolated world ids are obtained #

Patch Set 5 : Tweaks #

Patch Set 6 : Remove pointless check #

Total comments: 6

Patch Set 7 : Changes for Sami #

Total comments: 10

Patch Set 8 : Changes for caseq #

Patch Set 9 : Fix test #

Total comments: 2

Patch Set 10 : Added WorldType::kInspectorIsolated as requested #

Patch Set 11 : Fix compile ;) #

Total comments: 2

Patch Set 12 : Inlined the function as requested #

Patch Set 13 : Rebased #

Messages

Total messages: 85 (51 generated)
alex clarke (OOO till 29th)
3 years, 7 months ago (2017-04-27 17:03:57 UTC) #3
pfeldman
That's something that all the automation clients were asking for, so great to have it! ...
3 years, 7 months ago (2017-04-27 19:25:42 UTC) #7
Sami
https://codereview.chromium.org/2848653003/diff/1/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2848653003/diff/1/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode517 third_party/WebKit/Source/core/inspector/browser_protocol.json:517: { "name": "worldId", "type": "integer", "description": "Id of the ...
3 years, 7 months ago (2017-04-28 10:12:57 UTC) #10
alex clarke (OOO till 29th)
PTAL https://codereview.chromium.org/2848653003/diff/1/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2848653003/diff/1/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode517 third_party/WebKit/Source/core/inspector/browser_protocol.json:517: { "name": "worldId", "type": "integer", "description": "Id of ...
3 years, 7 months ago (2017-04-28 21:18:44 UTC) #13
caseq
https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp#newcode386 third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:386: int world_id = 1; I think allocating ids like ...
3 years, 7 months ago (2017-04-28 21:41:31 UTC) #17
Sami
https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp#newcode386 third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:386: int world_id = 1; On 2017/04/28 21:41:30, caseq wrote: ...
3 years, 7 months ago (2017-05-02 09:47:19 UTC) #20
alex clarke (OOO till 29th)
https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp#newcode386 third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:386: int world_id = 1; On 2017/04/28 21:41:30, caseq wrote: ...
3 years, 7 months ago (2017-05-02 10:00:06 UTC) #21
caseq
On 2017/05/02 10:00:06, alex clarke wrote: > https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp > File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right): > > https://codereview.chromium.org/2848653003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp#newcode386 ...
3 years, 7 months ago (2017-05-02 21:37:00 UTC) #22
alex clarke (OOO till 29th)
On 2017/05/02 21:37:00, caseq wrote: > On 2017/05/02 10:00:06, alex clarke wrote: > > > ...
3 years, 7 months ago (2017-05-03 13:39:28 UTC) #25
caseq
On 2017/05/03 13:39:28, alex clarke wrote: > A generic content API would indeed be nice ...
3 years, 7 months ago (2017-05-03 22:51:23 UTC) #28
alex clarke (OOO till 29th)
On 2017/05/03 22:51:23, caseq wrote: > On 2017/05/03 13:39:28, alex clarke wrote: > > > ...
3 years, 7 months ago (2017-05-04 07:45:37 UTC) #29
alex clarke (OOO till 29th)
Please take another look, thanks!
3 years, 7 months ago (2017-05-05 08:05:52 UTC) #30
Sami
lgtm https://codereview.chromium.org/2848653003/diff/100001/third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html File third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html (right): https://codereview.chromium.org/2848653003/diff/100001/third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html#newcode43 third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html:43: { nit: move to end of previous line ...
3 years, 7 months ago (2017-05-05 09:33:14 UTC) #31
alex clarke (OOO till 29th)
Pavel PTAL :) https://codereview.chromium.org/2848653003/diff/100001/third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html File third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html (right): https://codereview.chromium.org/2848653003/diff/100001/third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html#newcode43 third_party/WebKit/LayoutTests/inspector-protocol/page/createIsolatedWorld.html:43: { On 2017/05/05 09:33:14, Sami wrote: ...
3 years, 7 months ago (2017-05-05 14:52:29 UTC) #33
pfeldman
lgtm % nits https://codereview.chromium.org/2848653003/diff/120001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.cpp File third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2848653003/diff/120001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.cpp#newcode289 third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.cpp:289: DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<int>, DCHECK(isMainThread()) + simpler static local ...
3 years, 7 months ago (2017-05-05 15:40:24 UTC) #35
caseq
https://codereview.chromium.org/2848653003/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2848653003/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode517 third_party/WebKit/Source/core/inspector/browser_protocol.json:517: { "name": "worldName", "type": "string", "description": "An optional name ...
3 years, 7 months ago (2017-05-05 16:36:02 UTC) #36
alex clarke (OOO till 29th)
PTAL https://codereview.chromium.org/2848653003/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2848653003/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode517 third_party/WebKit/Source/core/inspector/browser_protocol.json:517: { "name": "worldName", "type": "string", "description": "An optional ...
3 years, 7 months ago (2017-05-05 19:29:44 UTC) #40
caseq
On 2017/05/05 19:29:44, alex clarke wrote: > I suppose we could. We'll need to either ...
3 years, 7 months ago (2017-05-08 03:20:59 UTC) #44
pfeldman
https://codereview.chromium.org/2848653003/diff/160001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h File third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h (right): https://codereview.chromium.org/2848653003/diff/160001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h#newcode200 third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h:200: static int GetNextDevToolsIsolatedWorldId(); I would go all the way ...
3 years, 7 months ago (2017-05-08 20:27:13 UTC) #49
alex clarke (OOO till 29th)
https://codereview.chromium.org/2848653003/diff/160001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h File third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h (right): https://codereview.chromium.org/2848653003/diff/160001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h#newcode200 third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h:200: static int GetNextDevToolsIsolatedWorldId(); On 2017/05/08 20:27:12, pfeldman wrote: > ...
3 years, 7 months ago (2017-05-09 08:19:06 UTC) #52
alex clarke (OOO till 29th)
The API has changed a bit so I'll wait for final LGTM before submitting. PTAL ...
3 years, 7 months ago (2017-05-09 11:06:20 UTC) #57
pfeldman
https://codereview.chromium.org/2848653003/diff/200001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h File third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h (right): https://codereview.chromium.org/2848653003/diff/200001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h#newcode205 third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h:205: static int GetNextInspectorIsolatedWorldId(); do you still need it?
3 years, 7 months ago (2017-05-09 18:57:37 UTC) #60
alex clarke (OOO till 29th)
https://codereview.chromium.org/2848653003/diff/200001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h File third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h (right): https://codereview.chromium.org/2848653003/diff/200001/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h#newcode205 third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h:205: static int GetNextInspectorIsolatedWorldId(); On 2017/05/09 18:57:36, pfeldman wrote: > ...
3 years, 7 months ago (2017-05-09 19:40:36 UTC) #61
alex clarke (OOO till 29th)
Thanks all.
3 years, 7 months ago (2017-05-09 19:40:46 UTC) #62
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/2848653003/220001
3 years, 7 months ago (2017-05-09 19:41:56 UTC) #65
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/439857)
3 years, 7 months ago (2017-05-09 21:50:48 UTC) #67
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/2848653003/220001
3 years, 7 months ago (2017-05-10 07:10:35 UTC) #69
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/449685)
3 years, 7 months ago (2017-05-10 10:09:42 UTC) #71
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/2848653003/220001
3 years, 7 months ago (2017-05-10 11:26:00 UTC) #73
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/449954)
3 years, 7 months ago (2017-05-10 13:11:39 UTC) #75
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/2848653003/240001
3 years, 7 months ago (2017-05-10 16:39:57 UTC) #78
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/450287)
3 years, 7 months ago (2017-05-10 19:15:23 UTC) #80
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/2848653003/240001
3 years, 7 months ago (2017-05-11 07:08:40 UTC) #82
commit-bot: I haz the power
3 years, 7 months ago (2017-05-11 08:08:57 UTC) #85
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/fa3fdf20cea4dbedd74476a40649...

Powered by Google App Engine
This is Rietveld 408576698