enable / disable Long Task instrumentation based on presence of LongTask observer

enable / disable Long Task instrumentation based on presence of LongTask observer

BUG=635596
Committed: https://crrev.com/ad2240ff457a2512b5868aa923f56bab1cd9bb4a
Cr-Commit-Position: refs/heads/master@{#419743}

[panicker, 2016-09-15 16:31:39]

panicker@chromium.org changed reviewers:
+ caseq@chromium.org, dgozman@chromium.org

[panicker, 2016-09-15 16:31:40]

PTAL

[caseq, 2016-09-15 23:29:40]

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:100: if
(hasObserverFor(PerformanceEntry::LongTask)
nit: we tend to prefer early bail-outs,
i.e. "if (!condition) return" as opposed to "if (condition) { ... }"

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/PerformanceBase.h:67: virtual void
disableLongTaskInstrumentation() const {}
Why are these two const? I find it to be a bit artificial, considering you have
to use mutable in the implementaiton.

[panicker, 2016-09-16 17:07:58]

PTAL.

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:100: if
(hasObserverFor(PerformanceEntry::LongTask)
On 2016/09/15 23:29:40, caseq wrote:
> nit: we tend to prefer early bail-outs,
> i.e. "if (!condition) return" as opposed to "if (condition) { ... }"

Ok. Done.

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/2343553005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/PerformanceBase.h:67: virtual void
disableLongTaskInstrumentation() const {}
On 2016/09/15 23:29:40, caseq wrote:
> Why are these two const? I find it to be a bit artificial, considering you
have
> to use mutable in the implementaiton.

Done.

[panicker, 2016-09-17 00:37:43]

Discussed with Dima and updated patch to prevent the issue of tasks getting
reported after disable - due to delay in Oilpan dtor call.

[dgozman, 2016-09-17 01:11:00]

Looks pretty good.

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:22: {
Let's either call disableObserver() from here or DCHECK we've been disabled.

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:105:
frame()->instrumentingAgents()->addInspectorWebPerfAgent(
I'd move this call into enableObserver.

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:114:
frame()->instrumentingAgents()->removeInspectorWebPerfAgent(
Similar here.

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:416:
disableLongTaskInstrumentation();
Looks like previous line will enable instrumentation just to disable it here.

[panicker, 2016-09-17 03:04:00]

PTAL

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:22: {
On 2016/09/17 01:11:00, dgozman wrote:
> Let's either call disableObserver() from here or DCHECK we've been disabled.

How's this?

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:105:
frame()->instrumentingAgents()->addInspectorWebPerfAgent(
On 2016/09/17 01:11:00, dgozman wrote:
> I'd move this call into enableObserver.

Done.

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:114:
frame()->instrumentingAgents()->removeInspectorWebPerfAgent(
On 2016/09/17 01:11:00, dgozman wrote:
> Similar here.

Done.

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:416:
disableLongTaskInstrumentation();
On 2016/09/17 01:11:00, dgozman wrote:
> Looks like previous line will enable instrumentation just to disable it here.

Yes this is strange, it's more clear to call from PerformanceObserver - so moved
there.

[panicker, 2016-09-18 17:49:59]

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:22: {
On 2016/09/17 03:04:00, Shubhie wrote:
> On 2016/09/17 01:11:00, dgozman wrote:
> > Let's either call disableObserver() from here or DCHECK we've been disabled.
> 
> How's this?

hmm it's not okay to access potentially finalized fields in dtor.
Were you concerned about the task-observers OR removeInspectorWebPerfAgent ?

[dgozman, 2016-09-19 17:55:59]

lgtm with comments

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:22: {
On 2016/09/18 17:49:59, Shubhie wrote:
> On 2016/09/17 03:04:00, Shubhie wrote:
> > On 2016/09/17 01:11:00, dgozman wrote:
> > > Let's either call disableObserver() from here or DCHECK we've been
disabled.
> > 
> > How's this?
> 
> hmm it's not okay to access potentially finalized fields in dtor.
> Were you concerned about the task-observers OR removeInspectorWebPerfAgent ?
Both. I propose to have |enabled_| field, update it in
enableObserver/disableObserver and DCHECK(!enabled_) here.

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/Performance.cpp:108: if
(hasObserverFor(PerformanceEntry::LongTask) || !m_longTaskInspectorAgent)
Since we check for enable conditions in both methods, I think we can combine
them into a single updateLongTaskInstrumentation which will either enable or
disable or do nothing.

[caseq, 2016-09-19 22:18:06]

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceObserver.cpp:55:
m_performance->enableLongTaskInstrumentation();
Should we rather make this part of
PerformanceBase::registerPerformanceObserver()?

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceObserver.cpp:63:
m_performance->disableLongTaskInstrumentation();
ditto, consider moving to PerformanceBase::unregisterPerformanceObserver()

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceTest.cpp (right):

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceTest.cpp:44:
m_performance->enableLongTaskInstrumentation();
Can we rather express the test in a higher-level terms, i.e. closer to the user
level -- register performance observer (or two) and check that  the long task
agent is appropriately added/removed from instrumentation?

[panicker, 2016-09-20 12:00:40]

Thanks for the review!
Will try to submit soon.
If you have further comments feel free to add them, and I'll address in a
follow-up CL.
Thanks!

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2343553005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:22: {
On 2016/09/19 17:55:59, dgozman wrote:
> On 2016/09/18 17:49:59, Shubhie wrote:
> > On 2016/09/17 03:04:00, Shubhie wrote:
> > > On 2016/09/17 01:11:00, dgozman wrote:
> > > > Let's either call disableObserver() from here or DCHECK we've been
> disabled.
> > > 
> > > How's this?
> > 
> > hmm it's not okay to access potentially finalized fields in dtor.
> > Were you concerned about the task-observers OR removeInspectorWebPerfAgent ?
> Both. I propose to have |enabled_| field, update it in
> enableObserver/disableObserver and DCHECK(!enabled_) here.

Done.

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/Performance.cpp:108: if
(hasObserverFor(PerformanceEntry::LongTask) || !m_longTaskInspectorAgent)
On 2016/09/19 17:55:59, dgozman wrote:
> Since we check for enable conditions in both methods, I think we can combine
> them into a single updateLongTaskInstrumentation which will either enable or
> disable or do nothing.

Done.

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceObserver.cpp:55:
m_performance->enableLongTaskInstrumentation();
On 2016/09/19 22:18:06, caseq wrote:
> Should we rather make this part of
> PerformanceBase::registerPerformanceObserver()?

Yes, I started with that in this CL, but the issue was observe() didn't
necessarily call PerformanceBase::registerPerformanceObserver().
Though moving it back, now that I've merged enable / disable into 1 method this
is fine.

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceObserver.cpp:63:
m_performance->disableLongTaskInstrumentation();
On 2016/09/19 22:18:06, caseq wrote:
> ditto, consider moving to PerformanceBase::unregisterPerformanceObserver()

Done.

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceTest.cpp (right):

https://codereview.chromium.org/2343553005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceTest.cpp:44:
m_performance->enableLongTaskInstrumentation();
On 2016/09/19 22:18:06, caseq wrote:
> Can we rather express the test in a higher-level terms, i.e. closer to the
user
> level -- register performance observer (or two) and check that  the long task
> agent is appropriately added/removed from instrumentation?

Since I've moved this out of PerformanceObserver, I dont think it belongs in a
unittest.
This test will be added as a web-platform-test (JS).

[panicker, 2016-09-20 12:00:51]

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

[commit-bot: I haz the power, 2016-09-20 12:01: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, 2016-09-20 13:38:11]

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

[commit-bot: I haz the power, 2016-09-20 13:38:12]

Dry run: This issue passed the CQ dry run.

[panicker, 2016-09-20 13:43:09]

The CQ bit was checked by panicker@chromium.org

[panicker, 2016-09-20 13:43:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2343553005/#ps120001
(title: "address review comments")

[commit-bot: I haz the power, 2016-09-20 13:43:29]

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, 2016-09-20 13:48:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-20 13:51:30]

Description was changed from

==========
enable / disable Long Task instrumentation based on presence of LongTask
observer

BUG=635596
==========

to

==========
enable / disable Long Task instrumentation based on presence of LongTask
observer

BUG=635596

Committed: https://crrev.com/ad2240ff457a2512b5868aa923f56bab1cd9bb4a
Cr-Commit-Position: refs/heads/master@{#419743}
==========

[commit-bot: I haz the power, 2016-09-20 13:51:31]

Patchset 7 (id:??) landed as
https://crrev.com/ad2240ff457a2512b5868aa923f56bab1cd9bb4a
Cr-Commit-Position: refs/heads/master@{#419743}