Timeline markers #275

gregtatum · 2017-04-18T21:39:16Z

This is a work in progress for timeline markers. I need to clean up a bunch of stuff. @mikeconley this is here if you want to check it out, but I'm only showing TracingMarkers, or markers that we can determine have a start and end time. Your AsyncTabSwitch aren't being turned into TracingMarkers right now.

codecov-io · 2017-04-27T15:23:23Z

Codecov Report

Merging #275 into master will increase coverage by 0.83%.
The diff coverage is 98.3%.

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   71.08%   71.91%   +0.83%     
==========================================
  Files          59       60       +1     
  Lines        3247     3351     +104     
==========================================
+ Hits         2308     2410     +102     
- Misses        939      941       +2

Impacted Files	Coverage Δ
src/content/profile-data.js	`62.79% <ø> (ø)`	⬆️
src/content/stack-timing.js	`96.47% <ø> (ø)`	⬆️
src/test/store/fixtures/profiles.js	`100% <100%> (ø)`	⬆️
src/test/unit/profile-data.js	`100% <100%> (ø)`	⬆️
src/content/reducers/timeline-view.js	`92.85% <100%> (+2.85%)`	⬆️
src/test/store/actions.js	`100% <100%> (ø)`	⬆️
src/content/actions/timeline.js	`71.42% <100%> (+11.42%)`	⬆️
src/content/reducers/profile-view.js	`62.89% <100%> (+0.33%)`	⬆️
src/content/marker-timing.js	`94.11% <94.11%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51af0de...e15876f. Read the comment docs.

gregtatum · 2017-04-27T15:27:38Z

I pulled out the canvas logic into its own separate component. Next up is re-use the TimelineCanvas component for the TImelineFlameChart.

gregtatum · 2017-05-04T21:27:21Z

This is now ready for review. This is a first pass and I have many follow-ups I would like to do. If you could be specific on review changes on what can be follow-ups I would appreciate it.

Follow-ups

Move labels to the left side area. This would change the markup for the area and some assumptions about the components. Although clutter-wise the current solution might make things cleaner.
Colors are preliminary.
Sort markers into a better order.
Group makers by type.
Marker thread sorting and hiding.
UserTimings have labels, but others do not.
Do we care about the duplicated strings, or should I move to unique strings?
At this point I really only need to draw charts that are in view.
Build out marker payload definitions.

julienw · 2017-05-05T15:21:10Z

FlameChartCanvas.js seem to conflict a lot with current master.

gregtatum · 2017-05-05T15:23:46Z

I think it's my recent PureComponent changes.

julienw

I left several comments, some of them perf related, others style related , others as suggestions.

I admit I haven't looked so much at the canvas code, I'll trust you on this.

The feature is quite cool and works fine. I'd rather see it in rather than make it perfect. So if you prefer to move some of my comments to follow-ups it's fine by me.

julienw · 2017-05-05T17:04:18Z

src/content/components/FlameChartCanvas.js

+                           onDoubleClickItem={this.onDoubleClickStack}
+                           getHoveredItemInfo={this.getHoveredStackInfo}
+                           drawCanvas={this.drawCanvas}
+                           hitTest={this.hitTest} />;


nit: should all of the functions names be prefixed by an underscore ?

Yeah, that would be more consistent with the codebase. I'll update it. I'm not sure (from a bikeshedding point of view) of what utility underscore has for React classes' member functions, as the class instances are never used by anyone, so all class functions are implicitly private.

TBH I usually don't care, but it's just what I've been told in this project ;)

julienw · 2017-05-05T17:04:59Z

src/content/components/TimelineCanvas.css

+}
+
+.timelineCanvas.hover {
+  cursor: default;


nit: have you tried putting the "cursor: default" on the above block ?

The behavior here is to show the grab cursor on the Timeline when not hovering over a stack frame, then when you hover over a stack frame it changes to the default cursor.

julienw · 2017-05-05T17:06:38Z

src/content/components/TimelineCanvas.js

+    const { className, drawCanvas } = this.props;
+    if (!this._requestedAnimationFrame) {
+      this._requestedAnimationFrame = true;
+      window.requestAnimationFrame(() => {


optional nit: you can assign to _requestedAnimationFrame the result of window.requestAnimationFrame.

julienw · 2017-05-05T17:07:26Z

src/content/components/TimelineCanvas.js

+  _ctx: CanvasRenderingContext2D
+  state: {
+    hoveredItem: null | HoveredItem;
+  }


nit: please put semicolons at the end of these lines

julienw · 2017-05-05T17:07:55Z

src/content/components/FlameChartCanvas.js

-  _devicePixelRatio: number
-  _textMeasurement: null|TextMeasurement
-  _ctx: null|CanvasRenderingContext2D
+  _textMeasurement: null | TextMeasurement

  props: Props


nit: can you put a semicolon at the end of these lines ?

Yeah, we should have a linting rule in place that covers this. I looked for one briefly but didn't find one, although maybe we can just switch to prettier.

julienw · 2017-05-05T17:47:34Z

src/content/containers/TimelineView.js

          <Reorderable tagName='div'
-                       className={`${className}ThreadList`}
+                       className={'timelineViewTimelinesThreadList'}


nit: you don't need the {} syntax, just put the class as a string :)

julienw · 2017-05-05T17:50:35Z

src/content/marker-timing.js

+    markerTimingRows = markerTimingRows.concat(value);
+  }
+  return markerTimingRows;
+}


Optional, but do you think you can split this function is several parts ? For example, the Map handling could be extracted into a higher-level component ?

julienw · 2017-05-05T17:51:47Z

src/content/marker-timing.js

+    // Handle different marker payloads.
+    switch (data.type) {
+      case 'UserTiming':
+        return (data: UserTimingMarkerPayload).name;


Optional nit: not sure the flow annotation is useful here.

I mainly did it as documentation, would it be better to leave this out?

julienw · 2017-05-05T18:01:55Z

src/content/marker-timing.js

+  let markerTimingRows = [];
+  for (const [, value] of markerTimingsMap) {
+    markerTimingRows = markerTimingRows.concat(value);
+  }


I think this just work to flatten: [].concat(...marketTimingRows.values());
without intermediary arrays :)

julienw · 2017-05-05T18:03:45Z

src/content/url-handling.js

+    query.react_perf = null;
+    /* eslint-enable camelcase */
+  }
+


pretty sure this landed elsewhere :)

mstange · 2017-05-05T19:24:20Z

src/content/containers/ProfileViewerHeader.js

@@ -70,7 +70,7 @@ class ProfileViewerHeader extends PureComponent {
        }
      </div>
      <div className={`${className}HeaderIntervalMarkerOverviewContainer ${className}HeaderIntervalMarkerOverviewContainerGfx`}>
-        {
+        {/*


I think we should keep the tracing markers at the top until this tab is more polished.

Woops, I knew I forgot something. Thanks I meant to bring this up. Will do.

mstange · 2017-05-05T19:24:58Z

src/content/containers/TimelineMarkers.css

+  user-select: none;
+}
+
+.timelineMarkersLabels > span {


Please set a class name on the span instead.

mstange · 2017-05-05T19:28:27Z

src/content/components/TimelineCanvas.js

+
+  _prepCanvas() {
+    const {canvas} = this.refs;
+    const {containerWidth, containerHeight} = this.props;


I think we need to pick one style here and add a lint for it. At the moment we have a wild mix of hugging braces and spaced braces.

Sounds good, I'm ready to take on prettier at this point.

gregtatum force-pushed the timeline-markers branch from fbd1ad0 to d89d468 Compare April 27, 2017 21:37

gregtatum mentioned this pull request May 2, 2017

Add a User Timing from window.performance.mark and window.performance.measure #48

Closed

gregtatum force-pushed the timeline-markers branch 2 times, most recently from 4fe9c72 to 7487dc5 Compare May 4, 2017 21:13

gregtatum changed the title ~~[WIP] Timeline markers~~ Timeline markers May 4, 2017

gregtatum requested review from mstange and julienw May 4, 2017 21:28

julienw approved these changes May 5, 2017

View reviewed changes

mstange approved these changes May 5, 2017

View reviewed changes

gregtatum force-pushed the timeline-markers branch from 7487dc5 to e0329ea Compare May 8, 2017 18:51

gregtatum added 2 commits May 8, 2017 14:03

Add a basic marker view implementation

1c8d0e5

Pull out timeline canvas logic into separate component

0974e45

gregtatum force-pushed the timeline-markers branch from e0329ea to 7cba79f Compare May 8, 2017 19:10

gregtatum added 5 commits May 8, 2017 14:16

Share TimelineCanvas with FlameChartCanvas

17ed0bb

Label user timings

be19a43

Add timeline marker tests

716ec49

Avoid label statements

ec7e264

Address review for Timeline Markers

e15876f

gregtatum force-pushed the timeline-markers branch from 7cba79f to e15876f Compare May 8, 2017 19:17

gregtatum merged commit 004705f into firefox-devtools:master May 8, 2017

gregtatum mentioned this pull request May 18, 2017

Timeline markers #114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeline markers #275

Timeline markers #275

gregtatum commented Apr 18, 2017

codecov-io commented Apr 27, 2017 •

edited

gregtatum commented Apr 27, 2017 •

edited

gregtatum commented May 4, 2017

julienw commented May 5, 2017

gregtatum commented May 5, 2017

julienw left a comment

julienw May 5, 2017

gregtatum May 8, 2017

julienw May 8, 2017

julienw May 5, 2017

gregtatum May 8, 2017

julienw May 5, 2017

julienw May 5, 2017

julienw May 5, 2017

gregtatum May 8, 2017

julienw May 5, 2017

julienw May 5, 2017

julienw May 5, 2017

gregtatum May 5, 2017

julienw May 5, 2017

julienw May 5, 2017

gregtatum May 5, 2017

mstange May 5, 2017

gregtatum May 5, 2017

mstange May 5, 2017

mstange May 5, 2017

gregtatum May 8, 2017

Timeline markers #275

Timeline markers #275

Conversation

gregtatum commented Apr 18, 2017

codecov-io commented Apr 27, 2017 • edited

Codecov Report

gregtatum commented Apr 27, 2017 • edited

gregtatum commented May 4, 2017

Follow-ups

julienw commented May 5, 2017

gregtatum commented May 5, 2017

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 27, 2017 •

edited

gregtatum commented Apr 27, 2017 •

edited