-
Notifications
You must be signed in to change notification settings - Fork 438
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
Fixed use of FBApplicationLaunchMode for FBDevice #648
base: main
Are you sure you want to change the base?
Fixed use of FBApplicationLaunchMode for FBDevice #648
Conversation
Hi @AndreasReich! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
} else { | ||
return (FBFuture*)processIdQueryResult; | ||
} |
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.
The final branch herer looks a little suspicious. If the app is running then we fail, but in all other conditions we should probably attempt to launch the app
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.
You're probably right but maybe we should still not launch it if the future state is cancelled?
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 you might be right and we should just propagate the cancel. @lawrencelomax I'm guessing cancelling will be so rare that either is fine
@AndreasReich thanks for working on idb! You'll need to accept the Facebook CLA before we can ship this PR |
Contacted my legal department already on helping me to fill out the company version of the CLA check before the holidays, just contacted them again to get this sorted :) |
Previously, the app would be restarted for FBApplicationLaunchModeForegroundIfRunning and not fail if already running for FBApplicationLaunchModeFailIfRunning.
11851a8
to
aadaad9
Compare
@c-ryan747 it's been quite a while, but the CLA should be sorted out by now |
Motivation
Previously, an Application on a device would be restarted for FBApplicationLaunchModeForegroundIfRunning and not fail if already running for FBApplicationLaunchModeFailIfRunning.
Test Plan
Tested this manually with an application that calls directly into FBDeviceControl framework.
This application in turn has some automated tests that capture this behavior (this is how I found the issue).
I have not added any tests to idb, but will happily do so if someone can point me to it (I'm still new to the project).
Related PRs
n/a