Make Route dispatching memory events. [prod-leak-fix]#133721
Make Route dispatching memory events. [prod-leak-fix]#133721polina-c merged 16 commits intoflutter:masterfrom
Conversation
| if (kFlutterMemoryAllocationsEnabled) { | ||
| MemoryAllocations.instance.dispatchObjectCreated( | ||
| library: 'package:flutter/widgets.dart', | ||
| className: '$Route<$T>', |
There was a problem hiding this comment.
I didn't notice this earlier, why would you need to provide a className if you already provide the object itself? Should the ObjectCreated uses the object.runtimeType if it needs the class name?
There was a problem hiding this comment.
runtimeType is different from type that owns instrumentation, because runtime type can be descender (like MaterialPageRoute). Does it help?
There was a problem hiding this comment.
Nit: why is Route<$T> a clearer name than MaterialPageRoute? I would have thought the more specific class name would be clearer for someone debugging.
| /// used instead. | ||
| Route({ RouteSettings? settings }) : _settings = settings ?? const RouteSettings(); | ||
| Route({ RouteSettings? settings }) : _settings = settings ?? const RouteSettings() { | ||
| if (kFlutterMemoryAllocationsEnabled) { |
There was a problem hiding this comment.
Sorry I have not brought up this earlier. Should these logic be wrapped in an assert?
There was a problem hiding this comment.
It is proven that this code is tree-shaked when kFlutterMemoryAllocationsEnabled is false, because it is a compile time constant.
So, it is ok to go without assert.
Does this answer your question?
There was a problem hiding this comment.
yes, thanks for explanation
| const TestPage page = TestPage(name: 'page'); | ||
| final Route<dynamic> route = page.createRoute(nav.currentContext!); | ||
| // ignore: invalid_use_of_protected_member | ||
| route.dispose(); |
There was a problem hiding this comment.
you can do
nav.currentState!.push(MaterialPageRoute(builder: (_) => const PlaceHolder())); // This should create a route
await tester.pumpAndSettle();
nav.currentState!.pop();
await tester.pumpAndSettle(); // this should dispose the route.
There was a problem hiding this comment.
nav.currentState is null on first line
Did i miss something?
There was a problem hiding this comment.
you will need this first before push
await tester.pumpWidget(
MaterialApp(
navigatorKey: nav,
home: const Scaffold(
body: Text('home'),
)
)
);There was a problem hiding this comment.
Applied. Thank you!
| MemoryAllocations.instance.addListener(listener); | ||
|
|
||
| await createAndDisposeRoute(); | ||
| expect(events, hasLength(2)); |
There was a problem hiding this comment.
This is interesting, I would expect 3 events. One for MaterialPageRoute created from home page of MaterialApp. Two for creating and disposing the TestPage
There was a problem hiding this comment.
ah nvm, the home page will create MaterialPageRoute which will be filter out in the listener
| ) | ||
| ); | ||
|
|
||
| nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route |
There was a problem hiding this comment.
| nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route | |
| nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route. |
| await tester.pumpAndSettle(); | ||
|
|
||
| nav.currentState!.pop(); | ||
| await tester.pumpAndSettle(); // this should dispose the route. |
There was a problem hiding this comment.
| await tester.pumpAndSettle(); // this should dispose the route. | |
| await tester.pumpAndSettle(); // This should dispose the route. |
No description provided.