Web Inspector: Inactive/active network bar segments overlap when latency is zero

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Created attachment 284451 [details]
[Image] Overlapping segments

The `has-inactive-segment` class should be removed from network bars with zero latency.

[Radar WebKit Bug Importer]

<rdar://problem/27516007>

[Matt Baker]

Created attachment 284742 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:307
> +            let minimumSegmentDuration = graphDataSource.secondsPerPixel * WebInspector.TimelineRecordBar.MinimumWidthPixels;

Nit: This block could use a comment like the ChangeLog.

Maybe we should assert all the graphDataSource methods somewhere in here. I worry there may be some data source that is missing secondsPerPixel. A brief look around and all data sources now seem fine, but a new data source in the future might forget it.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:308
> +            if (barDuration < minimumSegmentDuration || barActiveStartTime - barStartTime < minimumSegmentDuration) {

Is the second expression a stricter check then the first? Can we just do that check or is there some way the first is still needed?

[Matt Baker]

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

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:307
>> +            let minimumSegmentDuration = graphDataSource.secondsPerPixel * WebInspector.TimelineRecordBar.MinimumWidthPixels;
> 
> Nit: This block could use a comment like the ChangeLog.
> 
> Maybe we should assert all the graphDataSource methods somewhere in here. I worry there may be some data source that is missing secondsPerPixel. A brief look around and all data sources now seem fine, but a new data source in the future might forget it.

Agreed, it could be overlooked in the future.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:308
>> +            if (barDuration < minimumSegmentDuration || barActiveStartTime - barStartTime < minimumSegmentDuration) {
> 
> Is the second expression a stricter check then the first? Can we just do that check or is there some way the first is still needed?

The second check catches cases where the bar has a small (or zero) latency that would artificially inflate the bar width by 1-3px.

[Matt Baker]

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

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

>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:308
>>> +            if (barDuration < minimumSegmentDuration || barActiveStartTime - barStartTime < minimumSegmentDuration) {
>> 
>> Is the second expression a stricter check then the first? Can we just do that check or is there some way the first is still needed?
> 
> The second check catches cases where the bar has a small (or zero) latency that would artificially inflate the bar width by 1-3px.

Correction, the first check is not needed. Will remove.

[Matt Baker]

Created attachment 284893 [details]
[Patch] For Landing

[WebKit Commit Bot]

Comment on attachment 284893 [details]
[Patch] For Landing

Clearing flags on attachment: 284893

Committed r203920: <http://trac.webkit.org/changeset/203920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.