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
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
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
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)
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
Comment on attachment 172015 [details] Patch LGTM
Comment on attachment 172015 [details] Patch Clearing flags on attachment: 172015 Committed r133310: <http://trac.webkit.org/changeset/133310>
All reviewed patches have been landed. Closing bug.