Reland "Deduplicate tabs with same URL"

This is a reland of 559b867f57

Original change's description:
> Deduplicate tabs with same URL
> 
> Opening the same file in multiple workers results in only one copy
> open. Further deduplication work in the BreakpointManager results
> in disabled breakpoints being visible in all workers.
> 
> Bug: chromium:670180
> Change-Id: I57c7cf2aab46dcc2dc9d6622819df55bdd75d0a2
> Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2107522
> Commit-Queue: Eric Leese <leese@chromium.org>
> Reviewed-by: Simon Zünd <szuend@chromium.org>

Bug: chromium:670180
Change-Id: I48ef449a48c41413287bdd969510659e59c9663b
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2116436
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Eric Leese <leese@chromium.org>

front_end/bindings/BreakpointManager.js

@@ -63,6 +63,7 @@ export class BreakpointManager extends Common.ObjectWrapper.ObjectWrapper {
     this._breakpointByStorageId = new Map();
 
     this._workspace.addEventListener(Workspace.Workspace.Events.UISourceCodeAdded, this._uiSourceCodeAdded, this);
+    this._workspace.addEventListener(Workspace.Workspace.Events.UISourceCodeRemoved, this._uiSourceCodeRemoved, this);
   }
 
   /**
@@ -132,6 +133,15 @@ export class BreakpointManager extends Common.ObjectWrapper.ObjectWrapper {
     this._restoreBreakpoints(uiSourceCode);
   }
 
+  /**
+   * @param {!Common.EventTarget.EventTargetEvent} event
+   */
+  _uiSourceCodeRemoved(event) {
+    const uiSourceCode = /** @type {!Workspace.UISourceCode.UISourceCode} */ (event.data);
+    const breakpoints = this.breakpointLocationsForUISourceCode(uiSourceCode);
+    breakpoints.forEach(bp => bp.breakpoint.removeUISourceCode(uiSourceCode));
+  }
+
   /**
    * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
    * @param {number} lineNumber
@@ -164,7 +174,7 @@ export class BreakpointManager extends Common.ObjectWrapper.ObjectWrapper {
     let breakpoint = this._breakpointByStorageId.get(itemId);
     if (breakpoint) {
       breakpoint._updateState(condition, enabled);
-      breakpoint.setPrimaryUISourceCode(uiSourceCode);
+      breakpoint.addUISourceCode(uiSourceCode);
       breakpoint._updateBreakpoint();
       return breakpoint;
     }
@@ -334,20 +344,21 @@ export class Breakpoint {
     this._lineNumber = lineNumber;
     this._columnNumber = columnNumber;
 
-    /** @type {?Workspace.UISourceCode.UILocation} */
-    this._defaultUILocation = null;
     /** @type {!Set<!Workspace.UISourceCode.UILocation>} */
-    this._uiLocations = new Set();
+    this._uiLocations = new Set();  // Bound locations
+    /** @type {!Set<!Workspace.UISourceCode.UISourceCode>} */
+    this._uiSourceCodes = new Set();  // All known UISourceCodes with this url
 
     /** @type {string} */ this._condition;
     /** @type {boolean} */ this._enabled;
     /** @type {boolean} */ this._isRemoved;
 
     this._currentState = null;
+
     /** @type {!Map.<!SDK.DebuggerModel.DebuggerModel, !ModelBreakpoint>}*/
     this._modelBreakpoints = new Map();
     this._updateState(condition, enabled);
-    this.setPrimaryUISourceCode(primaryUISourceCode);
+    this.addUISourceCode(primaryUISourceCode);
     this._breakpointManager._targetManager.observeModels(SDK.DebuggerModel.DebuggerModel, this);
   }
 
@@ -379,19 +390,48 @@ export class Breakpoint {
   }
 
   /**
-   * @param {?Workspace.UISourceCode.UISourceCode} primaryUISourceCode
+   * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
    */
-  setPrimaryUISourceCode(primaryUISourceCode) {
-    if (this._uiLocations.size === 0 && this._defaultUILocation) {
-      this._breakpointManager._uiLocationRemoved(this, this._defaultUILocation);
+  addUISourceCode(uiSourceCode) {
+    if (!this._uiSourceCodes.has(uiSourceCode)) {
+      this._uiSourceCodes.add(uiSourceCode);
+      if (!this._isBound()) {
+        this._breakpointManager._uiLocationAdded(this, this._defaultUILocation(uiSourceCode));
+      }
     }
-    if (primaryUISourceCode) {
-      this._defaultUILocation = primaryUISourceCode.uiLocation(this._lineNumber, this._columnNumber);
-    } else {
-      this._defaultUILocation = null;
+  }
+
+  clearUISourceCodes() {
+    if (!this._isBound()) {
+      this._removeAllUnboundLocations();
     }
-    if (this._uiLocations.size === 0 && this._defaultUILocation && !this._isRemoved) {
-      this._breakpointManager._uiLocationAdded(this, this._defaultUILocation);
+    this._uiSourceCodes.clear();
+  }
+
+  /**
+   * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
+   */
+  removeUISourceCode(uiSourceCode) {
+    if (this._uiSourceCodes.has(uiSourceCode)) {
+      this._uiSourceCodes.delete(uiSourceCode);
+      if (!this._isBound()) {
+        this._breakpointManager._uiLocationRemoved(this, this._defaultUILocation(uiSourceCode));
+      }
+    }
+
+    // Do we need to do this? Not sure if bound locations will leak...
+    if (this._isBound()) {
+      for (const uiLocation of this._uiLocations) {
+        if (uiLocation.uiSourceCode === uiSourceCode) {
+          this._uiLocations.delete(uiLocation);
+          this._breakpointManager._uiLocationRemoved(this, uiLocation);
+        }
+      }
+
+      if (!this._isBound() && !this._isRemoved) {
+        // Switch to unbound locations
+        this._addAllUnboundLocations();
+      }
     }
   }
 
@@ -423,8 +463,9 @@ export class Breakpoint {
     if (this._isRemoved) {
       return;
     }
-    if (this._uiLocations.size === 0 && this._defaultUILocation) {
-      this._breakpointManager._uiLocationRemoved(this, this._defaultUILocation);
+    if (!this._isBound()) {
+      // This is our first bound location; remove all unbound locations
+      this._removeAllUnboundLocations();
     }
     this._uiLocations.add(uiLocation);
     this._breakpointManager._uiLocationAdded(this, uiLocation);
@@ -434,10 +475,12 @@ export class Breakpoint {
    * @param {!Workspace.UISourceCode.UILocation} uiLocation
    */
   _uiLocationRemoved(uiLocation) {
-    this._uiLocations.delete(uiLocation);
-    this._breakpointManager._uiLocationRemoved(this, uiLocation);
-    if (this._uiLocations.size === 0 && this._defaultUILocation && !this._isRemoved) {
-      this._breakpointManager._uiLocationAdded(this, this._defaultUILocation);
+    if (this._uiLocations.has(uiLocation)) {
+      this._uiLocations.delete(uiLocation);
+      this._breakpointManager._uiLocationRemoved(this, uiLocation);
+      if (!this._isBound() && !this._isRemoved) {
+        this._addAllUnboundLocations();
+      }
     }
   }
 
@@ -484,11 +527,11 @@ export class Breakpoint {
   }
 
   _updateBreakpoint() {
-    if (this._uiLocations.size === 0 && this._defaultUILocation) {
-      this._breakpointManager._uiLocationRemoved(this, this._defaultUILocation);
-    }
-    if (this._uiLocations.size === 0 && this._defaultUILocation && !this._isRemoved) {
-      this._breakpointManager._uiLocationAdded(this, this._defaultUILocation);
+    if (!this._isBound()) {
+      this._removeAllUnboundLocations();
+      if (!this._isRemoved) {
+        this._addAllUnboundLocations();
+      }
     }
     for (const modelBreakpoint of this._modelBreakpoints.values()) {
       modelBreakpoint._scheduleUpdateInDebugger();
@@ -508,7 +551,7 @@ export class Breakpoint {
 
     this._breakpointManager._removeBreakpoint(this, removeFromStorage);
     this._breakpointManager._targetManager.unobserveModels(SDK.DebuggerModel.DebuggerModel, this);
-    this.setPrimaryUISourceCode(null);
+    this.clearUISourceCodes();
   }
 
   /**
@@ -519,11 +562,38 @@ export class Breakpoint {
   }
 
   _resetLocations() {
-    this.setPrimaryUISourceCode(null);
+    this.clearUISourceCodes();
     for (const modelBreakpoint of this._modelBreakpoints.values()) {
       modelBreakpoint._resetLocations();
     }
   }
+
+  /**
+   * @return {boolean}
+   */
+  _isBound() {
+    return this._uiLocations.size !== 0;
+  }
+
+  /**
+   * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
+   * @return {!Workspace.UISourceCode.UILocation}
+   */
+  _defaultUILocation(uiSourceCode) {
+    return uiSourceCode.uiLocation(this._lineNumber, this._columnNumber);
+  }
+
+  _removeAllUnboundLocations() {
+    for (const uiSourceCode of this._uiSourceCodes) {
+      this._breakpointManager._uiLocationRemoved(this, this._defaultUILocation(uiSourceCode));
+    }
+  }
+
+  _addAllUnboundLocations() {
+    for (const uiSourceCode of this._uiSourceCodes) {
+      this._breakpointManager._uiLocationAdded(this, this._defaultUILocation(uiSourceCode));
+    }
+  }
 }
 
 /**
@@ -588,13 +658,13 @@ export class ModelBreakpoint {
    * @return {boolean}
    */
   _scriptDiverged() {
-    const uiLocation = this._breakpoint._defaultUILocation;
-    const uiSourceCode = uiLocation ? uiLocation.uiSourceCode : null;
-    if (!uiSourceCode) {
-      return false;
+    for (const uiSourceCode of this._breakpoint._uiSourceCodes) {
+      const scriptFile = this._debuggerWorkspaceBinding.scriptFile(uiSourceCode, this._debuggerModel);
+      if (scriptFile && scriptFile.hasDivergedFromVM()) {
+        return true;
+      }
     }
-    const scriptFile = this._debuggerWorkspaceBinding.scriptFile(uiSourceCode, this._debuggerModel);
-    return !!scriptFile && scriptFile.hasDivergedFromVM();
+    return false;
   }
 
   /**
@@ -608,17 +678,18 @@ export class ModelBreakpoint {
       return;
     }
 
-    const uiLocation = this._breakpoint._defaultUILocation;
-    const uiSourceCode = uiLocation ? uiLocation.uiSourceCode : null;
     const lineNumber = this._breakpoint._lineNumber;
     const columnNumber = this._breakpoint._columnNumber;
     const condition = this._breakpoint.condition();
 
     let debuggerLocation = null;
-    if (uiSourceCode) {
+    for (const uiSourceCode of this._breakpoint._uiSourceCodes) {
       const locations =
           await DebuggerWorkspaceBinding.instance().uiLocationToRawLocations(uiSourceCode, lineNumber, columnNumber);
       debuggerLocation = locations.find(location => location.debuggerModel === this._debuggerModel);
+      if (debuggerLocation) {
+        break;
+      }
     }
     let newState;
     if (this._breakpoint._isRemoved || !this._breakpoint.enabled() || this._scriptDiverged()) {
@@ -635,8 +706,8 @@ export class ModelBreakpoint {
     } else if (this._breakpoint._currentState && this._breakpoint._currentState.url) {
       const position = this._breakpoint._currentState;
       newState = new Breakpoint.State(position.url, null, null, position.lineNumber, position.columnNumber, condition);
-    } else if (uiSourceCode) {
-      newState = new Breakpoint.State(uiSourceCode.url(), null, null, lineNumber, columnNumber, condition);
+    } else if (this._breakpoint._uiSourceCodes.size > 0) {  // Uncertain if this condition is necessary
+      newState = new Breakpoint.State(this._breakpoint.url(), null, null, lineNumber, columnNumber, condition);
     }
     if (this._debuggerId && Breakpoint.State.equals(newState, this._currentState)) {
       callback();
@@ -734,17 +805,11 @@ export class ModelBreakpoint {
    */
   async _locationUpdated(liveLocation) {
     const oldUILocation = this._uiLocations.get(liveLocation);
+    const uiLocation = await liveLocation.uiLocation();
+
     if (oldUILocation) {
       this._breakpoint._uiLocationRemoved(oldUILocation);
     }
-    let uiLocation = await liveLocation.uiLocation();
-
-    if (uiLocation) {
-      const breakpointLocation = this._breakpoint._breakpointManager.findBreakpoint(uiLocation);
-      if (breakpointLocation && breakpointLocation.uiLocation !== breakpointLocation.breakpoint._defaultUILocation) {
-        uiLocation = null;
-      }
-    }
 
     if (uiLocation) {
       this._uiLocations.set(liveLocation, uiLocation);

front_end/bindings/DebuggerWorkspaceBinding.js

@@ -192,7 +192,7 @@ export class DebuggerWorkspaceBinding {
       }
     }
     const modelData = this._debuggerModelToData.get(rawLocation.debuggerModel);
-    return modelData._rawLocationToUILocation(rawLocation);
+    return modelData ? modelData._rawLocationToUILocation(rawLocation) : null;
   }
 
   /**

front_end/sources/TabbedEditorContainer.js

@@ -91,7 +91,7 @@ export class TabbedEditorContainer extends Common.ObjectWrapper.ObjectWrapper {
 
     this._previouslyViewedFilesSetting = setting;
     this._history = History.fromObject(this._previouslyViewedFilesSetting.get());
-    this._historyUriToUISourceCode = new Map();
+    this._uriToUISourceCode = new Map();
   }
 
   /**
@@ -188,7 +188,7 @@ export class TabbedEditorContainer extends Common.ObjectWrapper.ObjectWrapper {
    * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
    */
   showFile(uiSourceCode) {
-    this._innerShowFile(uiSourceCode, true);
+    this._innerShowFile(this._canonicalUISourceCode(uiSourceCode), true);
   }
 
   /**
@@ -213,7 +213,7 @@ export class TabbedEditorContainer extends Common.ObjectWrapper.ObjectWrapper {
     const result = [];
     const uris = this._history._urls();
     for (const uri of uris) {
-      const uiSourceCode = this._historyUriToUISourceCode.get(uri);
+      const uiSourceCode = this._uriToUISourceCode.get(uri);
       if (uiSourceCode) {
         result.push(uiSourceCode);
       }
@@ -377,12 +377,33 @@ export class TabbedEditorContainer extends Common.ObjectWrapper.ObjectWrapper {
     }
   }
 
+  /**
+   * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
+   * @return {!Workspace.UISourceCode.UISourceCode}
+   */
+  _canonicalUISourceCode(uiSourceCode) {
+    // Check if we have already a UISourceCode for this url
+    if (this._uriToUISourceCode.has(uiSourceCode.url())) {
+      // Ignore incoming uiSourceCode, we already have this file.
+      return this._uriToUISourceCode.get(uiSourceCode.url());
+    }
+    this._uriToUISourceCode.set(uiSourceCode.url(), uiSourceCode);
+    return uiSourceCode;
+  }
+
   /**
    * @param {!Workspace.UISourceCode.UISourceCode} uiSourceCode
    */
   addUISourceCode(uiSourceCode) {
-    const binding = self.Persistence.persistence.binding(uiSourceCode);
-    uiSourceCode = binding ? binding.fileSystem : uiSourceCode;
+    const canonicalSourceCode = this._canonicalUISourceCode(uiSourceCode);
+    const duplicated = canonicalSourceCode !== uiSourceCode;
+    const binding = self.Persistence.persistence.binding(canonicalSourceCode);
+    uiSourceCode = binding ? binding.fileSystem : canonicalSourceCode;
+
+    if (duplicated) {
+      uiSourceCode.disableEdit();
+    }
+
     if (this._currentFile === uiSourceCode) {
       return;
     }
@@ -393,12 +414,6 @@ export class TabbedEditorContainer extends Common.ObjectWrapper.ObjectWrapper {
       return;
     }
 
-    // Check if we have already opened a tab for this uri....
-    if (this._historyUriToUISourceCode.has(uiSourceCode.url())) {
-      return;
-    }
-    this._historyUriToUISourceCode.set(uiSourceCode.url(), uiSourceCode);
-
     if (!this._tabIds.has(uiSourceCode)) {
       this._appendFileTab(uiSourceCode, false);
     }
@@ -437,8 +452,8 @@ export class TabbedEditorContainer extends Common.ObjectWrapper.ObjectWrapper {
       if (tabId) {
         tabIds.push(tabId);
       }
-      if (this._historyUriToUISourceCode.get(uiSourceCode.url()) === uiSourceCode) {
-        this._historyUriToUISourceCode.delete(uiSourceCode.url());
+      if (this._uriToUISourceCode.get(uiSourceCode.url()) === uiSourceCode) {
+        this._uriToUISourceCode.delete(uiSourceCode.url());
       }
     }
     this._tabbedPane.closeTabs(tabIds);

front_end/sources/UISourceCodeFrame.js

@@ -242,6 +242,9 @@ export class UISourceCodeFrame extends SourceFrame.SourceFrame.SourceFrameImpl {
     if (this.hasLoadError()) {
       return false;
     }
+    if (this._uiSourceCode.editDisabled()) {
+      return false;
+    }
     if (self.Persistence.persistence.binding(this._uiSourceCode)) {
       return true;
     }

front_end/workspace/UISourceCode.js

@@ -83,6 +83,7 @@ export class UISourceCode extends Common.ObjectWrapper.ObjectWrapper {
     this._workingCopy = null;
     /** @type {?function() : string} */
     this._workingCopyGetter = null;
+    this._disableEdit = false;
   }
 
   /**
@@ -628,6 +629,17 @@ export class UISourceCode extends Common.ObjectWrapper.ObjectWrapper {
   decorationsForType(type) {
     return this._decorations ? this._decorations.get(type) : null;
   }
+
+  disableEdit() {
+    this._disableEdit = true;
+  }
+
+  /**
+   * @return {boolean}
+   */
+  editDisabled() {
+    return this._disableEdit;
+  }
 }
 
 /** @enum {symbol} */

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

@@ -15,18 +15,6 @@ async function validateSourceTabs() {
   assert.deepEqual(await getOpenSources(), ['multi-workers.js']);
 }
 
-async function validateBreakpoints(frontend: puppeteer.Page) {
-  assert.deepEqual(await getBreakpointDecorators(frontend), [6, 10]);
-  assert.deepEqual(await getBreakpointDecorators(frontend, true), [6]);
-}
-
-async function validateBreakpointsWithoutDisabled(frontend: puppeteer.Page) {
-  // Currently breakpoints do not get copied to workers if they are disabled.
-  // This behavior is enforced by a web test, which this test will replace.
-  // TODO(leese): Once breakpoint copying issue is fixed, remove this function.
-  assert.deepEqual(await getBreakpointDecorators(frontend), [10]);
-  assert.deepEqual(await getBreakpointDecorators(frontend, true), []);
-}
 
 describe('Multi-Workers', async () => {
   beforeEach(async () => {
@@ -48,6 +36,13 @@ describe('Multi-Workers', async () => {
       await waitFor(workerFileSelectors(10).rootSelector);
     }
 
+    async function validateBreakpoints(frontend: puppeteer.Page) {
+      // TODO(crbug.com/1062308): Fix the source map so the breakpoint at 10 doesn't move
+      const bpLine = sourceMaps ? 9 : 10;
+      assert.deepEqual(await getBreakpointDecorators(frontend), [6, bpLine]);
+      assert.deepEqual(await getBreakpointDecorators(frontend, true), [6]);
+    }
+
     it(`loads scripts exactly once on reload ${withOrWithout}`, async () => {
       const {target} = getBrowserAndPages();
 
@@ -71,8 +66,7 @@ describe('Multi-Workers', async () => {
       await validateSourceTabs();
     });
 
-    // TODO(leese): Enable once chromium:670180 is fixed.
-    it.skip(`[crbug.com/670180]loads scripts exactly once on break ${withOrWithout}`, async () => {
+    it(`loads scripts exactly once on break ${withOrWithout}`, async () => {
       const {target, frontend} = getBrowserAndPages();
 
       // Have the target load the page.
@@ -114,57 +108,54 @@ describe('Multi-Workers', async () => {
       await validateSourceTabs();
     });
 
-    // TODO(leese): Enable with source maps once chromium:670180 is fixed.
-    if (!sourceMaps) {
-      it(`copies breakpoints between workers ${withOrWithout}`, async () => {
-        const {target, frontend} = getBrowserAndPages();
+    it(`copies breakpoints between workers ${withOrWithout}`, async () => {
+      const {target, frontend} = getBrowserAndPages();
 
-        // Have the target load the page.
-        await target.goto(targetPage);
+      // Have the target load the page.
+      await target.goto(targetPage);
 
-        await click('#tab-sources');
-        // Wait for all workers to load
-        await validateNavigationTree();
-        // Open file from second worker
-        await openNestedWorkerFile(workerFileSelectors(2));
-        // Set two breakpoints
-        await addBreakpointForLine(frontend, 6);
-        // Disable first breakpoint
-        const bpEntry = await waitFor('.breakpoint-entry');
-        const bpCheckbox = await waitFor('input', bpEntry);
-        if (!bpCheckbox) {
-          assert.fail('Could not find checkbox to disable breakpoint');
-          return;
-        }
-        await bpCheckbox.evaluate(n => (n as HTMLElement).click());
-        await frontend.waitFor('.cm-breakpoint-disabled');
-        // Add another breakpoint
-        await addBreakpointForLine(frontend, 10);
+      await click('#tab-sources');
+      // Wait for all workers to load
+      await validateNavigationTree();
+      // Open file from second worker
+      await openNestedWorkerFile(workerFileSelectors(2));
+      // Set two breakpoints
+      await addBreakpointForLine(frontend, 6);
+      // Disable first breakpoint
+      const bpEntry = await waitFor('.breakpoint-entry');
+      const bpCheckbox = await waitFor('input', bpEntry);
+      if (!bpCheckbox) {
+        assert.fail('Could not find checkbox to disable breakpoint');
+        return;
+      }
+      await bpCheckbox.evaluate(n => (n as HTMLElement).click());
+      await frontend.waitFor('.cm-breakpoint-disabled');
+      // Add another breakpoint
+      await addBreakpointForLine(frontend, 10);
 
-        // Check breakpoints
-        await validateBreakpoints(frontend);
+      // Check breakpoints
+      await validateBreakpoints(frontend);
 
-        // Close tab
-        await click('[aria-label="Close multi-workers.js"]');
+      // Close tab
+      await click('[aria-label="Close multi-workers.js"]');
 
-        // Open different worker
-        await openNestedWorkerFile(workerFileSelectors(3));
+      // Open different worker
+      await openNestedWorkerFile(workerFileSelectors(3));
 
-        // Check breakpoints
-        await validateBreakpointsWithoutDisabled(frontend);
+      // Check breakpoints
+      await validateBreakpoints(frontend);
 
-        // Close tab
-        await click('[aria-label="Close multi-workers.js"]');
+      // Close tab
+      await click('[aria-label="Close multi-workers.js"]');
 
-        // Reload
-        await target.goto(targetPage);
+      // Reload
+      await target.goto(targetPage);
 
-        // Open different worker
-        await openNestedWorkerFile(workerFileSelectors(4));
+      // Open different worker
+      await openNestedWorkerFile(workerFileSelectors(4));
 
-        // Check breakpoints
-        await validateBreakpointsWithoutDisabled(frontend);
-      });
-    }
+      // Check breakpoints
+      await validateBreakpoints(frontend);
+    });
   });
 });