-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[MacOS] Add support for creating and presenting platform views. #22905
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
99a48ad
to
9ee9d45
Compare
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.
We are also missing tests in this PR.
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterPlatformViews.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
6e91de1
to
b1c6f57
Compare
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 % nits, assuming @iskakaushik LGTMed as well.
Thanks for adding tests!
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterPlatformViews.h
Outdated
Show resolved
Hide resolved
444f71e
to
6874a3e
Compare
Please don't merge this until i've taken a look :-) I think the public API here needs a little more scrutiny. |
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
87de964
to
8fbb288
Compare
shell/platform/darwin/macos/framework/Source/FlutterPlatformViewController.mm
Outdated
Show resolved
Hide resolved
Thanks for the review! I'll update this on Friday. |
Add platform view support in FlutterGLCompositor and FlutterViewController
Renaming variables, created PresentPlatformView method and updated documentation
@stuartmorgan @iskakaushik
First ran into after rebasing this branch ontop of master but it seems like I'm running into on master as well with a local engine. Tried following the suggestions in this issue flutter/flutter#72608, ie upgrading and doing a new project but none seem to work with a local engine.
Anyone have any ideas whats wrong here? |
I wouldn't have expected a local engine to have any effect on App.framework copying. I'll see if I can reproduce locally and follow up in flutter/flutter#72608 |
612f034
to
b8f5837
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I'm getting the same failures as https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8857625998621949056/+/steps/build_host_debug_unopt/0/stdout locally. Can you fix those up and see if you are still reproducing flutter/flutter#72608? |
Never mind, I was able to solve the problem by recloning the repos and setting up the environment again. |
Hey @RichardJCai , thanks for continuing to work and make progress on this PR. Given the inactivity over the last week, I'm closing this. Feel free to open a new PR once you get a chance to address the failing pre-submit checks and tag me. Thanks! |
Sorry tried to address the PR comments but I still can't seem to get my engine environment set up for some reason. |
@RichardJCai did you have a chance to fix your environment? I know you probably already checked this but I once got this exact same error message when the underlying issue was in fact a build error in my dart code. Can you maybe post a verbose output of your build with the local engine? I'd be happy to help you debug your environment as good as I can. Here on or on Discord (cben#2483). |
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable.
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable: flutter/flutter#96668
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable: flutter/flutter#96668 * Example is at: https://github.com/iskakaushik/flutter_macos_platform_view_example
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable: flutter/flutter#96668 * Example is at: https://github.com/iskakaushik/flutter_macos_platform_view_example
Add support for:
Will add follow up PR for static thread merging, there may be visual bugs since raster/ui threads are not merged.