-
Notifications
You must be signed in to change notification settings - Fork 19
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
Update version-script #87
Conversation
* Change default visibilty to local Signed-off-by: MuHong Byun <mh.byun@samsung.com>
It's tested on webview example at FHUB |
I was talking about the embedder binary ( |
May I ask any reason for this change? |
Basically, main purpose of this is to prevent unnecessary symbols to show outside.(like FT?) |
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.
Actually I'm against this change because it makes another non-trivial diff between the upstream engine and our engine, and can potentially cause symbol resolution failures.
FlutterDesktop*; | ||
FlutterEngine*; | ||
FlutterPlatformMessage*; | ||
*Dart*; |
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.
How did you get this list?
Also you need to update my comment in line 5 to reflect this change.
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.
How did you get this list?
I added it after seeing the error message during build & running.
Hmm... I'm hard to understand .. I don’t think it’s an important issue that needs discussion. |
Related PR :#85
Minor changes to achieve the below: