Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port CPU bottom up table to Flutter. #1659

Merged
merged 4 commits into from
Feb 25, 2020

Conversation

kenzieschmoll
Copy link
Member

Screen Shot 2020-02-24 at 1 35 20 PM

This change applies to both the Timeline page (#1289) and the Performance page (#1287)

);
}

void _performOnDataRoots(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice approach to this problem.

@@ -161,6 +161,20 @@ void main() {
expect(find.byKey(const Key('empty')), findsOneWidget);
});

testWidgets('displays with multiple data roots',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a test for interactions on the roots: ensure that a few of the roots are properly expandable without breaking the state of the rest of the tree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, include tests for the cases where the roots of the tree are changed. We don't want to get a red screen of death when we request new data and we get a broken tree because there were fewer roots being used by the same tree representation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. ptal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!
dicaprio

This was referenced Feb 24, 2020
Copy link
Contributor

@DaveShuckerow DaveShuckerow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

# Conflicts:
#	packages/devtools_app/lib/src/profiler/flutter/cpu_profiler.dart
#	packages/devtools_app/test/flutter/cpu_profiler_test.dart
@kenzieschmoll kenzieschmoll merged commit 9707653 into flutter:master Feb 25, 2020
@kenzieschmoll kenzieschmoll deleted the bottomUp branch February 25, 2020 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants