Bug 100951 – Web Inspector: Timeline: show popup for CPU bars.

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

Created attachment 171830 [details]
Screenshot

Each CPU bar can represent a combination of several shorter messages.
We should show information about combined messages - start time,
total duration, CPU time, message count.

Created attachment 171833 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 171833 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePanel.js:907
> +                lastElement.generatePopupContent = WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent.bind(null, barStartTime - timeOffset, barEndTime - barStartTime, cpuTime, messageCount);

I don't quite like the way we associate state with element by re-binding function arguments. Also, we normally use underscore to prefix our fields when associating them with DOM elements. How about lastElement._firstTask and lastElement._lastTask (and compute the rest lazily).

> Source/WebCore/inspector/front-end/TimelinePanel.js:928
> +                lastElement.generatePopupContent = WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent.bind(null, barStartTime - timeOffset, barEndTime - barStartTime, cpuTime, messageCount);

indent

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:390
> +    getMinimumRecordTime: function()

drop get, just minimumRecordTime -- it's cleaner.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1087
> +WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent = function(startTime, duration, cpuTime, messageCount, callback) {

{ => next line

[WebKit Review Bot]

Comment on attachment 171833 [details]
Patch

Attachment 171833 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14670506

New failing tests:
inspector-protocol/debugger-pause-dedicated-worker.html

Comment on attachment 171833 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TimelinePanel.js:907
>> +                lastElement.generatePopupContent = WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent.bind(null, barStartTime - timeOffset, barEndTime - barStartTime, cpuTime, messageCount);
> 
> I don't quite like the way we associate state with element by re-binding function arguments. Also, we normally use underscore to prefix our fields when associating them with DOM elements. How about lastElement._firstTask and lastElement._lastTask (and compute the rest lazily).

done

>> Source/WebCore/inspector/front-end/TimelinePanel.js:928
>> +                lastElement.generatePopupContent = WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent.bind(null, barStartTime - timeOffset, barEndTime - barStartTime, cpuTime, messageCount);
> 
> indent

removed

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:390
>> +    getMinimumRecordTime: function()
> 
> drop get, just minimumRecordTime -- it's cleaner.

removed

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1087
>> +WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent = function(startTime, duration, cpuTime, messageCount, callback) {
> 
> { => next line

fixed

Created attachment 171863 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 171863 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1076
> +WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent = function(info)

So why is this not an instance method? Instead of passing presentationModel via info, we could as well pass it on the call site, as _this_. Another option is to have this method on TimelinePanel, this way you won't have to pass tasks.

Comment on attachment 171863 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1076
>> +WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent = function(info)
> 
> So why is this not an instance method? Instead of passing presentationModel via info, we could as well pass it on the call site, as _this_. Another option is to have this method on TimelinePanel, this way you won't have to pass tasks.

Passing tasks prevents out-of index in case tasks are cleared, but refresh is deferred.
Instance method will make little to nothing difference: 
1) we will need to take pointer to presentation model in place we use it; 
2) closure couldn't check types; when we use static method - we at least declare types

Moving to TimelinePanel will require making public several things declared if presentation model (but extensively referenced -> more than 25 changes)

[Andrey Kosyakov]

Comment on attachment 171863 [details]
Patch

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

>>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1076
>>> +WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent = function(info)
>> 
>> So why is this not an instance method? Instead of passing presentationModel via info, we could as well pass it on the call site, as _this_. Another option is to have this method on TimelinePanel, this way you won't have to pass tasks.
> 
> Passing tasks prevents out-of index in case tasks are cleared, but refresh is deferred.
> Instance method will make little to nothing difference: 
> 1) we will need to take pointer to presentation model in place we use it; 
> 2) closure couldn't check types; when we use static method - we at least declare types
> 
> Moving to TimelinePanel will require making public several things declared if presentation model (but extensively referenced -> more than 25 changes)

1) -- I don't quite follow. You only refer to this method in TimelinePanel, you could bind it to this._presentationModel there.

Re moving to TimelinePanel - you just use _minimumRecordTime from PresentationModel, you could as well take it from TimelineModel.

Comment on attachment 171863 [details]
Patch

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

>>>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1076
>>>> +WebInspector.TimelinePresentationModel.generateMainThreadBarPopupContent = function(info)
>>> 
>>> So why is this not an instance method? Instead of passing presentationModel via info, we could as well pass it on the call site, as _this_. Another option is to have this method on TimelinePanel, this way you won't have to pass tasks.
>> 
>> Passing tasks prevents out-of index in case tasks are cleared, but refresh is deferred.
>> Instance method will make little to nothing difference: 
>> 1) we will need to take pointer to presentation model in place we use it; 
>> 2) closure couldn't check types; when we use static method - we at least declare types
>> 
>> Moving to TimelinePanel will require making public several things declared if presentation model (but extensively referenced -> more than 25 changes)
> 
> 1) -- I don't quite follow. You only refer to this method in TimelinePanel, you could bind it to this._presentationModel there.
> 
> Re moving to TimelinePanel - you just use _minimumRecordTime from PresentationModel, you could as well take it from TimelineModel.

We also need to disclose PopupContent helper fields - that is what i was talking about

Created attachment 172015 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 172015 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 172015 [details]
Patch

Clearing flags on attachment: 172015

Committed r133310: <http://trac.webkit.org/changeset/133310>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.