[framework] inline AbstractNode into RenderObject#103832
[framework] inline AbstractNode into RenderObject#103832fluttergithubbot merged 6 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
f: cupertino
flutter-dashboard
bot
added
the
a: tests
|
From tests on customer:money's app, I was not able to make any measurable performance improvements. That said, if this change isn't breaking to google3 we might consider it since I think removing casts is also beneficial from an ease-of-understanding perspective. |
| final AnimationController opacityAnimation = AnimationController(value: 1, vsync: FakeTickerProvider()); | ||
| final RenderSliverAnimatedOpacity opacity = RenderSliverAnimatedOpacity(opacity: opacityAnimation, sliver: sliver); | ||
| // ignore: invalid_use_of_protected_member | ||
| parent.adoptChild(opacity); |
There was a problem hiding this comment.
@dnfield this one is the problem. if you use the child setter its a type error
| assert(!debugNeedsLayout); | ||
| assert(() { | ||
| final RenderObject? parent = this.parent as RenderObject?; | ||
| final RenderObject? parent = this.parent; |
There was a problem hiding this comment.
this might in fact be unnecessary now, the getter should just get inlined right?
There was a problem hiding this comment.
This is used in the asset on L2154 that asserts that the parent getter is stable
| child.markNeedsLayout(); | ||
| root.layout(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
*respectful minute of silence*
|
(cc @goderbauer) |
| test('AnimatedOpacity sets paint matrix to zero when alpha == 0 (sliver)', () { | ||
| final RenderSliver sliver = RenderSliverToBoxAdapter(child: RenderConstrainedBox(additionalConstraints: const BoxConstraints.tightFor(width: 20))); | ||
| final RenderBox parent = RenderConstrainedBox(additionalConstraints: const BoxConstraints.tightFor(width: 20)); | ||
| final RenderSliverPadding parent = RenderSliverPadding(padding: const EdgeInsets.all(4)); |
There was a problem hiding this comment.
This change is fine, the test only needed to be sure to have a child, doesn't really matter what the type is.
|
Marking as a draft until we figure out how to land this in a non-breaking manner |
|
Google changes were not as bad as expected due to this PR getting bundled with some other breaking PRs. Will apply g3fix and then attempt to land |
|
I ran a globla presubmit and bundled the changes into a g3fix. It doesn't seem like that is getting applied to the PR so I've marked as passing so we can land it. |
This reverts commit 24bd28f.
5 participants
AbstractNode isn't a free abstraction, because we can't do template style specialization in Dart. Many places in the framework that would otherwise have a free access or null check have to do a cast due to how AbstractNode is implemented.
Why not change to
AbstractNode<T extends AbstractNode<T>>? In short, because it would still require casting in several places to prove that your instance ofTis actually anAbstractNode<T>I've also not found any code that attempts to abstract over an AbstractNode, usually code is written for one of RenderObject, Layer, or SemanticsNode and casts as necessary