Bug 81909 – Web Inspector: only update Timeline overview when really needed

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

Currently, we update timeline overview upon any refresh of the lower pane. This really needs to be done only when we have new events, or displayed categories change.

[Andrey Kosyakov]

Created attachment 133262 [details]
Patch

[Pavel Feldman]

Comment on attachment 133262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133262&action=review

> Source/WebCore/inspector/front-end/TimelinePanel.js:562
> +        if (this._needOverviewUpdate) {

What about onRecordsCleared? why would we end up here with this._needOverviewUpdate === false anyways?

[Andrey Kosyakov]

Created attachment 134295 [details]
Patch

[Pavel Feldman]

Comment on attachment 134295 [details]
Patch

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.

[Andrey Kosyakov]

Created attachment 134306 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #4)
> (From update of attachment 134295 [details])
> 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.

Well, these changes are actually quite interconnected, separating them would appear artificial to me. Changed wording of the ChangeLog instead.

> No get prefix in WebKit functions.

Fixed.

> > Source/WebCore/inspector/front-end/TimelineModel.js:90
> > +    return record.startTime / 1000;
> 
> I think we should operate original units on the model level.

The model does operate original unit, we just provide this as a helper for higher-level code -- presentation model and overview should use same units, as they pass times back and forth. We could change PresentationModel to use original units as well, but I'd prefer this to be a separate change.

> categories()

Fixed.

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

This was a drive by fix, I got rid of the method and the constant in bug 82471.

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

Fixed.

[Andrey Kosyakov]

Created attachment 134318 [details]
Patch

[Pavel Feldman]

Comment on attachment 134318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134318&action=review

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

startTimeInSeconds ?

[Andrey Kosyakov]

Committed r112409: <http://trac.webkit.org/changeset/112409>