Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(303)

Issue 77043006: DevTools: render time summary as a pie chart on Timeline panel. (Closed)

Created:
7 years, 1 month ago by pfeldman
Modified:
7 years, 1 month ago
Reviewers:
caseq, alph
CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org
Visibility:
Public.

Description

DevTools: render time summary as a pie chart on Timeline panel. Drive by: remove pid comparison from TimelinePanel.js - it does not belong there. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162503

Patch Set 1 : #

Total comments: 2

Patch Set 2 : Rebaselined #

Patch Set 3 : #

Total comments: 20

Patch Set 4 : Review comments addressed. #

Patch Set 5 : Review comments addressed #

Patch Set 6 : Review comments addressed #

Total comments: 4

Patch Set 7 : Removed the attempt to fix of coalescing records. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+245 lines, -73 lines) Patch
M Source/devtools/devtools.gyp View 1 chunk +1 line, -0 lines 0 comments Download
A + Source/devtools/front_end/PieChart.js View 1 2 1 chunk +28 lines, -27 lines 0 comments Download
M Source/devtools/front_end/TimelinePanel.js View 1 2 3 4 5 6 14 chunks +96 lines, -26 lines 0 comments Download
M Source/devtools/front_end/TimelinePresentationModel.js View 1 2 3 4 5 6 8 chunks +60 lines, -18 lines 0 comments Download
M Source/devtools/front_end/timelinePanel.css View 1 2 3 4 5 6 3 chunks +59 lines, -2 lines 0 comments Download
M Source/devtools/scripts/compile_frontend.py View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
pfeldman
7 years, 1 month ago (2013-11-20 06:18:26 UTC) #1
alph
Is there a screenshot to take a look? https://codereview.chromium.org/77043006/diff/30001/Source/devtools/front_end/TimelinePanel.js File Source/devtools/front_end/TimelinePanel.js (left): https://codereview.chromium.org/77043006/diff/30001/Source/devtools/front_end/TimelinePanel.js#oldcode175 Source/devtools/front_end/TimelinePanel.js:175: this._gpuTasks ...
7 years, 1 month ago (2013-11-20 07:23:53 UTC) #2
pfeldman
Screenshot: http://grab.by/sbVo https://codereview.chromium.org/77043006/diff/30001/Source/devtools/front_end/TimelinePanel.js File Source/devtools/front_end/TimelinePanel.js (left): https://codereview.chromium.org/77043006/diff/30001/Source/devtools/front_end/TimelinePanel.js#oldcode175 Source/devtools/front_end/TimelinePanel.js:175: this._gpuTasks = /** @type {!Array.<{startTime: number, endTime: ...
7 years, 1 month ago (2013-11-20 14:32:00 UTC) #3
pfeldman
On 2013/11/20 14:32:00, pfeldman wrote: > Screenshot: http://grab.by/sbVo > A better one: http://grab.by/sbVE
7 years, 1 month ago (2013-11-20 14:33:32 UTC) #4
caseq
https://codereview.chromium.org/77043006/diff/100001/Source/devtools/front_end/TimelinePanel.js File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/77043006/diff/100001/Source/devtools/front_end/TimelinePanel.js#newcode869 Source/devtools/front_end/TimelinePanel.js:869: function aggregateTimeForRecordWithinWindow(rawRecord) aggregateTimeForRecordsWithinWindow https://codereview.chromium.org/77043006/diff/100001/Source/devtools/front_end/TimelinePanel.js#newcode883 Source/devtools/front_end/TimelinePanel.js:883: var categoryName = WebInspector.TimelinePresentationModel.recordStyle(rawRecord).category.name; ...
7 years, 1 month ago (2013-11-21 01:30:13 UTC) #5
alph
https://codereview.chromium.org/77043006/diff/100001/Source/devtools/front_end/PieChart.js File Source/devtools/front_end/PieChart.js (right): https://codereview.chromium.org/77043006/diff/100001/Source/devtools/front_end/PieChart.js#newcode53 Source/devtools/front_end/PieChart.js:53: var sliceAngle = value / this._totalValue * 360; maybe ...
7 years, 1 month ago (2013-11-21 02:19:50 UTC) #6
pfeldman
https://codereview.chromium.org/77043006/diff/100001/Source/devtools/front_end/PieChart.js File Source/devtools/front_end/PieChart.js (right): https://codereview.chromium.org/77043006/diff/100001/Source/devtools/front_end/PieChart.js#newcode53 Source/devtools/front_end/PieChart.js:53: var sliceAngle = value / this._totalValue * 360; rotate(Infinitydeg) ...
7 years, 1 month ago (2013-11-21 15:22:15 UTC) #7
caseq
https://codereview.chromium.org/77043006/diff/210002/Source/devtools/front_end/PieChart.js File Source/devtools/front_end/PieChart.js (right): https://codereview.chromium.org/77043006/diff/210002/Source/devtools/front_end/PieChart.js#newcode53 Source/devtools/front_end/PieChart.js:53: var sliceAngle = value / this._totalValue * 360; nit: ...
7 years, 1 month ago (2013-11-21 18:43:03 UTC) #8
pfeldman
Leaving the coalescing bug to you. https://codereview.chromium.org/77043006/diff/210002/Source/devtools/front_end/PieChart.js File Source/devtools/front_end/PieChart.js (right): https://codereview.chromium.org/77043006/diff/210002/Source/devtools/front_end/PieChart.js#newcode53 Source/devtools/front_end/PieChart.js:53: var sliceAngle = ...
7 years, 1 month ago (2013-11-21 20:24:58 UTC) #9
caseq
lgtm
7 years, 1 month ago (2013-11-21 20:33:13 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pfeldman@chromium.org/77043006/250001
7 years, 1 month ago (2013-11-22 00:02:43 UTC) #11
commit-bot: I haz the power
7 years, 1 month ago (2013-11-22 01:16:53 UTC) #12
Message was sent while issue was closed.
Change committed as 162503

Powered by Google App Engine
This is Rietveld 408576698