Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2878933003:
Makes WorkerInspectorProxy work with ExecutionContext instead of Document (Closed)
|
Created:
3 years, 7 months ago by kinuko Modified:
3 years, 7 months ago CC:
chromium-reviews, kinuko+worker_chromium.org, caseq+blink_chromium.org, lushnikov+blink_chromium.org, shimazu+worker_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org, kozyatinskiy+blink_chromium.org, nhiroki Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMakes WorkerInspectorProxy work with ExecutionContext instead of Document
As a part of efforts to make {Shared,Service}Workers independent from
Frame and Document.
BUG=538751
Review-Url: https://codereview.chromium.org/2878933003
Cr-Commit-Position: refs/heads/master@{#472838}
Committed: https://chromium.googlesource.com/chromium/src/+/3e527f87b33591975570c2e71487ac8d6d0480f1
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : more fixes #Patch Set 4 : . #
Total comments: 2
Patch Set 5 : cast #Patch Set 6 : . #Patch Set 7 : . #
Total comments: 1
Messages
Total messages: 50 (40 generated)
Description was changed from
==========
Makes WorkerInspectorProxy work with ExecutionContext instead of Document
As a part of efforts to make {Shared,Service}Workers independent from
Frame and Document.
BUG=538751
==========
to
==========
Makes WorkerInspectorProxy work with ExecutionContext instead of Document
As a part of efforts to make {Shared,Service}Workers independent from
Frame and Document.
BUG=538751
==========
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kinuko@chromium.org changed reviewers: + dgozman@chromium.org, falken@chromium.org
PTL falken@ for worker part (cc-ing nhiroki, who's ooo) dgozman@ for inspector
lgtm https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerInspectorProxy.cpp (right): https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerInspectorProxy.cpp:72: // workerStartMode right before. seems there is no such thing as "workerStartMode" anymore. "ask for workerStartMode" -> "call WorkerStartMode()" ? https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:320: SecurityOrigin* starter_origin = loading_document_->GetSecurityOrigin(); To be honest I don't understand this FIXME... what is action we are supposed to take? The shadow document is not supposed to be "pristine"? And did anything change now that we use loading_document_ instead of main_frame_->getDocument()? If you know, could you possibly update the comment?
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kinuko@chromium.org
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Thanks! Updated, and also removed AddMessageFromWorker from FrameConsole. https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerInspectorProxy.cpp (right): https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerInspectorProxy.cpp:72: // workerStartMode right before. On 2017/05/15 06:09:35, falken wrote: > seems there is no such thing as "workerStartMode" anymore. > > "ask for workerStartMode" -> "call WorkerStartMode()" ? Done. https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2878933003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:320: SecurityOrigin* starter_origin = loading_document_->GetSecurityOrigin(); On 2017/05/15 06:09:35, falken wrote: > To be honest I don't understand this FIXME... what is action we are supposed to > take? The shadow document is not supposed to be "pristine"? And did anything > change now that we use loading_document_ instead of main_frame_->getDocument()? > > If you know, could you possibly update the comment? This SecurityOrigin doesn't correctly reflect some of the frame-level settings (e.g. GrantUniversalAccess) which could be true in some cases (e.g. in android webview it looks it's possible to turn it true from what I quickly searched) And no, it shouldn't change anything by this CL. I updated the comment to make it slightly clearer.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a comment https://codereview.chromium.org/2878933003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectedFrames.cpp (right): https://codereview.chromium.org/2878933003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectedFrames.cpp:27: return context->GetProbeSink() == root_->InstrumentingAgents(); This code looks strange as is. I would not expect that InspectedFrames contains anything but a frame. Should we instead cast to LocalFrame and call Contains(LocalFrame*) ? Alternatively, we can change this class to be more abstract, but I think that's a bit premature.
Thanks! I have one question I wanted to ask you/inspector folks: https://codereview.chromium.org/2878933003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectedFrames.cpp (right): https://codereview.chromium.org/2878933003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectedFrames.cpp:27: return context->GetProbeSink() == root_->InstrumentingAgents(); On 2017/05/15 16:39:55, dgozman wrote: > This code looks strange as is. I would not expect that InspectedFrames contains > anything but a frame. Should we instead cast to LocalFrame and call > Contains(LocalFrame*) ? > > Alternatively, we can change this class to be more abstract, but I think that's > a bit premature. Actually this is the important bit I want to discuss a bit further. Some context: some Worker-related features (i.e. ServiceWorker and SharedWorker) we're creating dummy (so-called 'shadow') LocalFrame and Document mostly because DevTools and loading code require them, but we want to get rid of the dependency (crbug.com/538751) and want to make them work without creating dummy frame/document. And InspectedFrames is one of the part that seems to need some changes in doing this... lots of DevTools code expects that all the plumbable stuff has LocalFrame via InspectedFrames. I've been trying to write a patch that makes InspectedFrames a bit more abstract and make it possible to contain a stand-alone, frame-less ExecutionContext. What's your gut feeling about that? If you have any suggestions/recommendations to move this forward they're really appreciated!
On 2017/05/15 20:09:34, kinuko wrote: > Thanks! I have one question I wanted to ask you/inspector folks: > > https://codereview.chromium.org/2878933003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/InspectedFrames.cpp (right): > > https://codereview.chromium.org/2878933003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectedFrames.cpp:27: return > context->GetProbeSink() == root_->InstrumentingAgents(); > On 2017/05/15 16:39:55, dgozman wrote: > > This code looks strange as is. I would not expect that InspectedFrames > contains > > anything but a frame. Should we instead cast to LocalFrame and call > > Contains(LocalFrame*) ? > > > > Alternatively, we can change this class to be more abstract, but I think > that's > > a bit premature. > > Actually this is the important bit I want to discuss a bit further. Some > context: some Worker-related features (i.e. ServiceWorker and SharedWorker) > we're creating dummy (so-called 'shadow') LocalFrame and Document mostly because > DevTools and loading code require them, but we want to get rid of the dependency > (crbug.com/538751) and want to make them work without creating dummy > frame/document. > > And InspectedFrames is one of the part that seems to need some changes in doing > this... lots of DevTools code expects that all the plumbable stuff has > LocalFrame via InspectedFrames. I've been trying to write a patch that makes > InspectedFrames a bit more abstract and make it possible to contain a > stand-alone, frame-less ExecutionContext. What's your gut feeling about that? > If you have any suggestions/recommendations to move this forward they're really > appreciated! I think a lot of DevTools code should not be run for workers, like DOM, CSS, debugger, etc. Those which make sense (like InspectorWorkerAgent or InspectorNetworkAgent) could either be migrated from InspectedFrames, or we can pass an additional ExecutionContext to them (is it a single one?), or add a specially-handled ExecutionContext to InspectedFrames. Should we check on case-by-case basis?
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Patchset #7 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:120001) has been deleted
> I think a lot of DevTools code should not be run for workers, like DOM, CSS,
> debugger, etc.
> Those which make sense (like InspectorWorkerAgent or InspectorNetworkAgent)
> could either be migrated
> from InspectedFrames, or we can pass an additional ExecutionContext to them
(is
> it a single one?),
> or add a specially-handled ExecutionContext to InspectedFrames. Should we
check
> on case-by-case basis?
Yeah. I looked into some classes, I think we'll need
Inspector{Worker,Network,Tracing}Agent at least and they can likely just take
additional ExecutionContext when the target is not a frame. For this patch I
just reverted the changes in InspectedFrames.
https://codereview.chromium.org/2878933003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.cpp (right):
https://codereview.chromium.org/2878933003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.cpp:143: Document*
document = ToDocument(proxy->GetExecutionContext());
I turned this to use cast & back to use frames for now with a TODO.
I'm landing this now (no inspected frames changes), but please feel free to un-CQ if anything looks wrong. Thanks!
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2878933003/#ps180001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495118553174250,
"parent_rev": "6c5222823cb86a00c1fcf5c90f0e08cd52367505", "commit_rev":
"3e527f87b33591975570c2e71487ac8d6d0480f1"}
Message was sent while issue was closed.
Description was changed from
==========
Makes WorkerInspectorProxy work with ExecutionContext instead of Document
As a part of efforts to make {Shared,Service}Workers independent from
Frame and Document.
BUG=538751
==========
to
==========
Makes WorkerInspectorProxy work with ExecutionContext instead of Document
As a part of efforts to make {Shared,Service}Workers independent from
Frame and Document.
BUG=538751
Review-Url: https://codereview.chromium.org/2878933003
Cr-Commit-Position: refs/heads/master@{#472838}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e527f87b33591975570c2e71487...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/3e527f87b33591975570c2e71487... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
