[webkit-reviews] review denied: [Bug 81909] Web Inspector: only update Timeline overview when really needed : [Attachment 134295] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 81909: Web Inspector: only update Timeline overview when really needed
https://bugs.webkit.org/show_bug.cgi?id=81909

Attachment 134295: Patch
https://bugs.webkit.org/attachment.cgi?id=134295&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134295&action=review


A bunch of nits.

> Source/WebCore/ChangeLog:6
> +	   - do not use formatted records and TimelinePresentationModel in
Overview;

It sounds like you are doing more than one thing in this change. Please
consider splitting it into parts.

> Source/WebCore/inspector/front-end/TimelineModel.js:88
> +WebInspector.TimelineModel.getStartTime = function(record)

No get prefix in WebKit functions.

> Source/WebCore/inspector/front-end/TimelineModel.js:90
> +    return record.startTime / 1000;

I think we should operate original units on the model level.

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:102
> +    var categories = WebInspector.TimelinePresentationModel.getCategories();


categories()

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:155
> +	   this._heapGraph.setSize(this._overviewGrid.element.offsetWidth, 60);


Please extract constant.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:60
> +WebInspector.TimelinePresentationModel.getRecordStyle = function(record) {

recordStyle.