We'd like to reuse frame information in both overview pane and lower pane, so this should be moved out to a separate class.
Created attachment 134824 [details] Patch
Comment on attachment 134824 [details] Patch Attachment 134824 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12288282
Comment on attachment 134824 [details] Patch Attachment 134824 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12266359
Comment on attachment 134824 [details] Patch Please add the new file to the change.
Created attachment 134828 [details] Patch
Comment on attachment 134828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134828&action=review It sounds like this change is not only factoring out what is claimed, but makes random changes to the code. Could you please split it? > Source/WebCore/ChangeLog:8 > + * WebCore.gypi: Added TimelineFrameController.js Where is the new file? > Source/WebCore/inspector/front-end/TimelineFrameController.js:52 > + this._addRecord(event.data); Do we need to add this when panel is not shown? > Source/WebCore/inspector/front-end/TimelineFrameController.js:87 > + console.error("shynthetic frame"); Please remove the debug logging. > Source/WebCore/inspector/front-end/TimelineModel.js:105 > + var categories = Object.keys(increment); Please annotate this, btw we do for in all over the place. > Source/WebCore/inspector/front-end/TimelineModel.js:116 > + var childrenTime = 0; Please annotate this. > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:-931 > - var totalTime = 0; Where did this method go? > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:885 > + if (!longestFrame || longestFrame.cpuTime < this._frames[currentFrame].cpuTime) What is longestFrame?
Created attachment 135044 [details] Patch
(In reply to comment #6) > > It sounds like this change is not only factoring out what is claimed, but makes random changes to the code. Could you please split it? Well, those are not entirely random. I wouldn't like to spit it, as these are closely interconnected. I tried to do a better job of explaining what happens in the ChangeLog. > > Source/WebCore/ChangeLog:8 > > + * WebCore.gypi: Added TimelineFrameController.js > Where is the new file? Fixing. > > Source/WebCore/inspector/front-end/TimelineFrameController.js:52 > > + this._addRecord(event.data); > Do we need to add this when panel is not shown? I addressed this as a separate change (bug 82861) > > Source/WebCore/inspector/front-end/TimelineFrameController.js:87 > > + console.error("shynthetic frame"); > Please remove the debug logging. Oops. Thanks for spotting! > > Source/WebCore/inspector/front-end/TimelineModel.js:105 > > + var categories = Object.keys(increment); > Please annotate this, btw we do for in all over the place. done. > > Source/WebCore/inspector/front-end/TimelineModel.js:116 > > + var childrenTime = 0; > > Please annotate this. Done. > > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:-931 > > - var totalTime = 0; > > Where did this method go? We used to do aggregation separately by frames or by top-level events in case we don't have frames. Now we just always simulate frames in TimelineFrameController, and the aggregation happens there as well -- except for the case when we need to show one bar per multiple frames. > > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:885 > > + if (!longestFrame || longestFrame.cpuTime < this._frames[currentFrame].cpuTime) > > What is longestFrame? The frame that took most CPU time.
Committed r112865: <http://trac.webkit.org/changeset/112865>