Bug 144346

<rdar://problem/20732420>

Looks good. I'd horizontally center the header above the graph.

Created attachment 252124 [details] [Patch] Proposed Fix

(In reply to comment #2) > Looks good. I'd horizontally center the header above the graph. I ended up going with a left-aligned title. I'll add some screenshots.

Created attachment 252125 [details] UI Screenshot - selected frames

Created attachment 252126 [details] UI Screenshot - no selected frames

Comment on attachment 252124 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=252124&action=review > Source/WebInspectorUI/ChangeLog:45 > + The chart now has beeter support for adding empty data points, ensuring that a meaningful legend is shown even beeter

(In reply to comment #7) > Comment on attachment 252124 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252124&action=review > > > Source/WebInspectorUI/ChangeLog:45 > > + The chart now has beeter support for adding empty data points, ensuring that a meaningful legend is shown even > > beeter I should know beeter than that.

Created attachment 252139 [details] [Patch] Proposed Fix

Comment on attachment 252139 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=252139&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:87 > + this._chartColors = { > + layout: "rgb(212, 108, 108)", > + script: "rgb(153, 113, 185)", > + other: "rgb(221, 221, 221)", > + idle: "rgb(255, 255, 255)" > + }; This is fine, but it should probably be a Map with WebInspector.TimelineRecord.Type.Layout, etc as the keys. Except I wouldn't know what to do with Other and Idle… > Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:704 > + this._frameSelectionChartRow.title = WebInspector.UIString("Frames: %d â %d (%s â %s)").format( > + firstRecord.frameNumber, > + lastRecord.frameNumber, > + Number.secondsToString(firstRecord.startTime), > + Number.secondsToString(lastRecord.endTime)); Style: I'd wrap this on two lines max: this._frameSelectionChartRow.title = WebInspector.UIString("Frames: %d â %d (%s â %s)").format(firstRecord.frameNumber, lastRecord.frameNumber, Number.secondsToString(firstRecord.startTime), Number.secondsToString(lastRecord.endTime)); > Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:709 > + this._frameSelectionChartRow.title = WebInspector.UIString("Frame: %d (%s â %s)").format( > + firstRecord.frameNumber, > + Number.secondsToString(firstRecord.startTime), > + Number.secondsToString(lastRecord.endTime)); Ditto.

Comment on attachment 252125 [details] UI Screenshot - selected frames I think the chart and top label should be centered in the sidebar. What happens if the sidebar is small?

Comment on attachment 252124 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=252124&action=review > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:61 > + set title(x) Nit: I would rename "x" to "title" with the hope that the change increases readability. Reading "x" I thought the parameter is of type Number. Of course my suggestion can't take the place of a type check :) > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:70 > set innerLabel(x) Nit: Same here. I would rename "x" to "label". > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:80 > + set innerRadius(x) Nit: Here "x" makes more sense imho, although I would rename "x" to "r" or "radius". > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:111 > + if (value > 0) { Now that you have a delegate that is supposed to format the value I wonder why it shouldn't also format "0"? Consider removing the condition. > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:126 > + this._refresh(); General question: After reading the code it feels `_refresh` is called very frequently and `_refresh` does DOM (Canvas) read and writes, right? In case you see a performance problem, I'd consider introducing a "markAsDirty" functionality that could help batching up DOM read and write operations. Imho that would make sense for all UI operations in WebInspector. Such functionality often helps understanding the intended rendering lifecycle. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:667 > + if (Math.floor(startIndex) <= this._renderingFrameTimeline.records.length) Is a check necessary here that makes sure that `this._renderingFrameTimeline` is not null? Saw that in other functions.

(In reply to comment #12) > Comment on attachment 252124 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252124&action=review > > > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:61 > > + set title(x) > > Nit: I would rename "x" to "title" with the hope that the change increases > readability. Reading "x" I thought the parameter is of type Number. Of > course my suggestion can't take the place of a type check :) > > > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:70 > > set innerLabel(x) > > Nit: Same here. I would rename "x" to "label". > > > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:80 > > + set innerRadius(x) > > Nit: Here "x" makes more sense imho, although I would rename "x" to "r" or > "radius". > > > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:111 > > + if (value > 0) { > > Now that you have a delegate that is supposed to format the value I wonder > why it shouldn't also format "0"? Consider removing the condition. > > > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:126 > > + this._refresh(); > > General question: After reading the code it feels `_refresh` is called very > frequently and `_refresh` does DOM (Canvas) read and writes, right? In case > you see a performance problem, I'd consider introducing a "markAsDirty" > functionality that could help batching up DOM read and write operations. > Imho that would make sense for all UI operations in WebInspector. Such > functionality often helps understanding the intended rendering lifecycle. In other places, the actual DOM updates and canvas writes are coalesced into a requestAnimationFrame. (Not relevant to this patch's intent, but) I was surprised to find that the chart is implemented in canvas instead of SVG. Wouldn't SVG be a lot easier?

Comment on attachment 252124 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=252124&action=review >>> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:61 >>> + set title(x) >> >> Nit: I would rename "x" to "title" with the hope that the change increases readability. Reading "x" I thought the parameter is of type Number. Of course my suggestion can't take the place of a type check :) > > In other places, the actual DOM updates and canvas writes are coalesced into a requestAnimationFrame. > > (Not relevant to this patch's intent, but) I was surprised to find that the chart is implemented in canvas instead of SVG. Wouldn't SVG be a lot easier? > In other places, the actual DOM updates and canvas writes are coalesced into a requestAnimationFrame. +1 > Wouldn't SVG be a lot easier? + You wouldn't need to take the devicePixelRatio into consideration +/- The Piechart is not intended to be animated right?, So Performance wouldn't be an issue. - The "arc" API is more complicated What other reasons are you thinking of?

> What happens if the sidebar is small? The minimum sidebar width is wide enough to hold the chart and legend. The title will use ellipses if clipped, but I haven't seen this occur.

> > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:111 > > + if (value > 0) { > > Now that you have a delegate that is supposed to format the value I wonder > why it shouldn't also format "0"? Consider removing the condition. Agree. In looking into this I noticed a subtle problem. If one chart value is zero and the others aren't, you get a legend like this: [ ] Layout X ms [ ] Script <-- ugly! should be "0 ms" [ ] Other X ms [ ] Idle X ms The decision to format zero as "" or "0 ms" will be moved to the delegate. > > Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:126 > > + this._refresh(); > > General question: After reading the code it feels `_refresh` is called very > frequently and `_refresh` does DOM (Canvas) read and writes, right? In case > you see a performance problem, I'd consider introducing a "markAsDirty" > functionality that could help batching up DOM read and write operations. > Imho that would make sense for all UI operations in WebInspector. Such > functionality often helps understanding the intended rendering lifecycle. Good point. This is beyond the scope of this patch, but we can at least make sure the chart refreshes only when necessary by simply adding a few checks before repainting.

Created attachment 252239 [details] [Patch] Proposed Fix

Comment on attachment 252239 [details] [Patch] Proposed Fix Clearing flags on attachment: 252239 Committed r183721: <http://trac.webkit.org/changeset/183721>

All reviewed patches have been landed. Closing bug.