Bug 146475 – Web Inspector: Rendering Frame bars appear misaligned and contain gaps when displaying small task segments

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Created attachment 255859 [details]
[Image] Subpixel problems

* SUMMARY
Rendering Frame bars appear misaligned and contain gaps when displaying small task segments.

The Rendering Frames overview graph has a fixed height of 108px. Frame time is shown on the vertical axis with a minimum/maximum height of 0.0185 s and 0.037 s. Thus each pixel on the y-axis has a value between 0.171 ms - 0.342 ms.

The duration of a single task can frequently fall below this resolution, resulting in subpixel segment heights. After rasterization and applying CSS styles (our 1px segment separator), these small tasks are invisible, appearing as gaps in the frame. It is especially noticeable when the gap occurs at the bottom of the frame, making it appear to float above the horizontal axis (see screenshot).

* NOTE
Some tasks will inevitably be too small to display. The ideal algorithm would:

1. Throw out tasks that are too small.
2. Pad tasks that are very close to the minimum resolution in order to display them (borrowing "rounded" time from larger tasks).
3. Show all frames, regardless of how small. A brief frame that would be less than a pixel high should be padded to a minimum frame height.
4. Keep the overall frame height and ratio of displayed tasks as accurate as possible.

[Radar WebKit Bug Importer]

<rdar://problem/21620063>

[Matt Baker]

Created attachment 255980 [details]
[Image] Results after applying new algorithm

Three timeline recordings rendered with the new frame-segment algorithm. Shown alongside the same data rendered with the current algorithm for comparison.

[Matt Baker]

Created attachment 256217 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=256217&action=review

Sounds good! r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:99
> +        var currentSegment;

Style: lets do an = null.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:116
> +            if (currentSegment.duration < secondsPerBlock)
> +                invisibleSegments.push({segment: segments.lastValue, index: segments.length - 1});

Nit: segments.lastValue at this point could just be "currentSegment".

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:156
> +        // After grouping sub-block tasks, a second is needed to handle those groups that are still beneath the minimum

Grammaro: "a second is needed" => "a second pass is needed"

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:186
> +            }, this);

Unnecessary "this".

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:197
> +        }, this);

Unnecessary "this".

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:227
> +        this._record.__displayData.segments.forEach(function(segment) {
> +            var element = document.createElement("div");
> +            this._updateElementPosition(element, segment.duration / this._record.__displayData.frameDuration, "height");
> +            element.classList.add("duration", segment.taskType);
> +            frameElement.insertBefore(element, frameElement.firstChild);
>          }, this);

I wonder if it would be better to append in reverse order to avoid a whole bunch of insertBefores? I'm not sure if there is a possible performance cost, especially for very small lists (1-3).

[Brian Burg]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=256217&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:804
> +        return Math.round(num  / step) * step;

weird spacing.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:170
> +            var available = previous && next ? previous.remainder + next.remainder : (previous ? previous.remainder : next.remainder);

Ow, my brain. Try to be consistent about naming (i.e., availableDuration, previousSegment, nextSegment)

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:173
> +                var targetSegment = previous && next ? (previous.duration > next.duration ? previous : next) : previous || next;

Ow, my brain.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:179
> +            var neighbors = previous && next ? (previous.remainder > next.remainder ? [previous, next] : [next, previous]) : [previous || next]

I've written this type of line before. You will regret it next year.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:199
> +        return {frameDuration, segments};

Cool!

[Matt Baker]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=256217&action=review

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:179
>> +            var neighbors = previous && next ? (previous.remainder > next.remainder ? [previous, next] : [next, previous]) : [previous || next]
> 
> I've written this type of line before. You will regret it next year.

I agree, nested ternaries are pretty nasty. I'll rewrite them as if-else for clarity.

[Matt Baker]

Created attachment 256235 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=256235&action=review

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:104
> +            }
> +            var roundedDuration = Math.roundTo(segment.duration, secondsPerBlock);

Nit: Newline between these lines.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:123
> +        Object.keys(WebInspector.RenderingFrameTimelineRecord.TaskType).forEach(function(key) {

for (var key in WebInspector.RenderingFrameTimelineRecord.TaskType) {

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:140
> +            }
> +            if (currentSegment.duration >= secondsPerBlock)

Nit: Newline between these lines.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:159
> +        invisibleSegments.forEach(function(item) {

for (var item of invisibleSegments) {

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:238
> +        this._record.__displayData.segments.forEach(function(segment) {

for (var segment of this._record.__displayData.segments)

[Matt Baker]

Created attachment 256253 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=256253&action=review

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:192
> +            adjacentSegments.forEach(function(adjacentSegment) {

Not: Could have also been converted to for (..of..).

[Matt Baker]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=256253&action=review

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:192
>> +            adjacentSegments.forEach(function(adjacentSegment) {
> 
> Not: Could have also been converted to for (..of..).

What are our style guidelines with regard to Array.prototype.forEach? I see it being used with and without the optional thisArg.

[Timothy Hatcher]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=256253&action=review

>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:192
>>> +            adjacentSegments.forEach(function(adjacentSegment) {
>> 
>> Not: Could have also been converted to for (..of..).
> 
> What are our style guidelines with regard to Array.prototype.forEach? I see it being used with and without the optional thisArg.

We prefer not to use it now that we have for (..of..). That avoided a lot of function calls and closures.

[WebKit Commit Bot]

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

Clearing flags on attachment: 256253

Committed r186387: <http://trac.webkit.org/changeset/186387>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.