Bug 144346 – Web Inspector: Relocate the selected range details in the Rendering Frames timeline UI

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Created attachment 251877 [details]
UI mockup

* SUMMARY
Relocate the selected range details in the Rendering Frames timeline UI.

When the Timelines UI is in the Rendering Frames view mode we hide the timelines tree outline, which creates a 108px tall empty space in the sidebar. We should move the selected range chart and details from the details sidebar to this new space, and remove the sidebar (see UI mockup).

[Radar WebKit Bug Importer]

<rdar://problem/20732420>

[Timothy Hatcher]

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

[Matt Baker]

Created attachment 252124 [details]
[Patch] Proposed Fix

[Matt Baker]

(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.

[Matt Baker]

Created attachment 252125 [details]
UI Screenshot - selected frames

[Matt Baker]

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

[Brian Burg]

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

[Matt Baker]

(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.

[Matt Baker]

Created attachment 252139 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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.

[Timothy Hatcher]

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?

[Tobias Reiss]

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.

[Brian Burg]

(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?

[Tobias Reiss]

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?

[Matt Baker]

> 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.

[Matt Baker]

> > 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.

[Matt Baker]

Created attachment 252239 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 252239 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 252239

Committed r183721: <http://trac.webkit.org/changeset/183721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.