Timeline view refactoring #20
Conversation
lib/timeline/timeline.dart
Outdated
| @override | ||
| String toString() => '[$category] [$phase] $name'; | ||
| HelpInfo get helpInfo => | ||
| new HelpInfo(title: 'timeline docs', url: 'http://www.cheese.com'); |
There was a problem hiding this comment.
There should probably be a TODO to update this 😄
lib/timeline/timeline.dart
Outdated
|
|
||
| TimelineFrame(); | ||
| while (dartEvents.length > 1 && |
There was a problem hiding this comment.
Should this be >= 1? Otherwise a single item that doesn't meet the criteria might be left?
There was a problem hiding this comment.
At this point, we know that there are no gpu samples that start before the dart ones. What we're doing here is making sure there's just one dart sample that starts before the gpu ones, and that it's the latest dart sample.
lib/timeline/timeline.dart
Outdated
| } | ||
| frames.add(frame); | ||
| if (frames.length > maxFrames) { | ||
| frames.removeAt(0); |
There was a problem hiding this comment.
If this code is called a lot, we may have the same perf concerns as logging - removing the first item repeatedly might be expensive (I'm assuming, I haven't checked the implementation). We might want to do the same (remove in batches), or it may be worth us making a custom list that's just a fixed array that overwrites the oldest items (so there are no resizes) and just offsets indexing operations/enumeration.
There was a problem hiding this comment.
I think this'll be OK as frames will likely just come in every 1/60 of a second. I made a note about the UI respecting the maxFrames list - which it doesn't currently. We will want to profile this for a 60 fps animating app, to make sure that the UI (and dom manipulations we do) can keep up with that rate of frames.
| } | ||
|
|
||
| switch (event.phase) { | ||
| case 'B': |
There was a problem hiding this comment.
For my curiosity - are these documented anywhere?
There was a problem hiding this comment.
Yes - in the google doc listed at the top of the file (https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.yr4qxyxotyw).
lib/timeline/timeline_protocol.dart
Outdated
|
|
||
| int get category { | ||
| if (name.endsWith('.ui')) { | ||
| return 1; |
There was a problem hiding this comment.
Looks strange that category is ints - would an enum work? Or if used only for sorting, maybe naming it sortCategory or sortPriority or something might be clearer?
There was a problem hiding this comment.
Dart enums are a bit underwhelming - agree that something a bit more semantic here would be better. I'll rename to sortPriority.
Add feature to pause auto play when user input is detected
PipelineProduce; for the GPU portion, look forMessageLoop::RunExpiredTasksThis does a better job of determining where frames are in the event stream, and displays more info in the timeline view.
@DanTup