DevTools: add the logging aspect into the PerformanceMonitor

DevTools: add the logging aspect into the PerformanceMonitor, plumb it over the remote debugging protocol, surface it in DevTools. Also adds proof of concept violation reporting for layout thrashing and long running tasks.

BUG=662497
Committed: https://crrev.com/8bf066cc7d7529342fd260e5bf3d98a97c6b133f
Cr-Commit-Position: refs/heads/master@{#430418}

[pfeldman, 2016-11-04 19:43:14]

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

[caseq, 2016-11-04 20:59:00]

lgtm

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/EventTarget.cpp (left):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventTarget.cpp:143:
registeredListener->setBlockedEventWarningEmitted();
Bring this back, we really need it!

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:165: if
(!m_loggingEnabled && !m_webPerformanceObservers.size()) {
This really could be an else. 
bool should_enable = m_loggingEnabled || m_webPerformanceObservers.size()
if (should_enable == m_enabled)
  return;
if (should_enable) {
   ...
} else {
   ...
}

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:536:
RevokedError: 'revokedError'  // This is frontend-only level, used to put
exceptions to console.,
remove ,

[panicker, 2016-11-04 20:59:28]

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:527:
PerformanceMonitor::didExecuteScript(context);
could you move this to separate CL and submit sooner (Long tasks is broken
otherwise).
Alternatively I can send a CL.

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:26: static const
double kMaxSyncLayoutMillis = 15.0;
kSyncLayoutThresholdMillis for consistency?

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:217:
m_isExecutingTask = true;
this is not used yet?

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/ConsoleTypes.h (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/ConsoleTypes.h:31:
ViolationMessageLevel = 6
PerfViolationMessageLevel ?
I don't think we want non-performance related messages here (yet)

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/ConsoleTypes.h:31:
ViolationMessageLevel = 6
Seems like we might need severity levels within ViolationMessageLevel, but this
is probably fine to start with

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:122: if
(m_performanceMonitor)
under what conditions is this enabled ?
is this exposed to developers? if so, what's their opt-out?

[panicker, 2016-11-04 21:04:58]

Also could you update the CL description to reflect that this is the ground work
for more generic PerformanceMonitor, with Devtools as a consumer?

[pfeldman, 2016-11-04 21:26:39]

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:527:
PerformanceMonitor::didExecuteScript(context);
How come are they broken?

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/EventTarget.cpp (left):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventTarget.cpp:143:
registeredListener->setBlockedEventWarningEmitted();
On 2016/11/04 20:58:59, caseq wrote:
> Bring this back, we really need it!

Ouch, thank you.

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:26: static const
double kMaxSyncLayoutMillis = 15.0;
On 2016/11/04 20:59:28, Shubhie wrote:
> kSyncLayoutThresholdMillis for consistency?

Done.

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:165: if
(!m_loggingEnabled && !m_webPerformanceObservers.size()) {
Omg, so much better! (I had more branches there)

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:217:
m_isExecutingTask = true;
On 2016/11/04 20:59:28, Shubhie wrote:
> this is not used yet?

Done.

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/ConsoleTypes.h (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/ConsoleTypes.h:31:
ViolationMessageLevel = 6
On 2016/11/04 20:59:28, Shubhie wrote:
> PerfViolationMessageLevel ?
> I don't think we want non-performance related messages here (yet)

I'll make it a source, not a level, then you get the levels. Also, violations
seems more appropriate for lighthouse.

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:122: if
(m_performanceMonitor)
On 2016/11/04 20:59:28, Shubhie wrote:
> under what conditions is this enabled ?
> is this exposed to developers? if so, what's their opt-out?

This is called when devtools is opened, so one gets logging messages. Observers
go the observers route

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/2474073005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:536:
RevokedError: 'revokedError'  // This is frontend-only level, used to put
exceptions to console.,
On 2016/11/04 20:58:59, caseq wrote:
> remove ,

Done.

[pfeldman, 2016-11-04 21:30:18]

On 2016/11/04 21:04:58, Shubhie wrote:
> Also could you update the CL description to reflect that this is the ground
work
> for more generic PerformanceMonitor, with Devtools as a consumer?

I'm hesitant referring to it as a ground work towards something :). In this
case, I'm adding the 'logging' aspect to the performance monitor and surfacing
it in devtools. Lets me say that in the description.

[pfeldman, 2016-11-04 21:31:33]

Description was changed from

==========
DevTools: surface perf violations in console (log domain)

BUG=662497
==========

to

==========
DevTools: add the logging aspect into the PerformanceMonitor, plumb it over the
remote debugging protocol, surface it in DevTools. Also adds proof of concept
violation reporting for layout thrashing and long running tasks.

BUG=662497
==========

[panicker, 2016-11-04 21:39:19]

LGTM
(although I don't see the updated patchset yet)

[pfeldman, 2016-11-04 22:11:36]

On 2016/11/04 21:39:19, Shubhie wrote:
> LGTM
> (although I don't see the updated patchset yet)

Sorry, was changing it from Level to Source for Violation. caseq@ mind taking
another look at the front-end? I really need to move ConsoleMessage off
inspector/ folder as a next thing.

[pfeldman, 2016-11-04 22:16:18]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-04 22:16:18]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2474073005/#ps20001
(title: "w/ violation as a source, not level.")

[commit-bot: I haz the power, 2016-11-04 22:17:08]

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-11-04 22:21:13]

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

[commit-bot: I haz the power, 2016-11-04 22:21:14]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pfeldman, 2016-11-04 22:56:06]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-04 22:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2474073005/#ps40001
(title: "rebaselined")

[commit-bot: I haz the power, 2016-11-04 22:57:06]

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

[pfeldman, 2016-11-04 23:25:37]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2016-11-04 23:25:54]

I need to filter out these from the test results...

[caseq, 2016-11-05 00:14:35]

lgtm

https://codereview.chromium.org/2474073005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/2474073005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.cpp:103: return
PerformanceMonitor::enabled(context) ? 0.1 : 0;
Extract constant for 0.1

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

https://codereview.chromium.org/2474073005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:161: return
Response::Error("Log is not enabled.");
no terminating punctuation in error messages.

[pfeldman, 2016-11-05 00:16:02]

https://codereview.chromium.org/2474073005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/2474073005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.cpp:103: return
PerformanceMonitor::enabled(context) ? 0.1 : 0;
On 2016/11/05 00:14:35, caseq wrote:
> Extract constant for 0.1

Done.

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

https://codereview.chromium.org/2474073005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:161: return
Response::Error("Log is not enabled.");
On 2016/11/05 00:14:35, caseq wrote:
> no terminating punctuation in error messages.

Done.

[pfeldman, 2016-11-05 00:16:09]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-05 00:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
panicker@chromium.org, caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2474073005/#ps80001
(title: "for landing")

[commit-bot: I haz the power, 2016-11-05 00:16:43]

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-11-05 02:20:44]

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

[commit-bot: I haz the power, 2016-11-05 02:20:45]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pfeldman, 2016-11-07 21:38:29]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-07 21:38:29]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2474073005/#ps100001
(title: "test fixed")

[commit-bot: I haz the power, 2016-11-07 21:38:58]

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-11-07 23:35:38]

Description was changed from

==========
DevTools: add the logging aspect into the PerformanceMonitor, plumb it over the
remote debugging protocol, surface it in DevTools. Also adds proof of concept
violation reporting for layout thrashing and long running tasks.

BUG=662497
==========

to

==========
DevTools: add the logging aspect into the PerformanceMonitor, plumb it over the
remote debugging protocol, surface it in DevTools. Also adds proof of concept
violation reporting for layout thrashing and long running tasks.

BUG=662497
==========

[commit-bot: I haz the power, 2016-11-07 23:35:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-07 23:43:39]

Description was changed from

==========
DevTools: add the logging aspect into the PerformanceMonitor, plumb it over the
remote debugging protocol, surface it in DevTools. Also adds proof of concept
violation reporting for layout thrashing and long running tasks.

BUG=662497
==========

to

==========
DevTools: add the logging aspect into the PerformanceMonitor, plumb it over the
remote debugging protocol, surface it in DevTools. Also adds proof of concept
violation reporting for layout thrashing and long running tasks.

BUG=662497

Committed: https://crrev.com/8bf066cc7d7529342fd260e5bf3d98a97c6b133f
Cr-Commit-Position: refs/heads/master@{#430418}
==========

[commit-bot: I haz the power, 2016-11-07 23:43:41]

Patchset 6 (id:??) landed as
https://crrev.com/8bf066cc7d7529342fd260e5bf3d98a97c6b133f
Cr-Commit-Position: refs/heads/master@{#430418}

[henrika (OOO until Aug 14), 2016-11-08 12:07:48]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2490563002/ by henrika@chromium.org.

The reason for reverting is: Speculative revert as sheriff since we see a
failure in WorkerDevToolsSanityTest.PauseInSharedWorkerInitialization on Win10.

See link below for details:

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win10%20Tests%20....

[henrika (OOO until Aug 14), 2016-11-08 14:22:10]

Looks like my revert failed. Perhaps I was to slow to revert. Investigating.

[stgao, 2016-11-10 23:16:11]

On 2016/11/08 12:07:48, henrika wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/2490563002/ by mailto:henrika@chromium.org.
> 
> The reason for reverting is: Speculative revert as sheriff since we see a
> failure in WorkerDevToolsSanityTest.PauseInSharedWorkerInitialization on
Win10.
> 
> See link below for details:
> 
>
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win10%20Tests%20....

The flake also shows up on Mac too. See the flakiness trend here.
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....