Fix durations in the network panel #1752
Conversation
| const startPosition = _timeToCssPixels(props, networkPayload.startTime); | ||
| const endPosition = _timeToCssPixels(props, networkPayload.endTime); | ||
| const startPosition = _timeToCssPixels(props, marker.start); | ||
| const endPosition = _timeToCssPixels(props, marker.start + marker.dur); |
There was a problem hiding this comment.
I think that all our marker components deal with marker.start/dur instead of looking at the payload, at least for the general things.
Codecov Report
@@ Coverage Diff @@
## master #1752 +/- ##
==========================================
+ Coverage 83.26% 83.34% +0.07%
==========================================
Files 178 178
Lines 12103 12108 +5
Branches 2971 2974 +3
==========================================
+ Hits 10078 10091 +13
+ Misses 1846 1839 -7
+ Partials 179 178 -1
Continue to review full report at Codecov.
|
| status: 'STATUS_STOP', | ||
| startTime: startPayload.endTime, | ||
| endTime: startPayload.endTime + 0.5, | ||
| }; |
There was a problem hiding this comment.
Now this function generated a pair of start/stop markers.
Ideally the stop payload should also hold all the possible properties, but I defer this to a future PR.
| ]); | ||
| profile.threads.push(markersThread); | ||
| const { getState, dispatch } = storeWithProfile(profile); | ||
| samplesThread.name = 'Thread with samples'; | ||
| markersThread.name = 'Thread with markers'; | ||
| samplesThread.markers = markersThread.markers; |
There was a problem hiding this comment.
as discussed on Slack, the "markers" raw table holds references to the string table, so this was incorrect.
Then I changed the marker-related selectors below to refer to the markersThread instead of the samplesThread.
| data: { | ||
| ...endData, | ||
| startTime: startData.startTime, | ||
| }, |
There was a problem hiding this comment.
This isn't the end of the road, because we'll want to add a property to represent the startMarker's endTime. I think handling this would fix #1349 too.
but at least we now display the right durations.
|
Hey @gregtatum, I'm flagging you on this one because you looked at it last year. I especially wanted to get the durations right because I use them in tests in #1746 (which will incidentally provide some more coverage on the merging mechanism). I plan to continue fixing a few small other issues I found while working on that PR. |
src/test/store/app.test.js
Outdated
| @@ -78,7 +78,7 @@ describe('app actions', function() { | |||
| }); | |||
|
|
|||
| it('shows the network chart when network markers are present in the thread', function() { | |||
| const profile = getProfileWithMarkers([getNetworkMarker(10, 0)]); | |||
| const profile = getProfileWithMarkers([...getNetworkMarkers(10, 0)]); | |||
There was a problem hiding this comment.
ah yeah, changed this :)
julienw
force-pushed
the
fix-durations-network
branch
from
ec66eaf to
2210de8
Compare
Deploy preview
(compare with the version in production).