Reland "Show unbound breakpoints as disabled"

This is a reland of ca63eb417e

Original change's description:
> Show unbound breakpoints as disabled
>
> We need the UI to distinguish between a successfully bound breakpoint
> and one that has not been bound. This change makes unbound breakpoints
> look like disabled breakpoints, though that could be changed to a
> different icon in the future.
>
> This change also eliminates a race condition in some tests by waiting
> for a breakpoint to be bound after setting it.
>
> Fixed: chromium:1063864, chromium:1064581
> Change-Id: Id181a19efd7f9939a1555368b9367aaa22af0fd2
> Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2128131
> Commit-Queue: Eric Leese <leese@chromium.org>
> Reviewed-by: Simon Zünd <szuend@chromium.org>
> Reviewed-by: Tim van der Lippe <tvanderlippe@chromium.org>

Change-Id: I02c4ce21a617f797862fb8f5ec996ef2e45c20ca
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2132227
Reviewed-by: Tim van der Lippe <tvanderlippe@chromium.org>
Commit-Queue: Eric Leese <leese@chromium.org>

front_end/bindings/BreakpointManager.js

@@ -395,14 +395,14 @@ export class Breakpoint {
   addUISourceCode(uiSourceCode) {
     if (!this._uiSourceCodes.has(uiSourceCode)) {
       this._uiSourceCodes.add(uiSourceCode);
-      if (!this._isBound()) {
+      if (!this.bound()) {
         this._breakpointManager._uiLocationAdded(this, this._defaultUILocation(uiSourceCode));
       }
     }
   }
 
   clearUISourceCodes() {
-    if (!this._isBound()) {
+    if (!this.bound()) {
       this._removeAllUnboundLocations();
     }
     this._uiSourceCodes.clear();
@@ -414,13 +414,13 @@ export class Breakpoint {
   removeUISourceCode(uiSourceCode) {
     if (this._uiSourceCodes.has(uiSourceCode)) {
       this._uiSourceCodes.delete(uiSourceCode);
-      if (!this._isBound()) {
+      if (!this.bound()) {
         this._breakpointManager._uiLocationRemoved(this, this._defaultUILocation(uiSourceCode));
       }
     }
 
     // Do we need to do this? Not sure if bound locations will leak...
-    if (this._isBound()) {
+    if (this.bound()) {
       for (const uiLocation of this._uiLocations) {
         if (uiLocation.uiSourceCode === uiSourceCode) {
           this._uiLocations.delete(uiLocation);
@@ -428,7 +428,7 @@ export class Breakpoint {
         }
       }
 
-      if (!this._isBound() && !this._isRemoved) {
+      if (!this.bound() && !this._isRemoved) {
         // Switch to unbound locations
         this._addAllUnboundLocations();
       }
@@ -463,7 +463,7 @@ export class Breakpoint {
     if (this._isRemoved) {
       return;
     }
-    if (!this._isBound()) {
+    if (!this.bound()) {
       // This is our first bound location; remove all unbound locations
       this._removeAllUnboundLocations();
     }
@@ -478,7 +478,7 @@ export class Breakpoint {
     if (this._uiLocations.has(uiLocation)) {
       this._uiLocations.delete(uiLocation);
       this._breakpointManager._uiLocationRemoved(this, uiLocation);
-      if (!this._isBound() && !this._isRemoved) {
+      if (!this.bound() && !this._isRemoved) {
         this._addAllUnboundLocations();
       }
     }
@@ -491,6 +491,13 @@ export class Breakpoint {
     return this._enabled;
   }
 
+  /**
+   * @return {boolean}
+   */
+  bound() {
+    return this._uiLocations.size !== 0;
+  }
+
   /**
    * @param {boolean} enabled
    */
@@ -527,7 +534,7 @@ export class Breakpoint {
   }
 
   _updateBreakpoint() {
-    if (!this._isBound()) {
+    if (!this.bound()) {
       this._removeAllUnboundLocations();
       if (!this._isRemoved) {
         this._addAllUnboundLocations();
@@ -568,13 +575,6 @@ export class Breakpoint {
     }
   }
 
-  /**
-   * @return {boolean}
-   */
-  _isBound() {
-    return this._uiLocations.size !== 0;
-  }
-
   /**
    * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
    * @return {!Workspace.UISourceCode.UILocation}

front_end/sources/DebuggerPlugin.js

@@ -1281,6 +1281,7 @@ export class DebuggerPlugin extends Plugin {
     function updateGutter(editorLineNumber, decorations) {
       this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint', false);
       this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-disabled', false);
+      this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-unbound', false);
       this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-conditional', false);
       this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-logpoint', false);
 
@@ -1288,10 +1289,12 @@ export class DebuggerPlugin extends Plugin {
         decorations.sort(BreakpointDecoration.mostSpecificFirst);
         const isDisabled = !decorations[0].enabled || this._muted;
         const isLogpoint = decorations[0].condition.includes(LogpointPrefix);
+        const isUnbound = !decorations[0].bound;
         const isConditionalBreakpoint = !!decorations[0].condition && !isLogpoint;
 
         this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint', true);
         this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-disabled', isDisabled);
+        this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-unbound', isUnbound && !isDisabled);
         this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-logpoint', isLogpoint);
         this._textEditor.toggleLineClass(editorLineNumber, 'cm-breakpoint-conditional', isConditionalBreakpoint);
       }
@@ -1435,8 +1438,8 @@ export class DebuggerPlugin extends Plugin {
       decoration.enabled = breakpoint.enabled();
     } else {
       const handle = this._textEditor.textEditorPositionHandle(editorLocation[0], editorLocation[1]);
-      decoration =
-          new BreakpointDecoration(this._textEditor, handle, breakpoint.condition(), breakpoint.enabled(), breakpoint);
+      decoration = new BreakpointDecoration(
+          this._textEditor, handle, breakpoint.condition(), breakpoint.enabled(), breakpoint.bound(), breakpoint);
       decoration.element.addEventListener('click', this._inlineBreakpointClick.bind(this, decoration), true);
       decoration.element.addEventListener(
           'contextmenu', this._inlineBreakpointContextMenu.bind(this, decoration), true);
@@ -1488,7 +1491,8 @@ export class DebuggerPlugin extends Plugin {
           continue;
         }
         const handle = this._textEditor.textEditorPositionHandle(editorLocation[0], editorLocation[1]);
-        const decoration = new BreakpointDecoration(this._textEditor, handle, '', false, null);
+        const decoration = new BreakpointDecoration(
+            this._textEditor, handle, '', /** enabled */ false, /** bound */ false, /** breakpoint */ null);
         decoration.element.addEventListener('click', this._inlineBreakpointClick.bind(this, decoration), true);
         decoration.element.addEventListener(
             'contextmenu', this._inlineBreakpointContextMenu.bind(this, decoration), true);
@@ -1834,13 +1838,15 @@ export class BreakpointDecoration {
    * @param {!TextEditor.CodeMirrorTextEditor.TextEditorPositionHandle} handle
    * @param {string} condition
    * @param {boolean} enabled
+   * @param {boolean} bound
    * @param {?Bindings.BreakpointManager.Breakpoint} breakpoint
    */
-  constructor(textEditor, handle, condition, enabled, breakpoint) {
+  constructor(textEditor, handle, condition, enabled, bound, breakpoint) {
     this._textEditor = textEditor;
     this.handle = handle;
     this.condition = condition;
     this.enabled = enabled;
+    this.bound = bound;
     this.breakpoint = breakpoint;
     this.element = createElement('span');
     this.element.classList.toggle('cm-inline-breakpoint', true);
@@ -1858,6 +1864,9 @@ export class BreakpointDecoration {
     if (decoration1.enabled !== decoration2.enabled) {
       return decoration1.enabled ? -1 : 1;
     }
+    if (decoration1.bound !== decoration2.bound) {
+      return decoration1.bound ? -1 : 1;
+    }
     if (!!decoration1.condition !== !!decoration2.condition) {
       return !!decoration1.condition ? -1 : 1;
     }
@@ -1898,6 +1907,7 @@ export class BreakpointDecoration {
     if (location) {
       this._textEditor.toggleLineClass(location.lineNumber, 'cm-breakpoint', false);
       this._textEditor.toggleLineClass(location.lineNumber, 'cm-breakpoint-disabled', false);
+      this._textEditor.toggleLineClass(location.lineNumber, 'cm-breakpoint-unbound', false);
       this._textEditor.toggleLineClass(location.lineNumber, 'cm-breakpoint-conditional', false);
       this._textEditor.toggleLineClass(location.lineNumber, 'cm-breakpoint-logpoint', false);
     }

front_end/text_editor/cmdevtools.css

@@ -182,34 +182,37 @@
     padding-top: 1px;
 }
 
-.breakpoint-element:before {
+.breakpoint-element::before {
     content: "\00AD";
 }
 
-.cm-breakpoint .breakpoint-element:before {
+.cm-breakpoint .breakpoint-element::before {
     content: url(Images/breakpoint.svg);
 }
 
-.breakpoints-deactivated .cm-breakpoint .breakpoint-element:before,
-.cm-breakpoint-disabled .breakpoint-element:before {
+.breakpoints-deactivated .cm-breakpoint .breakpoint-element::before,
+.cm-breakpoint-unbound .breakpoint-element::before,
+.cm-breakpoint-disabled .breakpoint-element::before {
     content: url(Images/breakpoint-disabled.svg);
 }
 
-.cm-breakpoint.cm-breakpoint-conditional .breakpoint-element:before {
+.cm-breakpoint.cm-breakpoint-conditional .breakpoint-element::before {
     content: url(Images/breakpoint-conditional.svg);
 }
 
-.cm-breakpoint-disabled.cm-breakpoint-conditional .breakpoint-element:before,
-.breakpoints-deactivated .cm-breakpoint.cm-breakpoint-conditional .breakpoint-element:before {
+.cm-breakpoint-disabled.cm-breakpoint-conditional .breakpoint-element::before,
+.cm-breakpoint-unbound.cm-breakpoint-conditional .breakpoint-element::before,
+.breakpoints-deactivated .cm-breakpoint.cm-breakpoint-conditional .breakpoint-element::before {
     content: url(Images/breakpoint-conditional-disabled.svg);
 }
 
-.cm-breakpoint.cm-breakpoint-logpoint .breakpoint-element:before {
+.cm-breakpoint.cm-breakpoint-logpoint .breakpoint-element::before {
     content: url(Images/logpoint.svg);
 }
 
-.cm-breakpoint-disabled.cm-breakpoint-logpoint .breakpoint-element:before,
-.breakpoints-deactivated .cm-breakpoint.cm-breakpoint-logpoint .breakpoint-element:before {
+.cm-breakpoint-disabled.cm-breakpoint-logpoint .breakpoint-element::before,
+.cm-breakpoint-unbound.cm-breakpoint-logpoint .breakpoint-element::before,
+.breakpoints-deactivated .cm-breakpoint.cm-breakpoint-logpoint .breakpoint-element::before {
     content: url(Images/logpoint-disabled.svg);
 }
 
@@ -219,39 +222,39 @@
     cursor: pointer;
 }
 
-.cm-inline-breakpoint:before {
+.cm-inline-breakpoint::before {
     content: url(Images/breakpoint.svg);
 }
 
-.cm-inline-breakpoint.cm-inline-disabled:before {
+.cm-inline-breakpoint.cm-inline-disabled::before {
     content: url(Images/breakpoint-disabled.svg);
 }
 
-.breakpoints-deactivated .cm-inline-breakpoint:before {
+.breakpoints-deactivated .cm-inline-breakpoint::before {
     content: url(Images/breakpoint-disabled.svg);
 }
 
-.cm-inline-breakpoint.cm-inline-breakpoint-conditional:before {
+.cm-inline-breakpoint.cm-inline-breakpoint-conditional::before {
     content: url(Images/breakpoint-conditional.svg);
 }
 
-.cm-inline-breakpoint.cm-inline-breakpoint-conditional.cm-inline-disabled:before {
+.cm-inline-breakpoint.cm-inline-breakpoint-conditional.cm-inline-disabled::before {
     content: url(Images/breakpoint-conditional-disabled.svg);
 }
 
-.breakpoints-deactivated .cm-inline-breakpoint.cm-inline-breakpoint-conditional:before {
+.breakpoints-deactivated .cm-inline-breakpoint.cm-inline-breakpoint-conditional::before {
     content: url(Images/breakpoint-conditional-disabled.svg);
 }
 
-.cm-inline-breakpoint.cm-inline-logpoint:before {
+.cm-inline-breakpoint.cm-inline-logpoint::before {
     content: url(Images/logpoint.svg);
 }
 
-.cm-inline-breakpoint.cm-inline-logpoint.cm-inline-disabled:before {
+.cm-inline-breakpoint.cm-inline-logpoint.cm-inline-disabled::before {
     content: url(Images/logpoint-disabled.svg);
 }
 
-.breakpoints-deactivated .cm-inline-breakpoint.cm-inline-logpoint:before {
+.breakpoints-deactivated .cm-inline-breakpoint.cm-inline-logpoint::before {
     content: url(Images/logpoint-disabled.svg);
 }
 
@@ -323,7 +326,7 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     position: relative;
 }
 
-.cm-tab:before {
+.cm-tab::before {
     display: none;
     content: ".";
     color: transparent;
@@ -334,7 +337,7 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     left: 5%;
 }
 
-.show-whitespaces .CodeMirror .cm-tab:before {
+.show-whitespaces .CodeMirror .cm-tab::before {
     display: block !important;
 }
 
@@ -369,7 +372,7 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     position: relative;
 }
 
-.cm-token-highlight:before {
+.cm-token-highlight::before {
     position: absolute;
     border: 1px solid gray;
     border-radius: 3px;
@@ -380,7 +383,7 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     content: "";
 }
 
-.cm-line-with-selection .cm-column-with-selection:before {
+.cm-line-with-selection .cm-column-with-selection::before {
     border: none;
 }
 
@@ -388,7 +391,7 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     position: relative;
 }
 
-.cm-search-highlight:before {
+.cm-search-highlight::before {
     position: absolute;
     border-top-style: solid;
     border-bottom-style: solid;
@@ -403,12 +406,12 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     content: "";
 }
 
-.cm-search-highlight-full:before {
+.cm-search-highlight-full::before {
     border: 1px solid gray;
     border-radius: 3px;
 }
 
-.cm-search-highlight-start:before {
+.cm-search-highlight-start::before {
     border-left-width: 1px;
     border-top-left-radius: 2px;
     border-bottom-left-radius: 2px;
@@ -416,7 +419,7 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     border-left-color: gray;
 }
 
-.cm-search-highlight-end:before {
+.cm-search-highlight-end::before {
     border-right-width: 1px;
     border-top-right-radius: 2px;
     border-bottom-right-radius: 2px;
@@ -424,28 +427,28 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
     border-right-color: gray;
 }
 
-.cm-line-with-selection .cm-column-with-selection.cm-search-highlight-full:before {
+.cm-line-with-selection .cm-column-with-selection.cm-search-highlight-full::before {
     border-radius: 1px;
 }
 
-.cm-line-with-selection .cm-column-with-selection.cm-search-highlight-start:before {
+.cm-line-with-selection .cm-column-with-selection.cm-search-highlight-start::before {
     border-top-left-radius: 1px;
     border-bottom-left-radius: 1px;
 }
 
-.cm-line-with-selection .cm-column-with-selection.cm-search-highlight-end:before {
+.cm-line-with-selection .cm-column-with-selection.cm-search-highlight-end::before {
     border-top-right-radius: 1px;
     border-bottom-right-radius: 1px;
 }
 
-.cm-line-with-selection .cm-column-with-selection.cm-search-highlight:before {
+.cm-line-with-selection .cm-column-with-selection.cm-search-highlight::before {
     margin: -1px -1px -1px -1px;
     background-color: rgb(241, 234, 0);
     z-index: -1;
 }
 
-:host-context(.-theme-with-dark-background) .cm-line-with-selection .cm-column-with-selection.cm-search-highlight:before,
-.-theme-with-dark-background .cm-line-with-selection .cm-column-with-selection.cm-search-highlight:before {
+:host-context(.-theme-with-dark-background) .cm-line-with-selection .cm-column-with-selection.cm-search-highlight::before,
+.-theme-with-dark-background .cm-line-with-selection .cm-column-with-selection.cm-search-highlight::before {
     background-color: hsl(133, 100%, 30%);
 }
 
@@ -679,7 +682,7 @@ div.CodeMirror:focus-within span.CodeMirror-nonmatchingbracket {
 }
 
 @media (forced-colors: active) {
-    .cm-token-highlight:before {
+    .cm-token-highlight::before {
         forced-color-adjust: none;
         border-color: Highlight;
     }

test/e2e/helpers/sources-helpers.ts

@@ -82,11 +82,14 @@ export async function addBreakpointForLine(frontend: puppeteer.Page, index: numb
     };
   }, index);
 
+  const currentBreakpointCount = await frontend.$$eval('.cm-breakpoint', nodes => nodes.length);
+
   await frontend.mouse.click(breakpointLineNumber.x, breakpointLineNumber.y);
 
-  await frontend.waitForFunction(() => {
-    return document.querySelectorAll('.cm-breakpoint').length !== 0;
-  });
+  await frontend.waitForFunction(bpCount => {
+    return document.querySelectorAll('.cm-breakpoint').length > bpCount &&
+        document.querySelectorAll('.cm-breakpoint-unbound').length === 0;
+  }, undefined, currentBreakpointCount);
 }
 
 export async function getBreakpointDecorators(frontend: puppeteer.Page, disabledOnly = false) {
@@ -170,11 +173,21 @@ export function createSelectorsForWorkerFile(
   };
 }
 
+async function expandSourceTreeItem(selector: string) {
+  const sourceTreeItem = await waitFor(selector, undefined, 1000);
+  const isExpanded = await sourceTreeItem.asElement()!.evaluate(element => {
+    return element.getAttribute('aria-expanded') === 'true';
+  });
+  if (!isExpanded) {
+    await doubleClickSourceTreeItem(selector);
+  }
+}
+
 export async function expandFileTree(selectors: NestedFileSelector) {
-  await doubleClickSourceTreeItem(selectors.rootSelector);
-  await doubleClickSourceTreeItem(selectors.domainSelector);
-  await doubleClickSourceTreeItem(selectors.folderSelector);
-  return await waitFor(selectors.fileSelector);
+  await expandSourceTreeItem(selectors.rootSelector);
+  await expandSourceTreeItem(selectors.domainSelector);
+  await expandSourceTreeItem(selectors.folderSelector);
+  return await waitFor(selectors.fileSelector, undefined, 1000);
 }
 
 export async function openNestedWorkerFile(selectors: NestedFileSelector) {

test/e2e/sources/dwarf-cxx-language-plugin.ts

@@ -59,7 +59,7 @@ describe('The CXX DWARF Language Plugin', async () => {
   });
 
   // Resolve the location for a breakpoint.
-  it.skip('[http://crbug.com/1063864] resolve locations for breakpoints correctly', async () => {
+  it('resolves locations for breakpoints correctly', async () => {
     const {target, frontend} = getBrowserAndPages();
 
     await openFileInSourcesPanel(target, 'wasm/global_variable_with_dwarf.html');

test/e2e/sources/script-in-multiple-workers.ts

@@ -7,7 +7,6 @@ import {describe, it} from 'mocha';
 import * as puppeteer from 'puppeteer';
 
 import {click, getBrowserAndPages, resetPages, resourcesPath, waitFor} from '../../shared/helper.js';
-import {focusConsolePrompt, switchToTopExecutionContext, typeIntoConsole} from '../helpers/console-helpers.js';
 import {addBreakpointForLine, createSelectorsForWorkerFile, getBreakpointDecorators, getOpenSources, openNestedWorkerFile, PAUSE_BUTTON, RESUME_BUTTON} from '../helpers/sources-helpers.js';
 
 async function validateSourceTabs() {
@@ -43,7 +42,7 @@ describe('Multi-Workers', async () => {
       assert.deepEqual(await getBreakpointDecorators(frontend, true), [6]);
     }
 
-    it.skip(`loads scripts exactly once on reload ${withOrWithout}`, async () => {
+    it(`loads scripts exactly once on reload ${withOrWithout}`, async () => {
       const {target} = getBrowserAndPages();
 
       // Have the target load the page.
@@ -66,8 +65,8 @@ describe('Multi-Workers', async () => {
       await validateSourceTabs();
     });
 
-    it.skip(`[crbug.com/1064581] loads scripts exactly once on break ${withOrWithout}`, async () => {
-      const {target, frontend} = getBrowserAndPages();
+    it(`loads scripts exactly once on break ${withOrWithout}`, async () => {
+      const {target} = getBrowserAndPages();
 
       // Have the target load the page.
       await target.goto(targetPage);
@@ -76,11 +75,8 @@ describe('Multi-Workers', async () => {
 
       await validateNavigationTree();
 
-      // Open console tab
-      await click('#tab-console');
-
-      // Send message to a worker by evaluating in the console
-      await typeIntoConsole(frontend, 'workers[3].postMessage({});');
+      // Send message to a worker to trigger break
+      await target.evaluate('workers[3].postMessage({});');
 
       // Should automatically switch to sources tab.
 
@@ -95,11 +91,7 @@ describe('Multi-Workers', async () => {
       // Verify that we have resumed.
       await waitFor(PAUSE_BUTTON);
 
-      await click('#tab-console');
-      await switchToTopExecutionContext(frontend);
-      await focusConsolePrompt();
-      // Send message to a different worker
-      await typeIntoConsole(frontend, 'workers[7].postMessage({});');
+      await target.evaluate('workers[7].postMessage({});');
 
       // Validate that we are paused
       await waitFor(RESUME_BUTTON);
@@ -108,7 +100,7 @@ describe('Multi-Workers', async () => {
       await validateSourceTabs();
     });
 
-    it.skip(`[crbug.com/1064581] copies breakpoints between workers ${withOrWithout}`, async () => {
+    it(`copies breakpoints between workers ${withOrWithout}`, async () => {
       const {target, frontend} = getBrowserAndPages();
 
       // Have the target load the page.
@@ -156,6 +148,6 @@ describe('Multi-Workers', async () => {
 
       // Check breakpoints
       await validateBreakpoints(frontend);
-    });
+    }).timeout(10000);
   });
 });