Make ExecutionContext aware about how it can return CoreProbeSink

Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext, which sounds a bit cumbersome.

This CL adds ExecutionContext::GetProbeSink() virtual method
to mitigate that.

BUG=538751
Review-Url: https://codereview.chromium.org/2847703002
Cr-Commit-Position: refs/heads/master@{#468243}
Committed: https://chromium.googlesource.com/chromium/src/+/b66ac187d7971914c0cbff0bf3733feb5b27f006

[kinuko, 2017-04-27 09:03:07]

Description was changed from

==========
Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext.

This CL adds ExecutionContext::GetInstrumentingAgents() virtual method
to mitigate that.

BUG=538751
==========

to

==========
Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext, which sounds a bit cumbersome.

This CL adds ExecutionContext::GetInstrumentingAgents() virtual method
to mitigate that.

BUG=538751
==========

[kinuko, 2017-04-27 09:03:07]

kinuko@chromium.org changed reviewers:
+ dgozman@chromium.org, nhiroki@chromium.org

[kinuko, 2017-04-27 09:04:02]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 09:04:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2017-04-27 09:07:23]

I'm not sure if there was a strong preference not to do this, but what do you
think about this change?

[kinuko, 2017-04-27 09:45:51]

On 2017/04/27 09:07:23, kinuko wrote:
> I'm not sure if there was a strong preference not to do this, but what do you
> think about this change?

Context: I'm trying to decouple resource loading code from Frame and Document,
and thinking about creating a small ExecutionContext class that can be used only
for resource loading and inspector plumbing for the cases we don't have real
documents. (Suggestions to this approach itself is appreciated too)

[commit-bot: I haz the power, 2017-04-27 10:55:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 10:55:53]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-04-27 15:33:08]

dgozman@chromium.org changed reviewers:
+ alph@chromium.org

[dgozman, 2017-04-27 15:33:08]

Note that resolving the sink must be very performant, as we try to impede the
minimum overhead when instrumentation is off.

@alph: what do you think?

[alph, 2017-04-27 23:40:09]

On 2017/04/27 15:33:08, dgozman wrote:
> Note that resolving the sink must be very performant, as we try to impede the
> minimum overhead when instrumentation is off.
> 
> @alph: what do you think?

I'm ok with it. I don't think a virtual call is much slower than a check for
context->IsWorkerGlobalScope()
So I'd lgtm it.

[dgozman, 2017-04-28 00:06:38]

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ExecutionContext.h:196: virtual
CoreProbeSink* GetInstrumentingAgents() { return nullptr; }
Let's call this GetProbeSink. @alph: we should rename from instrumenting agents
elsewhere.

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/probe/CoreProbes.cpp (right):

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/probe/CoreProbes.cpp:139: CoreProbeSink*
ToCoreProbeSinkForNonDocumentContext(ExecutionContext* context) {
Why not replace ToCoreProbeSink in CoreProbes.h as well?


https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/probe/Cor...

[kinuko, 2017-04-28 01:17:30]

Description was changed from

==========
Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext, which sounds a bit cumbersome.

This CL adds ExecutionContext::GetInstrumentingAgents() virtual method
to mitigate that.

BUG=538751
==========

to

==========
Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext, which sounds a bit cumbersome.

This CL adds ExecutionContext::GetProbeSink() virtual method
to mitigate that.

BUG=538751
==========

[kinuko, 2017-04-28 03:16:12]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[kinuko, 2017-04-28 03:16:26]

Thanks, updated.

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ExecutionContext.h:196: virtual
CoreProbeSink* GetInstrumentingAgents() { return nullptr; }
On 2017/04/28 00:06:38, dgozman wrote:
> Let's call this GetProbeSink. @alph: we should rename from instrumenting
agents
> elsewhere.

Done.

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/probe/CoreProbes.cpp (right):

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/probe/CoreProbes.cpp:139: CoreProbeSink*
ToCoreProbeSinkForNonDocumentContext(ExecutionContext* context) {
On 2017/04/28 00:06:38, dgozman wrote:
> Why not replace ToCoreProbeSink in CoreProbes.h as well?
> 
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/probe/Cor...

Done.

[commit-bot: I haz the power, 2017-04-28 03:16:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 03:38:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 03:38:40]

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...)

[nhiroki, 2017-04-28 05:25:46]

lgtm

[kinuko, 2017-04-28 05:28:42]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 05:29:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 07:15:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 07:15:34]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-04-28 18:38:40]

lgtm

[kinuko, 2017-04-29 06:07:02]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-04-29 06:07:03]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2847703002/#ps60001
(title: "fixed typo")

[commit-bot: I haz the power, 2017-04-29 06:07:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 06:10:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-29 06:10:23]

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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2017-04-29 11:04:57]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-04-29 11:04:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, alph@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2847703002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2017-04-29 11:05:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 15:19:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493463897624690,
"parent_rev": "01c3eb8fd187b96cf17164ebcf15c69da1fa660e", "commit_rev":
"b66ac187d7971914c0cbff0bf3733feb5b27f006"}

[commit-bot: I haz the power, 2017-04-29 15:19:27]

Description was changed from

==========
Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext, which sounds a bit cumbersome.

This CL adds ExecutionContext::GetProbeSink() virtual method
to mitigate that.

BUG=538751
==========

to

==========
Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext, which sounds a bit cumbersome.

This CL adds ExecutionContext::GetProbeSink() virtual method
to mitigate that.

BUG=538751

Review-Url: https://codereview.chromium.org/2847703002
Cr-Commit-Position: refs/heads/master@{#468243}
Committed:
https://chromium.googlesource.com/chromium/src/+/b66ac187d7971914c0cbff0bf373...
==========

[commit-bot: I haz the power, 2017-04-29 15:19:29]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b66ac187d7971914c0cbff0bf373...