DevTools: bring violations and timeline warnings in sync.

DevTools: bring violations and timeline warnings in sync.

BUG=662497
Committed: https://crrev.com/5c5acf32c515709bb28f067d3540e5a0ddde6547
Cr-Commit-Position: refs/heads/master@{#433752}

[pfeldman, 2016-11-19 03:02:45]

pfeldman@chromium.org changed reviewers:
+ caseq@chromium.org

[alph, 2016-11-19 05:36:26]

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

[alph, 2016-11-19 05:36:27]

lgtm

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1742:
span.textContent = Common.UIString('Handler took ' +
Number.millisToString(event.duration, true));
May be it can be more explicit on what's wrong, e.g. 'Handler took too long ...'

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js
(right):

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:648:
if (this._currentTaskLayoutAndRecalcEvents &&
this._currentTaskLayoutAndRecalcEvents.length) {
It won't work for the very last top level event.

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:649:
var totalTime = this._currentTaskLayoutAndRecalcEvents.reduce((time, event) =>
time + event.duration, 0);
We use const if the value is never changed.

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:1316:
Handler: 150,
Perhaps, there should be different types of handlers, e.g. 150 is to much for
mousemove. Not for this patch though.

[caseq, 2016-11-19 05:51:08]

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1742:
span.textContent = Common.UIString('Handler took ' +
Number.millisToString(event.duration, true));
Common.UIString('Handler look %s', Number.millisToString(...))

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1745:
span.textContent = Common.UIString('Recurring handler took ' +
Number.millisToString(event.duration, true));
ditto.

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineFrameModel.js
(right):

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineFrameModel.js:447:
return false;
This function becomes meaningless in its present form, it's not an override.
Please nuke it altogether along with all usages.

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js
(right):

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:652:
var timelineData = TimelineModel.TimelineData.forEvent(e);
This has the same name as a different variable below -- while this works, we
typically extract the definition to the top in such cases. Or use a different
name.

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:745:
if (parentEvent && parentEvent.name === recordTypes.EventDispatch) {
Why do it here? Let's do it when handling corresponding "parent" event instead.

https://codereview.chromium.org/2515693003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:754:
(parentEvent.name === recordTypes.TimerFire || parentEvent.name ===
recordTypes.FireAnimationFrame)) {
ditto.

[pfeldman, 2016-11-21 18:57:37]

all done

[pfeldman, 2016-11-21 18:57:40]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-21 18:57:40]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2515693003/#ps20001
(title: "review comments addressed.")

[commit-bot: I haz the power, 2016-11-21 18:58:22]

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

[pfeldman, 2016-11-21 19:52:39]

All done referred to caseq's comments. I'll fix alph's in a follow-up.

[commit-bot: I haz the power, 2016-11-21 20:42:39]

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

[commit-bot: I haz the power, 2016-11-21 20:42:40]

Try jobs failed on following builders:
  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-22 01:02:41]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-22 01:02:41]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2515693003/#ps40001
(title: "idle callback test fixed.")

[commit-bot: I haz the power, 2016-11-22 01:03: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-11-22 01:45:50]

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

[commit-bot: I haz the power, 2016-11-22 01:45:51]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pfeldman, 2016-11-22 01:51:07]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2016-11-22 01:51:41]

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-22 02:38:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479779467209380,
"parent_rev": "fe42aecaf51e6b21a4a4be87f575c970774190ae", "commit_rev":
"2c27ec06527cbf111ea02a8c6d7f8244db66087b"}

[commit-bot: I haz the power, 2016-11-22 02:38:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-22 02:42:44]

Description was changed from

==========
DevTools: bring violations and timeline warnings in sync.

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

to

==========
DevTools: bring violations and timeline warnings in sync.

BUG=662497

Committed: https://crrev.com/5c5acf32c515709bb28f067d3540e5a0ddde6547
Cr-Commit-Position: refs/heads/master@{#433752}
==========

[commit-bot: I haz the power, 2016-11-22 02:42:45]

Patchset 3 (id:??) landed as
https://crrev.com/5c5acf32c515709bb28f067d3540e5a0ddde6547
Cr-Commit-Position: refs/heads/master@{#433752}