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.
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.
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?
> 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 145977[details]
Diagram 3: Implicit contract is turned to explicit in chromium: WebDevToolsAgentClient implementation has access to corresponding WebDevToolsAgent.
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.
(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
(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.
(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.
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.
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.
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.
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.
2012-06-05 05:18 PDT, eustas.bug
2012-06-05 05:53 PDT, eustas.bug
2012-06-05 09:03 PDT, eustas.bug
2012-06-05 09:06 PDT, eustas.bug
2012-06-06 01:21 PDT, eustas.bug
2012-06-06 03:19 PDT, eustas.bug
2012-06-06 03:20 PDT, eustas.bug
2012-06-06 03:22 PDT, eustas.bug
2012-06-06 03:30 PDT, eustas.bug
2012-06-06 07:32 PDT, eustas.bug
2012-06-06 07:32 PDT, eustas.bug
2012-06-06 07:37 PDT, eustas.bug
2012-06-06 07:55 PDT, eustas.bug
2012-06-06 07:55 PDT, eustas.bug
2012-06-06 08:10 PDT, eustas.bug
2012-06-06 08:11 PDT, eustas.bug
2012-06-07 00:52 PDT, eustas.bug
2012-06-07 00:53 PDT, eustas.bug
2012-06-08 07:27 PDT, eustas.bug
2012-06-08 07:28 PDT, eustas.bug
2012-06-08 07:28 PDT, eustas.bug
2012-06-15 10:21 PDT, eustas.bug
2012-06-15 10:23 PDT, eustas.bug
2012-06-18 14:54 PDT, eustas.bug
2012-06-29 03:00 PDT, eustas.bug
2012-07-03 09:09 PDT, eustas.bug
2012-07-04 01:01 PDT, eustas.bug
2012-07-04 02:58 PDT, eustas.bug
2012-07-04 03:06 PDT, eustas.bug
2012-07-11 05:53 PDT, eustas.bug
2012-07-12 02:10 PDT, eustas.bug
2012-07-13 03:06 PDT, eustas.bug
2012-07-13 04:23 PDT, eustas.bug