-
Notifications
You must be signed in to change notification settings - Fork 6k
Fixes FlutterEngine internal retain cycle #18595
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the aforementioned performance changes.
}]; | ||
|
||
auto platformViewsController = _platformViewsController.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should still use the weakptr trick you were using previously. The reason is that platformViewsController is a C++ object which means it won't get retained when it is captured by the block. In practice this looks like it would be safe today but I think we should err on the side of caution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaaclarke Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the quick responses =)
9be9c9e
to
b511285
Compare
Landing on red, the CI failure was a flake. It passed when rerun here: https://ci.chromium.org/raw/build/logs.chromium.org/flutter/led/aaclarke_google.com/a96bf18d97c25854a608cf5e25f0bd8ce76c3a567885d6fcdce4aef30bb56b4b/+/annotations?server=chromium-swarm.appspot.com |
No description provided.