[cleanup] Move the host configuration to Root.Runtime.hostConfig.

Ideally the host configuration would live in `core/host`, but due to the
current setup, this isn't possible. However it also doesn't make sense
to bolt it into `Settings`, since this has nothing to do with settings.
Rather, it makes sense to keep it together with the type definitions as
part of `core/root`. This also simplifies the test handling and makes it
more robust.

Fixed: 396033932
Change-Id: I8f00fc84f62b180ede4407741d261544a13218da
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6257569
Reviewed-by: Alex Rudenko <[email protected]>
Commit-Queue: Benedikt Meurer <[email protected]>
Auto-Submit: Benedikt Meurer <[email protected]>
Commit-Queue: Alex Rudenko <[email protected]>

Author: bmeurer

docs/contributing/settings-experiments-features.md

@@ -131,9 +131,9 @@ out/Default/chrome --enable-features="DevToolsNewFeature:string_param/foo/double
 
 ### In DevTools, use the new property added to HostConfig
 
-* Update the type definition in [`Runtime.ts`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/root/Runtime.ts;l=326;drc=a1e6997df9503f1c29f84e7ffebcdadbaa91ed71).
-* Update the dummy value in [`InspectorFrontendHost.ts`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/host/InspectorFrontendHost.ts;l=401;drc=197a33e1793066c8d32b8670e06cd55364121537).
-* For tests, update the stub in [`EnvironmentHelpers.ts`](https://crsrc.org/c/third_party/devtools-frontend/src/front_end/testing/EnvironmentHelpers.ts;l=494;drc=f1699bd12f8a486c749a849561391d890208f613).
-* Access the host config via `Common.Settings.Settings.instance().getHostConfig()`.
+* Update the type definition in [`Runtime.ts`](https://crsrc.org/c/third_party/devtools-frontend/src/front_end/core/root/Runtime.ts).
+* Update the dummy value returned by `getHostConfig` in [`InspectorFrontendHost.ts`](https://crsrc.org/c/third_party/devtools-frontend/src/front_end/core/host/InspectorFrontendHost.ts).
+* For tests, update the `HOST_CONFIG` in [`EnvironmentHelpers.ts`](https://crsrc.org/c/third_party/devtools-frontend/src/front_end/testing/EnvironmentHelpers.ts).
+* Access the host config via `Root.Runtime.hostConfig`.
 
 Please refer to this [example CL](https://crrev.com/c/5626314).

front_end/core/common/SettingRegistration.test.ts

@@ -8,7 +8,7 @@ import {
 } from '../../testing/EnvironmentHelpers.js';
 import * as QuickOpen from '../../ui/legacy/components/quick_open/quick_open.js';
 import * as i18n from '../i18n/i18n.js';
-import type * as Root from '../root/root.js';
+import * as Root from '../root/root.js';
 
 import * as Common from './common.js';
 
@@ -108,6 +108,13 @@ describe('SettingRegistration', () => {
 
   it('can handle settings with condition which depends on host config', () => {
     const configSettingName = 'mock-setting-with-host-config';
+    Object.assign(Root.Runtime.hostConfig, {
+      devToolsConsoleInsights: {
+        modelId: 'mockModel',
+        temperature: -1,
+        enabled: true,
+      },
+    });
     Common.Settings.registerSettingExtension({
       settingName: configSettingName,
       settingType: Common.Settings.SettingType.BOOLEAN,
@@ -124,13 +131,6 @@ describe('SettingRegistration', () => {
       syncedStorage: dummyStorage,
       globalStorage: dummyStorage,
       localStorage: dummyStorage,
-      config: {
-        devToolsConsoleInsights: {
-          modelId: 'mockModel',
-          temperature: -1,
-          enabled: true,
-        },
-      } as Root.Runtime.HostConfig,
     });
     const setting = Common.Settings.Settings.instance().moduleSetting(configSettingName);
     assert.isNotNull(setting);

front_end/core/common/SettingRegistration.ts

@@ -93,10 +93,8 @@ export function registerSettingExtension(registration: SettingRegistration): voi
   registeredSettings.push(registration);
 }
 
-export function getRegisteredSettings(config: Root.Runtime.HostConfig): Array<SettingRegistration> {
-  return registeredSettings.filter(
-      setting => Root.Runtime.Runtime.isDescriptorEnabled(
-          {experiment: setting.experiment, condition: setting.condition}, config));
+export function getRegisteredSettings(): Array<SettingRegistration> {
+  return registeredSettings.filter(setting => Root.Runtime.Runtime.isDescriptorEnabled(setting));
 }
 
 export function registerSettingsForTest(settings: Array<SettingRegistration>, forceReset: boolean = false): void {

front_end/core/common/Settings.ts

@@ -58,11 +58,12 @@ export class Settings {
   #eventSupport: ObjectWrapper<GenericEvents>;
   #registry: Map<string, Setting<unknown>>;
   readonly moduleSettings: Map<string, Setting<unknown>>;
-  #config: Root.Runtime.HostConfig;
 
   private constructor(
-      readonly syncedStorage: SettingsStorage, readonly globalStorage: SettingsStorage,
-      readonly localStorage: SettingsStorage, config?: Root.Runtime.HostConfig) {
+      readonly syncedStorage: SettingsStorage,
+      readonly globalStorage: SettingsStorage,
+      readonly localStorage: SettingsStorage,
+  ) {
     this.#sessionStorage = new SettingsStorage({});
 
     this.settingNameSet = new Set();
@@ -73,12 +74,12 @@ export class Settings {
     this.#registry = new Map();
     this.moduleSettings = new Map();
 
-    this.#config = config || {};
     for (const registration of this.getRegisteredSettings()) {
       const {settingName, defaultValue, storageType} = registration;
       const isRegex = registration.settingType === SettingType.REGEX;
 
-      const evaluatedDefaultValue = typeof defaultValue === 'function' ? defaultValue(this.#config) : defaultValue;
+      const evaluatedDefaultValue =
+          typeof defaultValue === 'function' ? defaultValue(Root.Runtime.hostConfig) : defaultValue;
       const setting = isRegex && typeof evaluatedDefaultValue === 'string' ?
           this.createRegExpSetting(settingName, evaluatedDefaultValue, undefined, storageType) :
           this.createSetting(settingName, evaluatedDefaultValue, storageType);
@@ -94,7 +95,7 @@ export class Settings {
   }
 
   getRegisteredSettings(): SettingRegistration[] {
-    return getRegisteredSettingsInternal(this.#config);
+    return getRegisteredSettingsInternal();
   }
 
   static hasInstance(): boolean {
@@ -106,15 +107,14 @@ export class Settings {
     syncedStorage: SettingsStorage|null,
     globalStorage: SettingsStorage|null,
     localStorage: SettingsStorage|null,
-    config?: Root.Runtime.HostConfig,
   } = {forceNew: null, syncedStorage: null, globalStorage: null, localStorage: null}): Settings {
-    const {forceNew, syncedStorage, globalStorage, localStorage, config} = opts;
+    const {forceNew, syncedStorage, globalStorage, localStorage} = opts;
     if (!settingsInstance || forceNew) {
       if (!syncedStorage || !globalStorage || !localStorage) {
         throw new Error(`Unable to create settings: global and local storage must be provided: ${new Error().stack}`);
       }
 
-      settingsInstance = new Settings(syncedStorage, globalStorage, localStorage, config);
+      settingsInstance = new Settings(syncedStorage, globalStorage, localStorage);
     }
 
     return settingsInstance;
@@ -124,14 +124,6 @@ export class Settings {
     settingsInstance = undefined;
   }
 
-  getHostConfig(): Root.Runtime.HostConfig {
-    return this.#config;
-  }
-
-  setHostConfig(config: Root.Runtime.HostConfig): void {
-    this.#config = config;
-  }
-
   private registerModuleSetting(setting: Setting<unknown>): void {
     const settingName = setting.name;
     const category = setting.category();
@@ -421,7 +413,7 @@ export class Setting<V> {
 
   disabled(): boolean {
     if (this.#registration?.disabledCondition) {
-      const {disabled} = this.#registration.disabledCondition(Settings.instance().getHostConfig());
+      const {disabled} = this.#registration.disabledCondition(Root.Runtime.hostConfig);
       // If registration does not disable it, pass through to #disabled
       // attribute check.
       if (disabled) {
@@ -433,7 +425,7 @@ export class Setting<V> {
 
   disabledReasons(): string[] {
     if (this.#registration?.disabledCondition) {
-      const result = this.#registration.disabledCondition(Settings.instance().getHostConfig());
+      const result = this.#registration.disabledCondition(Root.Runtime.hostConfig);
       if (result.disabled) {
         return result.reasons;
       }

front_end/core/host/AidaClient.test.ts

@@ -2,27 +2,27 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import {describeWithEnvironment, getGetHostConfigStub} from '../../testing/EnvironmentHelpers.js';
+import {describeWithEnvironment} from '../../testing/EnvironmentHelpers.js';
+import * as Root from '../root/root.js';
 
 import * as Host from './host.js';
 
 const TEST_MODEL_ID = 'testModelId';
 
 describeWithEnvironment('AidaClient', () => {
   it('adds no model temperature if console insights is not enabled', () => {
-    const stub = getGetHostConfigStub({});
+    Object.assign(Root.Runtime.hostConfig, {});
     const request = Host.AidaClient.AidaClient.buildConsoleInsightsRequest('foo');
     assert.deepEqual(request, {
       current_message: {parts: [{text: 'foo'}], role: Host.AidaClient.Role.USER},
       client: 'CHROME_DEVTOOLS',
       client_feature: 1,
       functionality_type: 2,
     });
-    stub.restore();
   });
 
   it('adds a model temperature', () => {
-    const stub = getGetHostConfigStub({
+    Object.assign(Root.Runtime.hostConfig, {
       devToolsConsoleInsights: {
         enabled: true,
         temperature: 0.5,
@@ -38,11 +38,10 @@ describeWithEnvironment('AidaClient', () => {
       client_feature: 1,
       functionality_type: 2,
     });
-    stub.restore();
   });
 
   it('adds a model temperature of 0', () => {
-    const stub = getGetHostConfigStub({
+    Object.assign(Root.Runtime.hostConfig, {
       devToolsConsoleInsights: {
         enabled: true,
         temperature: 0,
@@ -58,11 +57,10 @@ describeWithEnvironment('AidaClient', () => {
       client_feature: 1,
       functionality_type: 2,
     });
-    stub.restore();
   });
 
   it('ignores a negative model temperature', () => {
-    const stub = getGetHostConfigStub({
+    Object.assign(Root.Runtime.hostConfig, {
       devToolsConsoleInsights: {
         enabled: true,
         temperature: -1,
@@ -75,11 +73,10 @@ describeWithEnvironment('AidaClient', () => {
       client_feature: 1,
       functionality_type: 2,
     });
-    stub.restore();
   });
 
   it('adds a model id and temperature', () => {
-    const stub = getGetHostConfigStub({
+    Object.assign(Root.Runtime.hostConfig, {
       devToolsConsoleInsights: {
         enabled: true,
         modelId: TEST_MODEL_ID,
@@ -97,11 +94,10 @@ describeWithEnvironment('AidaClient', () => {
       client_feature: 1,
       functionality_type: 2,
     });
-    stub.restore();
   });
 
   it('adds metadata to disallow logging', () => {
-    const stub = getGetHostConfigStub({
+    Object.assign(Root.Runtime.hostConfig, {
       aidaAvailability: {
         disallowLogging: true,
       },
@@ -123,7 +119,6 @@ describeWithEnvironment('AidaClient', () => {
       client_feature: 1,
       functionality_type: 2,
     });
-    stub.restore();
   });
 
   async function getAllResults(provider: Host.AidaClient.AidaClient): Promise<Host.AidaClient.AidaResponse[]> {

front_end/core/host/AidaClient.ts

@@ -3,7 +3,7 @@
 // found in the LICENSE file.
 
 import * as Common from '../common/common.js';
-import type * as Root from '../root/root.js';
+import * as Root from '../root/root.js';
 
 import {InspectorFrontendHostInstance} from './InspectorFrontendHost.js';
 import type {AidaClientResult, SyncInformation} from './InspectorFrontendHostAPI.js';
@@ -268,14 +268,14 @@ export class AidaClient {
       functionality_type: FunctionalityType.EXPLAIN_ERROR,
       client_feature: ClientFeature.CHROME_CONSOLE_INSIGHTS,
     };
-    const config = Common.Settings.Settings.instance().getHostConfig();
+    const {hostConfig} = Root.Runtime;
     let temperature = -1;
     let modelId = '';
-    if (config.devToolsConsoleInsights?.enabled) {
-      temperature = config.devToolsConsoleInsights.temperature ?? -1;
-      modelId = config.devToolsConsoleInsights.modelId || '';
+    if (hostConfig.devToolsConsoleInsights?.enabled) {
+      temperature = hostConfig.devToolsConsoleInsights.temperature ?? -1;
+      modelId = hostConfig.devToolsConsoleInsights.modelId || '';
     }
-    const disallowLogging = config.aidaAvailability?.disallowLogging ?? true;
+    const disallowLogging = hostConfig.aidaAvailability?.disallowLogging ?? true;
 
     if (temperature >= 0) {
       request.options ??= {};
@@ -498,9 +498,9 @@ export class HostConfigTracker extends Common.ObjectWrapper.ObjectWrapper<EventT
     const currentAidaAvailability = await AidaClient.checkAccessPreconditions();
     if (currentAidaAvailability !== this.#aidaAvailability) {
       this.#aidaAvailability = currentAidaAvailability;
-      const config = await new Promise<Root.Runtime.HostConfig>(
-          resolve => InspectorFrontendHostInstance.getHostConfig(config => resolve(config)));
-      Common.Settings.Settings.instance().setHostConfig(config);
+      const config =
+          await new Promise<Root.Runtime.HostConfig>(resolve => InspectorFrontendHostInstance.getHostConfig(resolve));
+      Object.assign(Root.Runtime.hostConfig, config);
       this.dispatchEventToListeners(Events.AIDA_AVAILABILITY_CHANGED);
     }
   }

front_end/core/root/Runtime.ts

@@ -80,12 +80,7 @@ export class Runtime {
     return runtimePlatform;
   }
 
-  static isDescriptorEnabled(
-      descriptor: {
-        experiment: ((string | undefined)|null),
-        condition?: Condition,
-      },
-      config?: HostConfig): boolean {
+  static isDescriptorEnabled(descriptor: {experiment?: string|null, condition?: Condition}): boolean {
     const {experiment} = descriptor;
     if (experiment === '*') {
       return true;
@@ -97,7 +92,7 @@ export class Runtime {
       return false;
     }
     const {condition} = descriptor;
-    return condition ? condition(config) : true;
+    return condition ? condition(hostConfig) : true;
   }
 
   loadLegacyModule(modulePath: string): Promise<void> {
@@ -412,13 +407,19 @@ export interface HostConfigThirdPartyCookieControls {
   managedBlockThirdPartyCookies: string|boolean;
 }
 
-// We use `RecursivePartial` here to enforce that DevTools code is able to
-// handle `HostConfig` objects of an unexpected shape. This can happen if
-// the implementation in the Chromium backend is changed without correctly
-// updating the DevTools frontend. Or if remote debugging a different version
-// of Chrome, resulting in the local browser window and the local DevTools
-// window being of different versions, and consequently potentially having
-// differently shaped `HostConfig`s.
+/**
+ * The host configuration that we expect from the DevTools back-end.
+ *
+ * We use `RecursivePartial` here to enforce that DevTools code is able to
+ * handle `HostConfig` objects of an unexpected shape. This can happen if
+ * the implementation in the Chromium backend is changed without correctly
+ * updating the DevTools frontend. Or if remote debugging a different version
+ * of Chrome, resulting in the local browser window and the local DevTools
+ * window being of different versions, and consequently potentially having
+ * differently shaped `HostConfig`s.
+ *
+ * @see hostConfig
+ */
 export type HostConfig = Platform.TypeScriptUtilities.RecursivePartial<{
   aidaAvailability: AidaAvailability,
   devToolsConsoleInsights: HostConfigConsoleInsights,
@@ -440,6 +441,22 @@ export type HostConfig = Platform.TypeScriptUtilities.RecursivePartial<{
   thirdPartyCookieControls: HostConfigThirdPartyCookieControls,
 }>;
 
+/**
+ * The host configuration for this DevTools instance.
+ *
+ * This is initialized early during app startup and should not be modified
+ * afterwards. In some cases it can be necessary to re-request the host
+ * configuration from Chrome while DevTools is already running. In these
+ * cases, the new host configuration should be reflected here, e.g.:
+ *
+ * ```js
+ * const config = await new Promise<Root.Runtime.HostConfig>(
+ *   resolve => InspectorFrontendHostInstance.getHostConfig(resolve));
+ * Object.assign(Root.runtime.hostConfig, config);
+ * ```
+ */
+export const hostConfig: HostConfig = Object.create(null);
+
 /**
  * When defining conditions make sure that objects used by the function have
  * been instantiated.

front_end/core/sdk/CSSModel.ts

@@ -38,6 +38,7 @@ import * as TextUtils from '../../models/text_utils/text_utils.js';
 import * as Common from '../common/common.js';
 import * as Host from '../host/host.js';
 import * as Platform from '../platform/platform.js';
+import * as Root from '../root/root.js';
 
 import {CSSFontFace} from './CSSFontFace.js';
 import {CSSMatchedStyles} from './CSSMatchedStyles.js';
@@ -355,8 +356,7 @@ export class CSSModel extends SDKModel<EventTypes> {
       return null;
     }
 
-    const shouldGetAnimatedStyles =
-        Common.Settings.Settings.instance().getHostConfig().devToolsAnimationStylesInStylesTab?.enabled;
+    const shouldGetAnimatedStyles = Root.Runtime.hostConfig.devToolsAnimationStylesInStylesTab?.enabled;
     const [matchedStylesResponse, animatedStylesResponse] = await Promise.all([
       this.agent.invoke_getMatchedStylesForNode({nodeId}),
       shouldGetAnimatedStyles ? this.agent.invoke_getAnimatedStylesForNode({nodeId}) : undefined,