Address review comments

Author: srujzs

dwds/lib/src/debugging/inspector.dart

@@ -35,9 +35,9 @@ import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
 class AppInspector implements AppInspectorInterface {
   var _scriptCacheMemoizer = AsyncMemoizer<List<ScriptRef>>();
 
-  Future<List<ScriptRef>> getScriptRefs([
+  Future<List<ScriptRef>> getScriptRefs({
     ModifiedModuleReport? modifiedModuleReport,
-  ]) => _populateScriptCaches(modifiedModuleReport);
+  }) => _populateScriptCaches(modifiedModuleReport: modifiedModuleReport);
 
   final _logger = Logger('AppInspector');
 
@@ -107,7 +107,7 @@ class AppInspector implements AppInspectorInterface {
   /// Reset all caches and recompute any mappings.
   ///
   /// Should be called across hot reloads with a valid [ModifiedModuleReport].
-  Future<void> initialize([ModifiedModuleReport? modifiedModuleReport]) async {
+  Future<void> initialize({ModifiedModuleReport? modifiedModuleReport}) async {
     _scriptCacheMemoizer = AsyncMemoizer<List<ScriptRef>>();
 
     // TODO(srujzs): We can invalidate these in a smarter way instead of
@@ -118,7 +118,7 @@ class AppInspector implements AppInspectorInterface {
 
     if (modifiedModuleReport != null) {
       // Invalidate `_libraryHelper` as we use it populate any script caches.
-      _libraryHelper.initialize(modifiedModuleReport);
+      _libraryHelper.initialize(modifiedModuleReport: modifiedModuleReport);
     } else {
       _libraryHelper = LibraryHelper(this)..initialize();
       _scriptRefsById.clear();
@@ -132,7 +132,9 @@ class AppInspector implements AppInspectorInterface {
     isolate.libraries?.clear();
     isolate.libraries?.addAll(libraries);
 
-    final scripts = await getScriptRefs(modifiedModuleReport);
+    final scripts = await getScriptRefs(
+      modifiedModuleReport: modifiedModuleReport,
+    );
 
     await DartUri.initialize();
     DartUri.recordAbsoluteUris(libraries.map((lib) => lib.uri).nonNulls);
@@ -730,9 +732,9 @@ class AppInspector implements AppInspectorInterface {
   /// recalculates caches for the modified libraries.
   ///
   /// Returns the list of scripts refs cached.
-  Future<List<ScriptRef>> _populateScriptCaches([
+  Future<List<ScriptRef>> _populateScriptCaches({
     ModifiedModuleReport? modifiedModuleReport,
-  ]) {
+  }) {
     return _scriptCacheMemoizer.runOnce(() async {
       final scripts =
           await globalToolConfiguration.loadStrategy

dwds/lib/src/debugging/libraries.dart

@@ -17,10 +17,10 @@ class LibraryHelper extends Domain {
   final Logger _logger = Logger('LibraryHelper');
 
   /// Map of library ID to [Library].
-  late final Map<String, Library> _librariesById;
+  final Map<String, Library> _librariesById = {};
 
   /// Map of libraryRef ID to [LibraryRef].
-  late final Map<String, LibraryRef> _libraryRefsById;
+  final Map<String, LibraryRef> _libraryRefsById = {};
 
   LibraryRef? _rootLib;
 
@@ -32,7 +32,7 @@ class LibraryHelper extends Domain {
   ///
   /// If [modifiedModuleReport] is not null, invalidates only modified libraries
   /// from the cache and recomputes values for any eager caches.
-  void initialize([ModifiedModuleReport? modifiedModuleReport]) {
+  void initialize({ModifiedModuleReport? modifiedModuleReport}) {
     _rootLib = null;
     if (modifiedModuleReport != null) {
       for (final library in modifiedModuleReport.modifiedLibraries) {
@@ -45,10 +45,10 @@ class LibraryHelper extends Domain {
         // map is empty before returning.
         _libraryRefsById[library] = _createLibraryRef(library);
       }
-    } else {
-      _librariesById = <String, Library>{};
-      _libraryRefsById = <String, LibraryRef>{};
+      return;
     }
+    _librariesById.clear();
+    _libraryRefsById.clear();
   }
 
   Future<LibraryRef> get rootLib async {

dwds/lib/src/debugging/location.dart

@@ -152,10 +152,14 @@ class Locations {
 
   Modules get modules => _modules;
 
+  /// Initialize any caches.
+  ///
+  /// If [modifiedModuleReport] is not null, only invalidates the caches for the
+  /// modified modules instead.
   Future<void> initialize(
-    String entrypoint, [
+    String entrypoint, {
     ModifiedModuleReport? modifiedModuleReport,
-  ]) async {
+  }) async {
     // If we know that only certain modules are deleted or added, we can only
     // invalidate those.
     if (modifiedModuleReport != null) {
@@ -170,13 +174,13 @@ class Locations {
           }
         }
       }
-    } else {
-      _locationMemoizer.clear();
-      _moduleToLocations.clear();
-      _sourceToTokenPosTable.clear();
-      _sourceToLocation.clear();
-      _entrypoint = entrypoint;
+      return;
     }
+    _locationMemoizer.clear();
+    _moduleToLocations.clear();
+    _sourceToTokenPosTable.clear();
+    _sourceToLocation.clear();
+    _entrypoint = entrypoint;
   }
 
   /// Returns all [Location] data for a provided Dart source.

dwds/lib/src/debugging/metadata/provider.dart

@@ -170,14 +170,10 @@ class MetadataProvider {
     return _moduleToModulePath.keys.toList();
   }
 
-  /// Compute metadata information after reading the metadata contents.
-  ///
-  /// If [hotReload] is true, skips adding the SDK metadata and caches the
-  /// computed [ModuleMetadata], returning the resulting cache.
-  ///
-  /// Otherwise, adds all metadata and returns null.
-  Future<Map<String, ModuleMetadata>?> _processMetadata(bool hotReload) async {
-    final modules = hotReload ? <String, ModuleMetadata>{} : null;
+  /// Compute metadata information after reading the metadata contents and
+  /// return a map from module names to their [ModuleMetadata].
+  Future<Map<String, ModuleMetadata>> _processMetadata() async {
+    final modules = <String, ModuleMetadata>{};
     // The merged metadata resides next to the entrypoint.
     // Assume that <name>.bootstrap.js has <name>.ddc_merged_metadata
     if (entrypoint.endsWith('.bootstrap.js')) {
@@ -188,8 +184,6 @@ class MetadataProvider {
       );
       final merged = await _assetReader.metadataContents(serverPath);
       if (merged != null) {
-        // We can't hot reload the SDK yet, so no need to invalidate this data.
-        if (!hotReload) _addSdkMetadata();
         for (final contents in merged.split('\n')) {
           try {
             if (contents.isEmpty ||
@@ -201,11 +195,7 @@ class MetadataProvider {
               moduleJson as Map<String, dynamic>,
             );
             final moduleName = metadata.name;
-            if (hotReload) {
-              modules![moduleName] = metadata;
-            } else {
-              _addMetadata(metadata);
-            }
+            modules[moduleName] = metadata;
             _logger.fine('Loaded debug metadata for module: $moduleName');
           } catch (e) {
             _logger.warning('Failed to read metadata: $e');
@@ -217,9 +207,13 @@ class MetadataProvider {
     return modules;
   }
 
-  /// Process all metadata and compute caches once.
+  /// Process all metadata, including SDK metadata, and compute caches once.
   Future<void> _initialize() async {
-    await _metadataMemoizer.runOnce(() => _processMetadata(false));
+    await _metadataMemoizer.runOnce(() async {
+      final metadata = await _processMetadata();
+      _addSdkMetadata();
+      metadata.values.forEach(_addMetadata);
+    });
   }
 
   /// Given a map of hot reloaded modules mapped to their respective libraries,
@@ -231,7 +225,7 @@ class MetadataProvider {
   Future<ModifiedModuleReport> reinitializeAfterHotReload(
     Map<String, List> reloadedModulesToLibraries,
   ) async {
-    final modules = (await _processMetadata(true))!;
+    final modules = await _processMetadata();
     final invalidatedLibraries = <String>{};
     void invalidateLibrary(String libraryImportUri) {
       invalidatedLibraries.add(libraryImportUri);

dwds/lib/src/debugging/modules.dart

@@ -38,9 +38,9 @@ class Modules {
   /// If [modifiedModuleReport] is not null, removes and recalculates caches for
   /// any modified modules and libraries.
   Future<void> initialize(
-    String entrypoint, [
+    String entrypoint, {
     ModifiedModuleReport? modifiedModuleReport,
-  ]) async {
+  }) async {
     if (modifiedModuleReport != null) {
       assert(_entrypoint == entrypoint);
       for (final library in modifiedModuleReport.modifiedLibraries) {
@@ -53,14 +53,14 @@ class Modules {
         _moduleToSources.remove(module);
       }
       await _initializeMapping(modifiedModuleReport);
-    } else {
-      _entrypoint = entrypoint;
-      _sourceToLibrary.clear();
-      _sourceToModule.clear();
-      _libraryToModule.clear();
-      _moduleToSources.clear();
-      _moduleMemoizer = AsyncMemoizer();
+      return;
     }
+    _entrypoint = entrypoint;
+    _sourceToLibrary.clear();
+    _sourceToModule.clear();
+    _libraryToModule.clear();
+    _moduleToSources.clear();
+    _moduleMemoizer = AsyncMemoizer();
   }
 
   /// Returns the containing module for the provided Dart server path.

dwds/lib/src/debugging/skip_list.dart

@@ -18,26 +18,29 @@ class SkipLists {
 
   SkipLists(this._root);
 
-  Future<void> initialize([
-    String? entrypoint,
+  /// Initialize any caches.
+  ///
+  /// If [modifiedModuleReport] is not null, only invalidates the caches for the
+  /// modified modules instead.
+  Future<void> initialize(
+    String entrypoint, {
     ModifiedModuleReport? modifiedModuleReport,
-  ]) async {
+  }) async {
     if (modifiedModuleReport != null) {
-      assert(entrypoint != null);
       for (final url in _urlToId.keys) {
         final dartUri = DartUri(url, _root);
         final serverPath = dartUri.serverPath;
         final module = await globalToolConfiguration.loadStrategy
-            .moduleForServerPath(entrypoint!, serverPath);
+            .moduleForServerPath(entrypoint, serverPath);
         if (modifiedModuleReport.modifiedModules.contains(module)) {
           _idToList.remove(_urlToId[url]!);
           _urlToId.remove(url);
         }
       }
-    } else {
-      _idToList.clear();
-      _urlToId.clear();
+      return;
     }
+    _idToList.clear();
+    _urlToId.clear();
   }
 
   /// Returns a skipList as defined by the Chrome DevTools Protocol.

dwds/lib/src/services/chrome_proxy_service.dart

@@ -243,21 +243,33 @@ class ChromeProxyService implements VmServiceInterface {
     final entrypoint = inspector.appConnection.request.entrypointPath;
     final modifiedModuleReport = await globalToolConfiguration.loadStrategy
         .reinitializeProviderAfterHotReload(entrypoint, reloadedModules);
-    await _initializeEntrypoint(entrypoint, modifiedModuleReport);
-    await inspector.initialize(modifiedModuleReport);
+    await _initializeEntrypoint(
+      entrypoint,
+      modifiedModuleReport: modifiedModuleReport,
+    );
+    await inspector.initialize(modifiedModuleReport: modifiedModuleReport);
   }
 
   /// Initializes metadata in [Locations], [Modules], and [ExpressionCompiler].
   ///
   /// If [modifiedModuleReport] is not null, only removes and reinitializes
   /// modified metadata.
   Future<void> _initializeEntrypoint(
-    String entrypoint, [
+    String entrypoint, {
     ModifiedModuleReport? modifiedModuleReport,
-  ]) async {
-    await _modules.initialize(entrypoint, modifiedModuleReport);
-    await _locations.initialize(entrypoint, modifiedModuleReport);
-    await _skipLists.initialize(entrypoint, modifiedModuleReport);
+  }) async {
+    await _modules.initialize(
+      entrypoint,
+      modifiedModuleReport: modifiedModuleReport,
+    );
+    await _locations.initialize(
+      entrypoint,
+      modifiedModuleReport: modifiedModuleReport,
+    );
+    await _skipLists.initialize(
+      entrypoint,
+      modifiedModuleReport: modifiedModuleReport,
+    );
     // We do not need to wait for compiler dependencies to be updated as the
     // [ExpressionEvaluator] is robust to evaluation requests during updates.
     safeUnawaited(_updateCompilerDependencies(entrypoint));

dwds/test/fixtures/fakes.dart

@@ -68,7 +68,7 @@ class FakeInspector implements AppInspector {
   }
 
   @override
-  Future<void> initialize([ModifiedModuleReport? modifiedModuleReport]) async =>
+  Future<void> initialize({ModifiedModuleReport? modifiedModuleReport}) async =>
       {};
 
   @override
@@ -159,9 +159,9 @@ class FakeModules implements Modules {
 
   @override
   Future<void> initialize(
-    String entrypoint, [
+    String entrypoint, {
     ModifiedModuleReport? modifiedModuleReport,
-  ]) async {}
+  }) async {}
 
   @override
   Future<Uri> libraryForSource(String serverPath) async => Uri(path: _library);