Add branded type for user-authored breakpoint conditions

This CL adds a new `UserCondition` type to mark user-authored
breakpoint conditions.

Note that there are only 2 places that can create these (modulo
pre-defined constants):
  1) When we load breakpoints from settings storage
  2) The result of the breakpoint edit dialog

Outside  of those two places we should never have to cast a string
to this new type.

R=kimanh@chromium.org

Bug: 1027458
Change-Id: Id49fa42207b813e5380c7ccf30fe6ab761525411
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/4190003
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>

front_end/models/bindings/BreakpointManager.ts

@@ -308,7 +308,7 @@ export class BreakpointManager extends Common.ObjectWrapper.ObjectWrapper<EventT
 
   async setBreakpoint(
       uiSourceCode: Workspace.UISourceCode.UISourceCode, lineNumber: number, columnNumber: number|undefined,
-      condition: string, enabled: boolean, isLogpoint: boolean, origin: BreakpointOrigin): Promise<Breakpoint> {
+      condition: UserCondition, enabled: boolean, isLogpoint: boolean, origin: BreakpointOrigin): Promise<Breakpoint> {
     let uiLocation: Workspace.UISourceCode.UILocation =
         new Workspace.UISourceCode.UILocation(uiSourceCode, lineNumber, columnNumber);
     const normalizedLocation = await this.debuggerWorkspaceBinding.normalizeUILocation(uiLocation);
@@ -324,7 +324,7 @@ export class BreakpointManager extends Common.ObjectWrapper.ObjectWrapper<EventT
 
   private innerSetBreakpoint(
       uiSourceCode: Workspace.UISourceCode.UISourceCode, lineNumber: number, columnNumber: number|undefined,
-      condition: string, enabled: boolean, isLogpoint: boolean, origin: BreakpointOrigin): Breakpoint {
+      condition: UserCondition, enabled: boolean, isLogpoint: boolean, origin: BreakpointOrigin): Breakpoint {
     const url = BreakpointManager.getScriptForInlineUiSourceCode(uiSourceCode)?.sourceURL ?? uiSourceCode.url();
     const resourceTypeName = uiSourceCode.contentType().name();
     const storageId =
@@ -537,8 +537,8 @@ export class Breakpoint implements SDK.TargetManager.SDKModelObserver<SDK.Debugg
 
   constructor(
       breakpointManager: BreakpointManager, primaryUISourceCode: Workspace.UISourceCode.UISourceCode,
-      url: Platform.DevToolsPath.UrlString, lineNumber: number, columnNumber: number|undefined, condition: string,
-      enabled: boolean, isLogpoint: boolean, origin: BreakpointOrigin) {
+      url: Platform.DevToolsPath.UrlString, lineNumber: number, columnNumber: number|undefined,
+      condition: UserCondition, enabled: boolean, isLogpoint: boolean, origin: BreakpointOrigin) {
     this.breakpointManager = breakpointManager;
     this.#origin = origin;
 
@@ -712,7 +712,7 @@ export class Breakpoint implements SDK.TargetManager.SDKModelObserver<SDK.Debugg
   /**
    * The breakpoint condition as entered by the user.
    */
-  condition(): string {
+  condition(): UserCondition {
     return this.#storageState.condition;
   }
 
@@ -725,10 +725,10 @@ export class Breakpoint implements SDK.TargetManager.SDKModelObserver<SDK.Debugg
       return `${LOGPOINT_PREFIX}${this.#storageState.condition}${LOGPOINT_SUFFIX}` as
           SDK.DebuggerModel.BackendCondition;
     }
-    return this.#storageState.condition as SDK.DebuggerModel.BackendCondition;
+    return this.#storageState.condition as unknown as SDK.DebuggerModel.BackendCondition;
   }
 
-  setCondition(condition: string, isLogpoint: boolean): void {
+  setCondition(condition: UserCondition, isLogpoint: boolean): void {
     this.updateState({...this.#storageState, condition, isLogpoint});
   }
 
@@ -1226,6 +1226,14 @@ class Storage {
   }
 }
 
+/**
+ * A breakpoint condition as entered by the user. We use the type to
+ * distinguish from {@link SDK.DebuggerModel.BackendCondition}.
+ */
+export type UserCondition = Platform.Brand.Brand<string, 'UserCondition'>;
+export const EMPTY_BREAKPOINT_CONDITION = '' as UserCondition;
+export const NEVER_PAUSE_HERE_CONDITION = 'false' as UserCondition;
+
 /**
  * All the data for a single `Breakpoint` thats stored in the settings.
  * Whenever any of these change, we need to update the settings.
@@ -1235,7 +1243,7 @@ interface BreakpointStorageState {
   readonly resourceTypeName: string;
   readonly lineNumber: number;
   readonly columnNumber?: number;
-  readonly condition: string;
+  readonly condition: UserCondition;
   readonly enabled: boolean;
   readonly isLogpoint: boolean;
 }

front_end/panels/sources/BreakpointEditDialog.ts

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import type * as Bindings from '../../models/bindings/bindings.js';
 import * as i18n from '../../core/i18n/i18n.js';
 import * as CodeMirror from '../../third_party/codemirror.next/codemirror.next.js';
 import * as TextEditor from '../../ui/components/text_editor/text_editor.js';
@@ -51,7 +52,7 @@ const i18nString = i18n.i18n.getLocalizedString.bind(undefined, str_);
 
 export interface BreakpointEditDialogResult {
   committed: boolean;
-  condition: string;
+  condition: Bindings.BreakpointManager.UserCondition;
   isLogpoint: boolean;
 }
 
@@ -202,7 +203,8 @@ export class BreakpointEditDialog extends UI.Widget.Widget {
     }
     this.finished = true;
     this.editor.remove();
-    this.onFinish({committed, condition, isLogpoint: this.isLogpoint});
+    this.onFinish(
+        {committed, condition: condition as Bindings.BreakpointManager.UserCondition, isLogpoint: this.isLogpoint});
   }
 
   wasShown(): void {

front_end/panels/sources/DebuggerPlugin.ts

@@ -52,6 +52,8 @@ import {ScriptFormatterEditorAction} from './ScriptFormatterEditorAction.js';
 import {SourcesPanel} from './SourcesPanel.js';
 import {getRegisteredEditorActions} from './SourcesView.js';
 
+const {EMPTY_BREAKPOINT_CONDITION, NEVER_PAUSE_HERE_CONDITION} = Bindings.BreakpointManager;
+
 const UIStrings = {
   /**
    *@description Text in Debugger Plugin of the Sources panel
@@ -463,7 +465,8 @@ export class DebuggerPlugin extends Plugin {
       if (this.editor && SourceFrame.SourceFrame.isBreakableLine(this.editor.state, line)) {
         contextMenu.debugSection().appendItem(
             i18nString(UIStrings.addBreakpoint),
-            this.createNewBreakpoint.bind(this, line, '', /* enabled */ true, /* isLogpoint */ false));
+            this.createNewBreakpoint.bind(
+                this, line, EMPTY_BREAKPOINT_CONDITION, /* enabled */ true, /* isLogpoint */ false));
         if (supportsConditionalBreakpoints) {
           contextMenu.debugSection().appendItem(i18nString(UIStrings.addConditionalBreakpoint), () => {
             Host.userMetrics.breakpointEditDialogRevealedFrom(
@@ -477,7 +480,8 @@ export class DebuggerPlugin extends Plugin {
           });
           contextMenu.debugSection().appendItem(
               i18nString(UIStrings.neverPauseHere),
-              this.createNewBreakpoint.bind(this, line, 'false', /* enabled */ true, /* isLogpoint */ false));
+              this.createNewBreakpoint.bind(
+                  this, line, NEVER_PAUSE_HERE_CONDITION, /* enabled */ true, /* isLogpoint */ false));
         }
       }
     } else {
@@ -1265,7 +1269,8 @@ export class DebuggerPlugin extends Plugin {
       const line = this.editor.state.doc.lineAt(editorLocation);
       const uiLocation = this.transformer.editorLocationToUILocation(line.number - 1, editorLocation - line.from);
       void this.setBreakpoint(
-          uiLocation.lineNumber, uiLocation.columnNumber, '', /* enabled */ true, /* isLogpoint */ false);
+          uiLocation.lineNumber, uiLocation.columnNumber, EMPTY_BREAKPOINT_CONDITION, /* enabled */ true,
+          /* isLogpoint */ false);
     }
   }
 
@@ -1305,7 +1310,8 @@ export class DebuggerPlugin extends Plugin {
       contextMenu.debugSection().appendItem(
           i18nString(UIStrings.neverPauseHere),
           () => this.setBreakpoint(
-              uiLocation.lineNumber, uiLocation.columnNumber, 'false', /* enabled */ true, /* isLogpoint */ false));
+              uiLocation.lineNumber, uiLocation.columnNumber, NEVER_PAUSE_HERE_CONDITION, /* enabled */ true,
+              /* isLogpoint */ false));
     }
     void contextMenu.show();
   }
@@ -1469,7 +1475,7 @@ export class DebuggerPlugin extends Plugin {
 
     const breakpoints = this.lineBreakpoints(line);
     if (!breakpoints.length) {
-      await this.createNewBreakpoint(line, '', /* enabled */ true, /* isLogpoint */ false);
+      await this.createNewBreakpoint(line, EMPTY_BREAKPOINT_CONDITION, /* enabled */ true, /* isLogpoint */ false);
       return;
     }
     const hasDisabled = breakpoints.some(b => !b.enabled());
@@ -1482,8 +1488,9 @@ export class DebuggerPlugin extends Plugin {
     }
   }
 
-  private async createNewBreakpoint(line: CodeMirror.Line, condition: string, enabled: boolean, isLogpoint: boolean):
-      Promise<void> {
+  private async createNewBreakpoint(
+      line: CodeMirror.Line, condition: Bindings.BreakpointManager.UserCondition, enabled: boolean,
+      isLogpoint: boolean): Promise<void> {
     if (!this.editor || !SourceFrame.SourceFrame.isBreakableLine(this.editor.state, line)) {
       return;
     }
@@ -1493,8 +1500,8 @@ export class DebuggerPlugin extends Plugin {
   }
 
   private async setBreakpoint(
-      lineNumber: number, columnNumber: number|undefined, condition: string, enabled: boolean,
-      isLogpoint: boolean): Promise<void> {
+      lineNumber: number, columnNumber: number|undefined, condition: Bindings.BreakpointManager.UserCondition,
+      enabled: boolean, isLogpoint: boolean): Promise<void> {
     Common.Settings.Settings.instance().moduleSetting('breakpointsActive').set(true);
     await this.breakpointManager.setBreakpoint(
         this.uiSourceCode, lineNumber, columnNumber, condition, enabled, isLogpoint,

test/unittests/front_end/models/bindings/BreakpointManager_test.ts

@@ -52,12 +52,13 @@ describeWithMockConnection('BreakpointManager (mock backend)', () => {
     hasSourceURL: false,
   };
 
-  const DEFAULT_BREAKPOINT: [string, boolean, boolean, Bindings.BreakpointManager.BreakpointOrigin] = [
-    '',     // condition
-    true,   // enabled
-    false,  // isLogpoint
-    Bindings.BreakpointManager.BreakpointOrigin.OTHER,
-  ];
+  const DEFAULT_BREAKPOINT:
+      [Bindings.BreakpointManager.UserCondition, boolean, boolean, Bindings.BreakpointManager.BreakpointOrigin] = [
+        Bindings.BreakpointManager.EMPTY_BREAKPOINT_CONDITION,
+        true,   // enabled
+        false,  // isLogpoint
+        Bindings.BreakpointManager.BreakpointOrigin.OTHER,
+      ];
 
   let target: SDK.Target.Target;
   let backend: MockProtocolBackend;

test/unittests/front_end/panels/sources/BreakpointsSidebarPane_test.ts

@@ -27,7 +27,7 @@ interface LocationTestData {
   columnNumber: number;
   enabled: boolean;
   content: string;
-  condition: string;
+  condition: Bindings.BreakpointManager.UserCondition;
   isLogpoint: boolean;
   hoverText?: string;
 }
@@ -74,7 +74,8 @@ function createStubBreakpointManagerAndSettingsWithMockdata(testData: LocationTe
 
 function createLocationTestData(
     url: string, lineNumber: number, columnNumber: number, enabled: boolean = true, content: string = '',
-    condition: string = '', isLogpoint: boolean = false, hoverText?: string): LocationTestData {
+    condition: Bindings.BreakpointManager.UserCondition = Bindings.BreakpointManager.EMPTY_BREAKPOINT_CONDITION,
+    isLogpoint: boolean = false, hoverText?: string): LocationTestData {
   return {
     url: url as Platform.DevToolsPath.UrlString,
     lineNumber,
@@ -426,7 +427,7 @@ describeWithEnvironment('BreakpointsSidebarController', () => {
     });
 
     it('correctly extracts conditional breakpoints', async () => {
-      const condition = 'x < a';
+      const condition = 'x < a' as Bindings.BreakpointManager.UserCondition;
       const testData = [
         createLocationTestData(
             TEST_JS_FILE, 3, 15, true /* enabled */, '', condition, false /* isLogpoint */, condition),
@@ -444,7 +445,7 @@ describeWithEnvironment('BreakpointsSidebarController', () => {
     });
 
     it('correctly extracts logpoints', async () => {
-      const logExpression = 'x';
+      const logExpression = 'x' as Bindings.BreakpointManager.UserCondition;
       const testData = [
         createLocationTestData(
             TEST_JS_FILE, 3, 15, true /* enabled */, '', logExpression, true /* isLogpoint */, logExpression),
@@ -527,12 +528,13 @@ describeWithEnvironment('BreakpointsSidebarController', () => {
 });
 
 describeWithRealConnection('BreakpointsSidebarController', () => {
-  const DEFAULT_BREAKPOINT: [string, boolean, boolean, Bindings.BreakpointManager.BreakpointOrigin] = [
-    '',     // condition
-    true,   // enabled
-    false,  // isLogpoint
-    Bindings.BreakpointManager.BreakpointOrigin.USER_ACTION,
-  ];
+  const DEFAULT_BREAKPOINT:
+      [Bindings.BreakpointManager.UserCondition, boolean, boolean, Bindings.BreakpointManager.BreakpointOrigin] = [
+        Bindings.BreakpointManager.EMPTY_BREAKPOINT_CONDITION,
+        true,   // enabled
+        false,  // isLogpoint
+        Bindings.BreakpointManager.BreakpointOrigin.USER_ACTION,
+      ];
 
   it('auto-expands if a user adds a new  breakpoint', async () => {
     const breakpointManager = Bindings.BreakpointManager.BreakpointManager.instance();
@@ -587,7 +589,8 @@ describeWithRealConnection('BreakpointsSidebarController', () => {
 
     // Add a new non-user triggered breakpoint and check if it's still collapsed.
     const b2 = await breakpointManager.setBreakpoint(
-        uiSourceCode, 0, 3, '', true, false, Bindings.BreakpointManager.BreakpointOrigin.OTHER);
+        uiSourceCode, 0, 3, Bindings.BreakpointManager.EMPTY_BREAKPOINT_CONDITION, true, false,
+        Bindings.BreakpointManager.BreakpointOrigin.OTHER);
     {
       const data = await controller.getUpdatedBreakpointViewData();
       assert.lengthOf(data.groups, 1);