Created attachment 145772 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 145772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145772&action=review > Source/WebCore/inspector/InspectorTimelineAgent.cpp:157 > + m_taskEventStartTime = std::numeric_limits<double>::quiet_NaN(); nit: I think we could as well use 0.0, we do this elsewhere. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:172 > + m_taskEventStartTime = std::numeric_limits<double>::quiet_NaN(); ditto. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:193 > + *result = m_client ? m_client->supportsFrameInstrumentation() : false; nit: m_client && m_client->supportsFrameInstrumentation() > Source/WebCore/inspector/InspectorTimelineAgent.cpp:462 > + counters->setNumber("nodes", (m_client) ? InspectorCounters::counterValue(InspectorCounters::NodeCounter) : 0); > + counters->setNumber("documents", (m_client) ? InspectorCounters::counterValue(InspectorCounters::DocumentCounter) : 0); Checking for client presence to determine inspector type looks like a hack here. I think we might as well keep inspector type explicit, though at the moment it will be actually redundant. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:474 > + TaskEvents events = m_taskEvents; > + m_taskEvents.clear(); You can avoid copying a vector here by using Vector::swap() > Source/WebCore/inspector/InspectorTimelineAgent.cpp:476 > + for (TaskEvents::iterator i = events.begin(); i != events.end(); ++i) { We normally use it for iterators. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:477 > + RefPtr<InspectorObject> task = TimelineRecordFactory::createMessageLoopTaskData(i->startTime, i->endTime); I think this method could accommodate the entire loop. In either case, though, it will not be a good match of the original purpose of TimelineRecordFactory -- we're not creating a complete record. Guess it may be ok. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:-464 > - , m_frameInstrumentationSupport(frameInstrumentationSupport) As above -- though having to pass inspector type to timeline agent is perhaps unfortunate, masking it as a check for client presence looks even worse. > Source/WebCore/inspector/InspectorTimelineAgent.h:194 > + TaskEvent(double startTime, double endTime) > + : startTime(startTime), endTime(endTime) Given it's only internal, I think we could as well use std::pair<double, double> > Source/WebCore/inspector/front-end/TimelinePanel.js:815 > + computeTime: function(position) I don't think this needs to be in calculator. If it did, it would have to be in _all_ implementations of the calculator. Just use it in place. > Source/WebCore/inspector/front-end/TimelinePanel.js:1067 > +WebInspector.TimelineMessageLoopModel = function(model) I wonder why we need a separate model for that? It doesn't do a lot and not shared between views, so perhaps this could be done by the view itself? > Source/WebCore/inspector/front-end/TimelinePanel.js:1087 > + if (!tasks || tasks.length == 0) tasks.length == 0 => !tasks.length > Source/WebCore/inspector/front-end/TimelinePanel.js:1095 > + for (var i = 0; i < tasks.length; ++i) { > + var task = tasks[i]; > + this._tasks.push({ > + startTime: task["startTime"], > + endTime: task["endTime"] > + }); > + } doesn't this._tasks.push.apply(this._tasks, tasks) just cut it? > Source/WebCore/inspector/front-end/TimelinePanel.js:1130 > + if (!width) > + return; when do we hit that? > Source/WebCore/inspector/front-end/TimelinePanel.js:1133 > + var startTime = this._calculator.computeTime(0) * 1000; > + var endTime = this._calculator.computeTime(width) * 1000; as above -- let's do this without the calculator. > Source/WebCore/inspector/front-end/TimelinePanel.js:1139 > + context.clearRect(0, 0, width, height); this seems redundant, assigning to width/height should take care of this. > Source/WebCore/inspector/front-end/TimelinePanel.js:1143 > + var delta = endTime - startTime; we call this timeSpan elsewhere > Source/WebCore/inspector/front-end/TimelinePanel.js:1144 > + var step = delta / width; scale? > Source/WebCore/inspector/front-end/TimelinePanel.js:1150 > + return value < task.endTime ? -1 : 1; just return value - task.endTime > Source/WebCore/inspector/front-end/TimelinePanel.js:1153 > + var taskIndex = insertionIndexForObjectInListSortedByFunction(startTime, tasks, compareEndTime); Aren't tasks already sorted? > Source/WebCore/inspector/front-end/TimelinePanel.js:1154 > + if (taskIndex == tasks.length) { == => === > Source/WebCore/inspector/front-end/TimelinePanel.js:1155 > + context.clearRect(0, 0, width, height); redundant > Source/WebCore/inspector/front-end/TimelinePanel.js:1195 > + pixels[pixelIndex + 2] = 255; > + pixels[pixelIndex + width * 4 + 2] = 255; > + pixels[pixelIndex + width * 8 + 2] = 255; > + if (sum > threshold) { > + pixels[pixelIndex + 3] = 255; > + pixels[pixelIndex + width * 4 + 3] = 128; > + pixels[pixelIndex + width * 8 + 3] = 64; > + } else if (sum > 0) { > + pixels[pixelIndex + 3] = 128; > + pixels[pixelIndex + width * 4 + 3] = 64; > + pixels[pixelIndex + width * 8 + 3] = 32; > + } > + > + pixelIndex += 4; > + } > + context.putImageData(imageData, 0, 0); This looks a bit cryptic. Any reasons not to use higher-level calls, like lineTo()? Comment on attachment 145772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145772&action=review >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:157 >> + m_taskEventStartTime = std::numeric_limits<double>::quiet_NaN(); > > nit: I think we could as well use 0.0, we do this elsewhere. Done. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:172 >> + m_taskEventStartTime = std::numeric_limits<double>::quiet_NaN(); > > ditto. Done. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:193 >> + *result = m_client ? m_client->supportsFrameInstrumentation() : false; > > nit: m_client && m_client->supportsFrameInstrumentation() Done. Thanks. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:462 >> + counters->setNumber("documents", (m_client) ? InspectorCounters::counterValue(InspectorCounters::DocumentCounter) : 0); > > Checking for client presence to determine inspector type looks like a hack here. I think we might as well keep inspector type explicit, though at the moment it will be actually redundant. Done. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:474 >> + m_taskEvents.clear(); > > You can avoid copying a vector here by using Vector::swap() Done. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:476 >> + for (TaskEvents::iterator i = events.begin(); i != events.end(); ++i) { > > We normally use it for iterators. Done. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:477 >> + RefPtr<InspectorObject> task = TimelineRecordFactory::createMessageLoopTaskData(i->startTime, i->endTime); > > I think this method could accommodate the entire loop. In either case, though, it will not be a good match of the original purpose of TimelineRecordFactory -- we're not creating a complete record. Guess it may be ok. Done. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:-464 >> - , m_frameInstrumentationSupport(frameInstrumentationSupport) > > As above -- though having to pass inspector type to timeline agent is perhaps unfortunate, masking it as a check for client presence looks even worse. Fixed. >> Source/WebCore/inspector/InspectorTimelineAgent.h:194 >> + : startTime(startTime), endTime(endTime) > > Given it's only internal, I think we could as well use std::pair<double, double> Done. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1067 >> +WebInspector.TimelineMessageLoopModel = function(model) > > I wonder why we need a separate model for that? It doesn't do a lot and not shared between views, so perhaps this could be done by the view itself? Combined. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1087 >> + if (!tasks || tasks.length == 0) > > tasks.length == 0 => !tasks.length Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1095 >> + } > > doesn't this._tasks.push.apply(this._tasks, tasks) just cut it? I was just unsure, if it is good to hold InspectorObjects. Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1130 >> + return; > > when do we hit that? When user makes inspector window too small. Checked to avoid division by zero. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1139 >> + context.clearRect(0, 0, width, height); > > this seems redundant, assigning to width/height should take care of this. Done. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1143 >> + var delta = endTime - startTime; > > we call this timeSpan elsewhere ok >> Source/WebCore/inspector/front-end/TimelinePanel.js:1144 >> + var step = delta / width; > > scale? ok >> Source/WebCore/inspector/front-end/TimelinePanel.js:1150 >> + return value < task.endTime ? -1 : 1; > > just return value - task.endTime No. We need to avoid returning zero. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1153 >> + var taskIndex = insertionIndexForObjectInListSortedByFunction(startTime, tasks, compareEndTime); > > Aren't tasks already sorted? They are. That is why binary search works fine =) >> Source/WebCore/inspector/front-end/TimelinePanel.js:1154 >> + if (taskIndex == tasks.length) { > > == => === fixed >> Source/WebCore/inspector/front-end/TimelinePanel.js:1155 >> + context.clearRect(0, 0, width, height); > > redundant fixed >> Source/WebCore/inspector/front-end/TimelinePanel.js:1195 >> + context.putImageData(imageData, 0, 0); > > This looks a bit cryptic. Any reasons not to use higher-level calls, like lineTo()? Simplified a bit. Pixel manipulation seems to be appropriate for a pixel-sized graphics. I'm OK to rewrite it to higher-level calls, when design is approved. Created attachment 145811 [details]
Updated demo screenshot
Created attachment 145812 [details]
Patch
Comment on attachment 145812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145812&action=review > Source/WebKit/chromium/public/WebDevToolsAgent.h:87 > + // Instrumentation method that marks beginning of task processing Could you put these before the static functions? Can you have multiple agents in a process (I think yes)? If so, are these calls made on each agent? > Source/WebKit/chromium/public/WebDevToolsAgentClient.h:65 > + virtual void startMessageLoopMonitoring() { } > + virtual void stopMessageLoopMonitoring() { } could you put these closer to the WebKitClientMessageLoop code (assuming it's related) and document what these functions do, please? Comment on attachment 145812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145812&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:1092 > + if (!record || !record["data"]) No need to check for record. > Source/WebCore/inspector/front-end/TimelinePanel.js:1096 > + if (!tasks || !tasks.length) task.length check appears redundant now. > > when do we hit that? > > When user makes inspector window too small. > Checked to avoid division by zero. x / 0 === Infinity in JS, so I think it's probably redundant. > I'm OK to rewrite it to higher-level calls, when design is approved. Speaking of the appearance, I would vote for moving the line to the lower border of time scale and making it black. Something like 10ms ___ _ _ ___ 20ms ___ _ _ 30ms etc. Alternatively, it may be represented as darker background areas taking full height of time scale. Pavel, WDYT? Created attachment 145958 [details]
Yet another message loop bar appearance.
> Alternatively, it may be represented as darker background areas taking full height of time scale.
>
> Pavel, WDYT?
I like the dark background idea. So that there is a sequence of white balloons on dark background. Those balloons represent CPU activity and contain existing bars within. I would even paint main timeline area accordingly (with dark gaps), not sure how feasible that is though. Such not ation would apply to all modes which is a plus.
Comment on attachment 145812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145812&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:1092 >> + if (!record || !record["data"]) > > No need to check for record. OK >> Source/WebCore/inspector/front-end/TimelinePanel.js:1096 >> + if (!tasks || !tasks.length) > > task.length check appears redundant now. Removed >> Source/WebKit/chromium/public/WebDevToolsAgent.h:87 >> + // Instrumentation method that marks beginning of task processing > > Could you put these before the static functions? > > Can you have multiple agents in a process (I think yes)? If so, are these calls made on each agent? Done. I've attached 3 diagrams that demonstrate that there is an implicit 1-to-1 relationship between WebDevToolsAgent and WebDevToolsAgentClient. So the contract for these new methods is: WebDevToolsAgentClient is controlling the notifications subscription for corresponding WebDevToolsAgent. Some commentaries on diagrams: 1) WebView has an implicit contract to hold both WebDevToolsAgent and WebDevToolsAgentClient references. 2) Implementation of WebDevToolsAgent holds reference to WebDevToolsAgentClient. 3) Implicit contract is turned to explicit in chromium: WebDevToolsAgentClient implementation has access to corresponding WebDevToolsAgent. >> Source/WebKit/chromium/public/WebDevToolsAgentClient.h:65 >> + virtual void stopMessageLoopMonitoring() { } > > could you put these closer to the WebKitClientMessageLoop code (assuming it's related) and document what these functions do, please? Done. Created attachment 145975 [details]
Diagram1: WebView has an implicit contract to hold both WebDevToolsAgent and WebDevToolsAgentClient references
Created attachment 145976 [details]
Diagram 2: Implementation of WebDevToolsAgent holds reference to WebDevToolsAgentClient
Created attachment 145977 [details]
Diagram 3: Implicit contract is turned to explicit in chromium: WebDevToolsAgentClient implementation has access to corresponding WebDevToolsAgent.
Created attachment 145980 [details]
Patch
Created attachment 146022 [details]
UI: take 4
Created attachment 146023 [details]
UI: take 5
(In reply to comment #18) > UI: take 5 take 4 is my favorite so far. How would it look if we're zoomed in close enough to see individual events? Created attachment 146025 [details]
UI: take 5, close-up
Created attachment 146033 [details]
UI: take 6
Created attachment 146034 [details]
UI: take 6, close-up
Created attachment 146035 [details]
UI: take 6 in light-blue
Created attachment 146036 [details]
UI: take 6 in light-blue, close-up
Light blue is definitely wrong. It is also not clear whether gray area is "busy" or "idle" time. Even when I know that it is one of the two, I can't decide which one it is. We know for sure that idle time can't intersect with any of the bars. From the mocks, https://bug-88325-attachments.webkit.org/attachment.cgi?id=146025 seems most reasonable, but I would 1) draw a solid gray spanning line above the chunks ans 2) remove the garbage dots that appear on that bar. Created attachment 146223 [details]
UI: take 7: something between 4 and 5 with removed garbage dots
Created attachment 146224 [details]
UI: take 7, close-up
All I see is a trail of pixel poop. How is the user suppose to comprehend what this is? (In reply to comment #25) > From the mocks, https://bug-88325-attachments.webkit.org/attachment.cgi?id=146025 seems most reasonable, but I would 1) draw a solid gray spanning line above the chunks ans 2) remove the garbage dots that appear on that bar. So... crazy question man asks "why not just put an event into the lower half of the timeline?" Assume this for a moment: * real_events= array of [start, finish, name] times * message_loop_events = array of [start,finish] times Roughly, go through each real event. For each real event, find out what message_loop event it is inside. If the real event is more than 75% of the message loop event, delete the message loop event. Otherwise, create two "unknown work" events that cover the non-instrumented time and add them to the timeline. E.g.: real_events= [0,9,x], [20,22,y] message_loop_events = [0,10], [20, 30], [35,40] Result: timeline_events= [0,9,x],[20,22,y],[22,30,unknown],[35,40,unknown] And, don't show anything up in the top graph. Could you split the code changes out to a separate bug from the UI discussion? I have more feedback to provide on some of the wiring but not so much about the UI. (In reply to comment #30) > Could you split the code changes out to a separate bug from the UI discussion? I have more feedback to provide on some of the wiring but not so much about the UI. Done. See: https://bugs.webkit.org/show_bug.cgi?id=88639 Created attachment 146562 [details]
Screenshot: close view on "unknown" tasks
Created attachment 146563 [details]
Screenshot: "unknown" tasks pollute timeline
Created attachment 146564 [details]
Screenshot: same data, with switched off "unknown" category
(In reply to comment #29) > (In reply to comment #25) > > From the mocks, https://bug-88325-attachments.webkit.org/attachment.cgi?id=146025 seems most reasonable, but I would 1) draw a solid gray spanning line above the chunks ans 2) remove the garbage dots that appear on that bar. > > So... crazy question man asks "why not just put an event into the lower half of the timeline?" > > Assume this for a moment: > * real_events= array of [start, finish, name] times > * message_loop_events = array of [start,finish] times > > Roughly, go through each real event. For each real event, find out what message_loop event it is inside. If the real event is more than 75% of the message loop event, delete the message loop event. Otherwise, create two "unknown work" events that cover the non-instrumented time and add them to the timeline. > > E.g.: > real_events= [0,9,x], [20,22,y] > message_loop_events = [0,10], [20, 30], [35,40] > > Result: > timeline_events= [0,9,x],[20,22,y],[22,30,unknown],[35,40,unknown] > > And, don't show anything up in the top graph. Done. But we have a problem here: a lot of small "unknown" tasks pollute timeline (see screenshots). They appear, because these message loops have "content" represented on timeline, that way "unknown" time is just 100%. We can't just filter them out, because we may get to situation, when reasonable amount of CPU time is spent in a ton of tiny message loops. The only solution I see - turn-off "unknown" category by default. > We can't just filter them out, because we may get to situation, when reasonable amount of CPU time is spent in a ton of tiny message loops. Understood. I wonder if you can coalesce... e.g. "[0,1][1.05->2]" --> "[0,2]"? > The only solution I see - turn-off "unknown" category by default. That seems okay to me. Or, if timeline could show more than 1 event on a row, that would work too ... part of your issue is that timeline is very inefficient in its use of vertical space. Comment on attachment 145980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145980&action=review Clearing r? - it has been partially landed. > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Please explain what is happening in the change. Created attachment 147848 [details]
Patch
(In reply to comment #38) > Created an attachment (id=147848) [details] > Patch A partial patch: this functionality should be disabled by default, and enabled with "experiment" settings checkbox. Created attachment 147849 [details]
Demo screenshot
Created attachment 148176 [details]
Patch
Created attachment 150126 [details]
Patch
Created attachment 150623 [details]
Patch
Rebased
Comment on attachment 150623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150623&action=review > Source/WebCore/inspector/front-end/Settings.js:190 > + this.mainThreadMonitoring = this._createExperiment("mainThreadMonitoring", "Main thread monitoring"); "Show CPU activity in Timeline"? > Source/WebCore/inspector/front-end/TimelinePanel.js:511 > + this._mainThreadTasks.push({startTime: record.startTime / 1000, endTime: record.endTime / 1000}); Please use WebInspector.TimelineModel.{start,end}TimeInSeconds() > Source/WebCore/inspector/front-end/TimelinePanel.js:769 > + _refreshCpuBars: function() Here and below, Cpu => CPU > Source/WebCore/inspector/front-end/TimelinePanel.js:773 > + var startTime = this._calculator.computeTime(0); > + var endTime = this._calculator.computeTime(width); this._overviewPane.window{Start,End}Time()? > Source/WebCore/inspector/front-end/TimelinePanel.js:801 > + var left = Math.max(0, this._calculator.computePosition(task.startTime) + WebInspector.TimelinePanel.barOffset); > + var right = Math.min(width, this._calculator.computePosition(task.endTime) + WebInspector.TimelineCalculator._minWidth + WebInspector.TimelinePanel.barOffset); barOffset is apparently only used here, so please define as a local const. > Source/WebCore/inspector/front-end/TimelinePanel.js:809 > + } else { > + lastElement.style.width = (lastRight - lastLeft) + "px"; > + } We don't use else after flow control statements (e.g. continue). Also, curly braces are redundant. > Source/WebCore/inspector/front-end/TimelinePanel.js:833 > + _enableMainThreadMonitoringExperiment: function() So you're looking to rename this method once it graduates out of experimental? > Source/WebCore/inspector/front-end/TimelinePanel.js:835 > + this._headerLineCount = 2; ++this._headerLineCount? > Source/WebCore/inspector/front-end/TimelinePanel.js:840 > + cpuBarsLabel.appendChild(document.createTextNode("CPU")); cpuBarsLabel.textContent = ...? Also, please use UIString for that. > Source/WebCore/inspector/front-end/TimelinePanel.js:845 > + container.style.height = headerHeight + WebInspector.TimelinePanel.headerMargin + "px"; > + this._timelineGrid.dividersLabelBarElement.style.height = headerHeight + WebInspector.TimelinePanel.headerMargin + "px"; headerMargin is apparently only used here -- please define it as a local const. > Source/WebCore/inspector/front-end/TimelinePanel.js:846 > + this._itemsGraphsElement.style.top = headerHeight + WebInspector.TimelinePanel.headerBorderWidth + "px"; ditto for headerBorderWidth > Source/WebCore/inspector/front-end/TimelinePanel.js:951 > + computeTime: function(position) I think this is confusing -- we have a few implementation of calculator, and you're just adding to one, and only use this method in one place. > Source/WebCore/inspector/front-end/TimelinePanel.js:999 > + this.clientWidth = clientWidth; This also looks like abuse of calculator class. > Source/WebCore/inspector/front-end/timelinePanel.css:632 > + font-family: monospace; Consider using monospace class instead. > Source/WebCore/inspector/front-end/timelinePanel.css:634 > + font-size: 9px; > + line-height: 7px; line-height < font-size looks fishy. is this intended? > Source/WebCore/inspector/front-end/timelinePanel.css:640 > + z-index: 350; Is this necessary? Comment on attachment 150623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150623&action=review >> Source/WebCore/inspector/front-end/Settings.js:190 >> + this.mainThreadMonitoring = this._createExperiment("mainThreadMonitoring", "Main thread monitoring"); > > "Show CPU activity in Timeline"? OK >> Source/WebCore/inspector/front-end/TimelinePanel.js:511 >> + this._mainThreadTasks.push({startTime: record.startTime / 1000, endTime: record.endTime / 1000}); > > Please use WebInspector.TimelineModel.{start,end}TimeInSeconds() Sure. >> Source/WebCore/inspector/front-end/TimelinePanel.js:769 >> + _refreshCpuBars: function() > > Here and below, Cpu => CPU Done (for acronyms in the middle of name). >> Source/WebCore/inspector/front-end/TimelinePanel.js:773 >> + var endTime = this._calculator.computeTime(width); > > this._overviewPane.window{Start,End}Time()? No, because: this._overviewPane.windowStartTime() == this._calculator.computeTime(this._expandOffset) != this._calculator.computeTime(0) >> Source/WebCore/inspector/front-end/TimelinePanel.js:801 >> + var right = Math.min(width, this._calculator.computePosition(task.endTime) + WebInspector.TimelineCalculator._minWidth + WebInspector.TimelinePanel.barOffset); > > barOffset is apparently only used here, so please define as a local const. OK. >> Source/WebCore/inspector/front-end/TimelinePanel.js:809 >> + } > > We don't use else after flow control statements (e.g. continue). Also, curly braces are redundant. Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:833 >> + _enableMainThreadMonitoringExperiment: function() > > So you're looking to rename this method once it graduates out of experimental? Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:835 >> + this._headerLineCount = 2; > > ++this._headerLineCount? OK. >> Source/WebCore/inspector/front-end/TimelinePanel.js:840 >> + cpuBarsLabel.appendChild(document.createTextNode("CPU")); > > cpuBarsLabel.textContent = ...? Also, please use UIString for that. Done. >> Source/WebCore/inspector/front-end/TimelinePanel.js:845 >> + this._timelineGrid.dividersLabelBarElement.style.height = headerHeight + WebInspector.TimelinePanel.headerMargin + "px"; > > headerMargin is apparently only used here -- please define it as a local const. Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:846 >> + this._itemsGraphsElement.style.top = headerHeight + WebInspector.TimelinePanel.headerBorderWidth + "px"; > > ditto for headerBorderWidth Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:951 >> + computeTime: function(position) > > I think this is confusing -- we have a few implementation of calculator, and you're just adding to one, and only use this method in one place. Why don't we have common interface / super type then? As I see, all calculators have methods that are used for specific purposes. >> Source/WebCore/inspector/front-end/timelinePanel.css:632 >> + font-family: monospace; > > Consider using monospace class instead. No. monospace overrides font size and definitely wins in selector matching. >> Source/WebCore/inspector/front-end/timelinePanel.css:634 >> + line-height: 7px; > > line-height < font-size looks fishy. is this intended? Sure. font-size = |font ascending| + |font descending| > |font cap height| Actually text is 5px height + 2 * 1px padding = 7px line height. >> Source/WebCore/inspector/front-end/timelinePanel.css:640 >> + z-index: 350; > > Is this necessary? Otherwise it will be hidden by bars. Created attachment 150731 [details]
Patch
Comment on attachment 150731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150731&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:513 > + if (record.type === WebInspector.TimelineModel.RecordType.Program) > + this._mainThreadTasks.push({ > + startTime: WebInspector.TimelineModel.startTimeInSeconds(record), > + endTime: WebInspector.TimelineModel.endTimeInSeconds(record) > + }); This spans multiple lines, so needs {} around the statement. > Source/WebCore/inspector/front-end/TimelinePanel.js:777 > + var startTime = this._calculator.computeTime(0); > + var endTime = this._calculator.computeTime(width); thought we agreed that we should not put additional methods on the calculator. Created attachment 150749 [details]
Patch
Created attachment 150751 [details]
Patch
Comment on attachment 150731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150731&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:513 >> + }); > > This spans multiple lines, so needs {} around the statement. Fixed >> Source/WebCore/inspector/front-end/TimelinePanel.js:777 >> + var endTime = this._calculator.computeTime(width); > > thought we agreed that we should not put additional methods on the calculator. Fixed. Created attachment 151692 [details]
Patch
Created attachment 151890 [details]
Patch
fixed-frame-dividers
Comment on attachment 151890 [details]
Patch
LGTM
Comment on attachment 151890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151890&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:375 > + /** @const */ var frameContainerBorderWidth = 1; /** @const */ const frameContainerBorderWidth = 1; > Source/WebCore/inspector/front-end/TimelinePanel.js:812 > + if (Math.ceil(lastRight) >= Math.floor(left)) { We may end up with as many chunks as pixels. You should introduce the minimal chunk width of 3px and make sure that gaps are longer than 3px as well. Or alternatively you could render it all using canvas! Comment on attachment 151890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151890&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:812 >> + if (Math.ceil(lastRight) >= Math.floor(left)) { > > We may end up with as many chunks as pixels. You should introduce the minimal chunk width of 3px and make sure that gaps are longer than 3px as well. Or alternatively you could render it all using canvas! Actually: 1) this line ensures that we may end up with half that number, because we create new chunk only when ceil(current) < floor(next), so there more than one pixel between chunks; 2) previous line (right =...) mimics main timeline bars appearance, so minimal width is 3 px. As a consequence, at most pixelWidth / 4 chunks will be displayed. > Actually: > 1) this line ensures that we may end up with half that number, because we create new chunk only when ceil(current) < floor(next), so there more than one pixel between chunks; In case current and next are integers, and current is 3 and next is 4, we will hit a chunk per pixel. Event half of the pixels is too much (1400 chunks on retina display?). I think I am just looking for the ways of making it more obvious from the code. Today, I need to think for quite some time before I figure out what the actual threshold is. Having it stated in a declarative manner would be great. > 2) previous line (right =...) mimics main timeline bars appearance, so minimal width is 3 px. > > As a consequence, at most pixelWidth / 4 chunks will be displayed. Exactly, I don't want it to be a consequence, but rather a clearly stated fact in the code. (In reply to comment #56) > > Actually: > > 1) this line ensures that we may end up with half that number, because we create new chunk only when ceil(current) < floor(next), so there more than one pixel between chunks; > > In case current and next are integers, and current is 3 and next is 4, we will hit a chunk per pixel. Event half of the pixels is too much (1400 chunks on retina display?). I think I am just looking for the ways of making it more obvious from the code. Today, I need to think for quite some time before I figure out what the actual threshold is. Having it stated in a declarative manner would be great. > > > 2) previous line (right =...) mimics main timeline bars appearance, so minimal width is 3 px. > > > > As a consequence, at most pixelWidth / 4 chunks will be displayed. > > Exactly, I don't want it to be a consequence, but rather a clearly stated fact in the code. On retina display we will have the same number of "pixels" -> no more than 320 chunks. Extracted explicit gap constant, PTAL. Created attachment 152202 [details]
Patch
Created attachment 152216 [details]
Patch
fixed-nits
Committed r122706: <http://trac.webkit.org/changeset/122706> |
Created attachment 145766 [details] Demo screenshot Message loop instrumentation will show when the render thread is busy. That way developer can discover if a render thread business causes low fps, or not.