Make InspectorService supporting issuing most of its requests over the Flutter Daemon instead of observatory. #1980
Conversation
flutter daemon instead of using the observatory protocol for requests that do not need to use the observatory. This avoids problematic cases where the observatory protocol allowed requests to issue at surprising times such as right in the middle of a flutter method call. It is expected this fixes many hard or impossible to reproduce inspector bugs. To simplify the code I have also changed the semantics so that the supportedServiceMethods set is computed before an InspectorService is considered ready. This reduces the number of future.thenCompose calls within the InspectorService code dealing with functionality only exposed by the latest version.
|
Good news is the service level logic was well isolated to InspectorService so this change doesn't impact the UI code in InspectorPanel. I've verified the plugin works both with and without the new daemon service. Generally performance is similar with both transport protocols but robustness appears to be improved with the daemon protocol. |
jacob314
changed the title
src/io/flutter/view/FlutterView.java
Outdated
| whenCompleteUiThread(InspectorService.create(app.getFlutterDebugProcess(), app.getVmService()), | ||
| (InspectorService inspectorService, Throwable throwable) -> { | ||
| if (throwable != null) { | ||
| LOG.error(throwable); |
There was a problem hiding this comment.
I think we want warn here, so we don't show a dialog to the user.
| @@ -82,6 +82,7 @@ | |||
| private final InspectorService.FlutterTreeType treeType; | |||
| @NotNull | |||
| private final FlutterApp flutterApp; | |||
| private final InspectorService inspectorService; | |||
There was a problem hiding this comment.
@NotNull, to match the constructor param.
| @@ -192,7 +195,7 @@ public void onAppChanged() { | |||
|
|
|||
| @Nullable | |||
There was a problem hiding this comment.
This should now be @NotNull I believe.
| @NotNull private final Set<String> supportedServiceMethods; | ||
|
|
||
| // TODO(jacobr): remove this field as soon as | ||
| // `ext.flutter.debugCallWidgetInspectorService` has been in two revs of the |
There was a problem hiding this comment.
This is a flutter framework service extension?
There was a problem hiding this comment.
discussed offline. Switched to
ext.flutter.inspector.methodName
and cleaned up how we were passing parameters.
Parameters are now passed as either
{'arg': arg, 'objectGroup': objectGroup}
{'objectGroup': objectGroup}
or
{'arg0': arg0, 'arg1': arg1, 'arg2': arg2, ...}
for the one vararg like case.
| if (arg == null || arg.getId() == null) { | ||
| return getInspectorLibrary().eval("WidgetInspectorService.instance." + methodName + "(null, \"" + groupName + "\")", null); | ||
| } | ||
| return getInspectorLibrary() | ||
| .eval("WidgetInspectorService.instance." + methodName + "(\"" + arg.getId() + "\", \"" + groupName + "\")", null); | ||
| } | ||
|
|
||
| CompletableFuture<InstanceRef> invokeServiceMethodOnRef(String methodName, InstanceRef arg) { | ||
| /** | ||
| * This API requires using the observatory service a the input parameter is an |
There was a problem hiding this comment.
rewrote this comment.
|
Jacob, does this address the two crashers on https://github.com/flutter/flutter-intellij/milestone/34? |
|
I've updated the two bugs. The first should be fixed and the second should be mitigated to the point where it can be pushed to M24. |
This avoids problematic cases where the observatory protocol allowed
requests to issue at surprising times such as right in the middle of
a flutter method call.
It is expected this fixes many hard or impossible to reproduce inspector
bugs.
To simplify the code I have also changed the semantics so that
the supportedServiceMethods set is computed before an InspectorService
is considered ready. This reduces the number of future.thenCompose
calls within the InspectorService code dealing with functionality only
exposed by the latest version.