From c30a7f6f681f9f682eb0a2f2e7b637aff6e23093 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Tue, 25 Jul 2023 15:27:02 -0700 Subject: [PATCH 1/4] Load extensions from DevTools server instead of using stubs. --- .../extensions/embedded/_controller_web.dart | 9 ++++- .../lib/src/extensions/extension_service.dart | 38 ++++++++++++++----- .../lib/src/service/service_manager.dart | 19 +++++++++- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/packages/devtools_app/lib/src/extensions/embedded/_controller_web.dart b/packages/devtools_app/lib/src/extensions/embedded/_controller_web.dart index e7b7baa3a9e..18f941e8f4a 100644 --- a/packages/devtools_app/lib/src/extensions/embedded/_controller_web.dart +++ b/packages/devtools_app/lib/src/extensions/embedded/_controller_web.dart @@ -8,6 +8,7 @@ import 'dart:html' as html; import 'dart:ui' as ui; import 'package:devtools_extensions/api.dart'; +import 'package:path/path.dart' as path; import 'controller.dart'; @@ -32,8 +33,12 @@ class EmbeddedExtensionControllerImpl extends EmbeddedExtensionController { late final viewId = 'ext-${extensionConfig.name}-${_viewIdIncrementer++}'; String get extensionUrl { - // TODO(kenz): load the extension url being served by devtools server. - return 'https://flutter.dev/'; + return path.join( + html.window.location.origin, + 'devtools_extensions', + extensionConfig.name, + 'index.html', + ); } html.IFrameElement get extensionIFrame => _extensionIFrame; diff --git a/packages/devtools_app/lib/src/extensions/extension_service.dart b/packages/devtools_app/lib/src/extensions/extension_service.dart index 26db4825ced..220ea6c6669 100644 --- a/packages/devtools_app/lib/src/extensions/extension_service.dart +++ b/packages/devtools_app/lib/src/extensions/extension_service.dart @@ -4,6 +4,7 @@ import 'package:flutter/foundation.dart'; +import '../shared/config_specific/server/server.dart' as server; import '../shared/globals.dart'; import '../shared/primitives/auto_dispose.dart'; import 'extension_model.dart'; @@ -14,17 +15,36 @@ class ExtensionService extends DisposableController _availableExtensions; final _availableExtensions = ValueNotifier>([]); - void initialize() { - addAutoDisposeListener(serviceManager.connectedState, () { - _refreshAvailableExtensions(); + Future initialize() async { + await maybeRefreshExtensions(); + addAutoDisposeListener(serviceManager.connectedState, () async { + await maybeRefreshExtensions(); }); + + // TODO(kenz): we should also refresh the available extensions on some event + // from the analysis server that is watching the + // .dart_tool/package_config.json file for changes. + } + + Future maybeRefreshExtensions() async { + final appRootPath = await _connectedAppRootPath(); + if (appRootPath != null) { + await _refreshAvailableExtensions(appRootPath); + } } - // TODO(kenz): actually look up the available extensions from devtools server, - // based on the root path(s) from the available isolate(s). - int _count = 0; - void _refreshAvailableExtensions() { - _availableExtensions.value = - debugExtensions.sublist(0, _count++ % (debugExtensions.length + 1)); + Future _refreshAvailableExtensions(String? rootPath) async { + final extensions = await server.refreshAvailableExtensions(rootPath); + _availableExtensions.value = extensions; + } +} + +Future _connectedAppRootPath() async { + var fileUri = await serviceManager.rootLibraryForSelectedIsolate(); + if (fileUri == null) return null; + + if (fileUri.endsWith('/lib/main.dart')) { + fileUri = fileUri.replaceFirst('/lib/main.dart', ''); } + return fileUri; } diff --git a/packages/devtools_app/lib/src/service/service_manager.dart b/packages/devtools_app/lib/src/service/service_manager.dart index 5cfaddb2153..710c5ea4dd0 100644 --- a/packages/devtools_app/lib/src/service/service_manager.dart +++ b/packages/devtools_app/lib/src/service/service_manager.dart @@ -323,7 +323,7 @@ class ServiceConnectionManager { } if (FeatureFlags.devToolsExtensions) { - extensionService.initialize(); + await extensionService.initialize(); } _connectedState.value = const ConnectedState(true); @@ -539,6 +539,23 @@ class ServiceConnectionManager { await whenValueNonNull(isolateManager.mainIsolate); return libraryUriAvailableNow(uri); } + + Future rootLibraryForSelectedIsolate() async { + if (!connectedState.value.connected) return null; + + final selectedIsolateRef = isolateManager.mainIsolate.value?.id; + if (selectedIsolateRef == null) return null; + + final selectedIsolate = await service!.getIsolate(selectedIsolateRef); + final rootLib = selectedIsolate.rootLib?.uri; + if (rootLib == null) return null; + + await resolvedUriManager.fetchFileUris(selectedIsolateRef, [rootLib]); + return resolvedUriManager.lookupFileUri( + selectedIsolateRef, + rootLib, + ); + } } class VmServiceCapabilities { From f9a46b1c24af73240d635b1bfc66282f2d1ab516 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 26 Jul 2023 09:59:33 -0700 Subject: [PATCH 2/4] extension model and test --- .../lib/src/extensions/extension_model.dart | 44 ++++--- .../test/extensions/extension_model_test.dart | 107 ++++++++++++++++++ 2 files changed, 127 insertions(+), 24 deletions(-) create mode 100644 packages/devtools_shared/test/extensions/extension_model_test.dart diff --git a/packages/devtools_shared/lib/src/extensions/extension_model.dart b/packages/devtools_shared/lib/src/extensions/extension_model.dart index b8e5f11cc87..e1ffdabe4ad 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_model.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_model.dart @@ -25,31 +25,27 @@ class DevToolsExtensionConfig { codePoint = codePointFromJson as int? ?? defaultCodePoint; } - final name = json[nameKey] as String?; - final path = json[pathKey] as String?; - final issueTrackerLink = json[issueTrackerKey] as String?; - final version = json[versionKey] as String?; - - final nullFields = [ - if (name == null) nameKey, - if (path == null) pathKey, - if (issueTrackerLink == null) issueTrackerKey, - if (version == null) versionKey, - ]; - if (nullFields.isNotEmpty) { + if (json + case { + nameKey: final String name, + pathKey: final String path, + issueTrackerKey: final String issueTracker, + versionKey: final String version, + }) { + return DevToolsExtensionConfig._( + name: name, + path: path, + issueTrackerLink: issueTracker, + version: version, + materialIconCodePoint: codePoint, + ); + } else { + final requiredKeys = {nameKey, pathKey, issueTrackerKey, versionKey}; throw StateError( - 'missing required fields ${nullFields.toString()} in the extension ' - 'config.json', + 'Missing required fields ${requiredKeys.difference(json.keys.toSet())} ' + 'in the extension config.json.', ); } - - return DevToolsExtensionConfig._( - name: name!, - path: path!, - issueTrackerLink: issueTrackerLink!, - version: version!, - materialIconCodePoint: codePoint, - ); } static const nameKey = 'name'; @@ -78,14 +74,14 @@ class DevToolsExtensionConfig { final String issueTrackerLink; /// The version for the DevTools extension. - /// + /// /// This may match the version of the parent package or use a different /// versioning system as decided by the extension author. final String version; /// The code point for the material icon that will parsed by Flutter's /// [IconData] class for displaying in DevTools. - /// + /// /// This code point should be part of the 'MaterialIcons' font family. final int materialIconCodePoint; diff --git a/packages/devtools_shared/test/extensions/extension_model_test.dart b/packages/devtools_shared/test/extensions/extension_model_test.dart new file mode 100644 index 00000000000..998af6c2cc3 --- /dev/null +++ b/packages/devtools_shared/test/extensions/extension_model_test.dart @@ -0,0 +1,107 @@ +// Copyright 2023 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:devtools_shared/devtools_extensions.dart'; +import 'package:test/test.dart'; + +void main() { + group('$DevToolsExtensionConfig', () { + test('parses with a String materialIconCodePoint field', () { + final config = DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'path': 'path/to/foo/extension', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + 'materialIconCodePoint': '0xf012', + }); + + expect(config.name, 'foo'); + expect(config.path, 'path/to/foo/extension'); + expect(config.issueTrackerLink, 'www.google.com'); + expect(config.version, '1.0.0'); + expect(config.materialIconCodePoint, 0xf012); + }); + + test('parses with an int materialIconCodePoint field', () { + final config = DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'path': 'path/to/foo/extension', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + 'materialIconCodePoint': 0xf012, + }); + + expect(config.name, 'foo'); + expect(config.path, 'path/to/foo/extension'); + expect(config.issueTrackerLink, 'www.google.com'); + expect(config.version, '1.0.0'); + expect(config.materialIconCodePoint, 0xf012); + }); + + test('parses with a null materialIconCodePoint field', () { + final config = DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'path': 'path/to/foo/extension', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + }); + + expect(config.name, 'foo'); + expect(config.path, 'path/to/foo/extension'); + expect(config.issueTrackerLink, 'www.google.com'); + expect(config.version, '1.0.0'); + expect(config.materialIconCodePoint, 0xf03f); + }); + + test('parse throws when missing a required field', () { + // Missing 'name'. + expect( + () { + DevToolsExtensionConfig.parse({ + 'path': 'path/to/foo/extension', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + }); + }, + throwsA(isA()), + ); + + // Missing 'path'. + expect( + () { + DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + }); + }, + throwsA(isA()), + ); + + // Missing 'issueTracker'. + expect( + () { + DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'path': 'path/to/foo/extension', + 'version': '1.0.0', + }); + }, + throwsA(isA()), + ); + + // Missing 'version'. + expect( + () { + DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'path': 'path/to/foo/extension', + 'issueTracker': 'www.google.com', + }); + }, + throwsA(isA()), + ); + }); + }); +} From 96cf7f9c110ad29d5cac2bc35cb789ef1ebcf251 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 26 Jul 2023 10:36:59 -0700 Subject: [PATCH 3/4] review comments --- .../lib/src/extensions/extension_service.dart | 26 +++++++++++-------- .../lib/src/service/service_manager.dart | 12 +++++---- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/devtools_app/lib/src/extensions/extension_service.dart b/packages/devtools_app/lib/src/extensions/extension_service.dart index b8cba08afdd..6c6f77a19ec 100644 --- a/packages/devtools_app/lib/src/extensions/extension_service.dart +++ b/packages/devtools_app/lib/src/extensions/extension_service.dart @@ -17,9 +17,10 @@ class ExtensionService extends DisposableController Future initialize() async { await maybeRefreshExtensions(); - addAutoDisposeListener(serviceManager.connectedState, () async { - await maybeRefreshExtensions(); - }); + addAutoDisposeListener( + serviceManager.connectedState, + maybeRefreshExtensions, + ); // TODO(kenz): we should also refresh the available extensions on some event // from the analysis server that is watching the @@ -29,22 +30,25 @@ class ExtensionService extends DisposableController Future maybeRefreshExtensions() async { final appRootPath = await _connectedAppRootPath(); if (appRootPath != null) { - await _refreshAvailableExtensions(appRootPath); + _availableExtensions.value = + await server.refreshAvailableExtensions(appRootPath); } } - - Future _refreshAvailableExtensions(String? rootPath) async { - final extensions = await server.refreshAvailableExtensions(rootPath); - _availableExtensions.value = extensions; - } } Future _connectedAppRootPath() async { var fileUri = await serviceManager.rootLibraryForSelectedIsolate(); if (fileUri == null) return null; - if (fileUri.endsWith('/lib/main.dart')) { - fileUri = fileUri.replaceFirst('/lib/main.dart', ''); + // TODO(kenz): for robustness, consider sending the root library uri to the + // server and having the server look for the package folder that contains the + // `.dart_tool` directory. + + // Assume that the parent folder of `lib` is the package root. + final libDirectoryRegExp = RegExp(r'\/lib\/[^\/.]*.dart'); + final libDirectoryIndex = fileUri.indexOf(libDirectoryRegExp); + if (libDirectoryIndex != -1) { + fileUri = fileUri.substring(0, libDirectoryIndex); } return fileUri; } diff --git a/packages/devtools_app/lib/src/service/service_manager.dart b/packages/devtools_app/lib/src/service/service_manager.dart index 710c5ea4dd0..d2cf83b94ea 100644 --- a/packages/devtools_app/lib/src/service/service_manager.dart +++ b/packages/devtools_app/lib/src/service/service_manager.dart @@ -543,16 +543,18 @@ class ServiceConnectionManager { Future rootLibraryForSelectedIsolate() async { if (!connectedState.value.connected) return null; - final selectedIsolateRef = isolateManager.mainIsolate.value?.id; + final selectedIsolateRef = isolateManager.mainIsolate.value; if (selectedIsolateRef == null) return null; - final selectedIsolate = await service!.getIsolate(selectedIsolateRef); - final rootLib = selectedIsolate.rootLib?.uri; + final isolateState = isolateManager.isolateState(selectedIsolateRef); + await isolateState.waitForIsolateLoad(); + final rootLib = isolateState.rootInfo!.library; if (rootLib == null) return null; - await resolvedUriManager.fetchFileUris(selectedIsolateRef, [rootLib]); + final selectedIsolateRefId = selectedIsolateRef.id!; + await resolvedUriManager.fetchFileUris(selectedIsolateRefId, [rootLib]); return resolvedUriManager.lookupFileUri( - selectedIsolateRef, + selectedIsolateRefId, rootLib, ); } From 2f6dc69c90a681469c25b4e2697935ab19f8d19c Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 26 Jul 2023 11:04:36 -0700 Subject: [PATCH 4/4] better error handling --- .../lib/src/extensions/extension_model.dart | 26 +++++++++--- .../test/extensions/extension_model_test.dart | 42 +++++++++++++++++-- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/packages/devtools_shared/lib/src/extensions/extension_model.dart b/packages/devtools_shared/lib/src/extensions/extension_model.dart index e1ffdabe4ad..dd95b3e841c 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_model.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_model.dart @@ -40,11 +40,27 @@ class DevToolsExtensionConfig { materialIconCodePoint: codePoint, ); } else { - final requiredKeys = {nameKey, pathKey, issueTrackerKey, versionKey}; - throw StateError( - 'Missing required fields ${requiredKeys.difference(json.keys.toSet())} ' - 'in the extension config.json.', - ); + const requiredKeys = {nameKey, pathKey, issueTrackerKey, versionKey}; + final diff = requiredKeys.difference(json.keys.toSet()); + if (diff.isEmpty) { + // All the required keys are present, but the value types did not match. + final sb = StringBuffer(); + for (final entry in json.entries) { + sb.writeln( + ' ${entry.key}: ${entry.value} (${entry.value.runtimeType})', + ); + } + throw StateError( + 'Unexpected value types in the extension config.json. Expected all ' + 'values to be of type String, but one or more had a different type:\n' + '${sb.toString()}', + ); + } else { + throw StateError( + 'Missing required fields ${diff.toString()} in the extension ' + 'config.json.', + ); + } } } diff --git a/packages/devtools_shared/test/extensions/extension_model_test.dart b/packages/devtools_shared/test/extensions/extension_model_test.dart index 998af6c2cc3..50a32f00a3e 100644 --- a/packages/devtools_shared/test/extensions/extension_model_test.dart +++ b/packages/devtools_shared/test/extensions/extension_model_test.dart @@ -55,6 +55,16 @@ void main() { }); test('parse throws when missing a required field', () { + Matcher throwsMissingRequiredFieldsError() { + return throwsA( + isA().having( + (e) => e.message, + 'missing required fields StateError', + startsWith('Missing required fields'), + ), + ); + } + // Missing 'name'. expect( () { @@ -64,7 +74,7 @@ void main() { 'version': '1.0.0', }); }, - throwsA(isA()), + throwsMissingRequiredFieldsError(), ); // Missing 'path'. @@ -76,7 +86,7 @@ void main() { 'version': '1.0.0', }); }, - throwsA(isA()), + throwsMissingRequiredFieldsError(), ); // Missing 'issueTracker'. @@ -88,7 +98,7 @@ void main() { 'version': '1.0.0', }); }, - throwsA(isA()), + throwsMissingRequiredFieldsError(), ); // Missing 'version'. @@ -100,7 +110,31 @@ void main() { 'issueTracker': 'www.google.com', }); }, - throwsA(isA()), + throwsMissingRequiredFieldsError(), + ); + }); + + test('parse throws when value has unexpected type', () { + Matcher throwsUnexpectedValueTypesError() { + return throwsA( + isA().having( + (e) => e.message, + 'unexpected value types StateError', + startsWith('Unexpected value types'), + ), + ); + } + + expect( + () { + DevToolsExtensionConfig.parse({ + 'name': 23, + 'path': 'path/to/foo/extension', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + }); + }, + throwsUnexpectedValueTypesError(), ); }); });