RESOLVED FIXED 160147
Web Inspector: Inactive/active network bar segments overlap when latency is zero
https://bugs.webkit.org/show_bug.cgi?id=160147
Summary Web Inspector: Inactive/active network bar segments overlap when latency is zero
Matt Baker
Reported 2016-07-24 12:10:54 PDT
Created attachment 284451 [details] [Image] Overlapping segments The `has-inactive-segment` class should be removed from network bars with zero latency.
Attachments
[Image] Overlapping segments (114.33 KB, image/png)
2016-07-24 12:10 PDT, Matt Baker
no flags
[Patch] Proposed Fix (3.82 KB, patch)
2016-07-27 16:15 PDT, Matt Baker
no flags
[Patch] For Landing (4.26 KB, patch)
2016-07-29 13:15 PDT, Matt Baker
no flags
Radar WebKit Bug Importer
Comment 1 2016-07-24 12:11:08 PDT
Matt Baker
Comment 2 2016-07-27 16:15:11 PDT
Created attachment 284742 [details] [Patch] Proposed Fix
Joseph Pecoraro
Comment 3 2016-07-28 11:21:54 PDT
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 4 2016-07-28 13:16:23 PDT
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 5 2016-07-29 13:02:40 PDT
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
Comment 6 2016-07-29 13:15:06 PDT
Created attachment 284893 [details] [Patch] For Landing
WebKit Commit Bot
Comment 7 2016-07-29 13:44:57 PDT
Comment on attachment 284893 [details] [Patch] For Landing Clearing flags on attachment: 284893 Committed r203920: <http://trac.webkit.org/changeset/203920>
WebKit Commit Bot
Comment 8 2016-07-29 13:45:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.