Reland "Remove references to Observatory (#38919)" #39139
Conversation
This reverts commit 5dd9454.
|
I reverted the changes to the public APIs and marked them as deprecated. I think I caught them all, but I'm not sure where the public API boundaries are, so please let me know if I missed any. |
|
What was breaking in the Flutter -> plugins roll? #39035 |
| */ | ||
| @Deprecated | ||
| @Nullable | ||
| public static String getObservatoryUri() { |
There was a problem hiding this comment.
@jmagman renaming this method had broken the espresso plugin.
There was a problem hiding this comment.
|
|
||
| if (err != 0) { | ||
| FML_LOG(ERROR) << "Failed to register observatory port with mDNS with error " << err << "."; | ||
| FML_LOG(ERROR) << "Failed to register Dart VM Service port with mDNS with error " << err << "."; |
There was a problem hiding this comment.
Networking in iOS is a protected resource and therefore requires permissions, specifically NSBonjourServices. Without it, this will always error and show this error message to users.
In the Flutter tools, we add in NSBonjourServices to the Info.plist on build here:
https://github.com/flutter/flutter/blob/c31856bc48596d251ade554c33a09184e69185d9/packages/flutter_tools/bin/xcode_backend.dart#L247-L285
To avoid errors, the tool should be updated first to include the new _dartVmService._tcp (and also keep the original _dartobservatory._tcp).
There was a problem hiding this comment.
This code is falling back to _dartobservatory._tcp with the legacyRegistrationType check below so I think it should still work. However, it shouldn't log any errors like this of or the iOS 14 one until both fail.
@vashworth's suggestion to add both to the tool first would also work.
There was a problem hiding this comment.
Updated to only output the error message after both registration attempts have failed.
jmagman
left a comment
There was a problem hiding this comment.
Requesting changes to remove the error logs until both the _dartVmService._tcp AND _dartobservatory._tcp checks fail.
jmagman
left a comment
There was a problem hiding this comment.
LGTM
@vashworth were your concerns addressed?
| */ | ||
| @Deprecated | ||
| @Nullable | ||
| public static String getObservatoryUri() { |
There was a problem hiding this comment.
Yup, LGTM too! |
This reverts commit 5dd9454.