Bug 1534786 - Show pending breakpoints as soon as their generated source appears, r=jlast.
☠☠ backed out by b3ac7a20c462 ☠ ☠
authorBrian Hackett <bhackett1024@gmail.com>
Wed, 13 Mar 2019 07:19:12 -1000
changeset 524896 92d7192b7d69054565010b2ba41dc667d01fc4cb
parent 524833 71aaa0c1b7d8e8520e1b221c55bc86931efab31b
child 524897 b3ac7a20c462f5bf671f9579db6a124131a871ff
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlast
bugs1534786
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1534786 - Show pending breakpoints as soon as their generated source appears, r=jlast.
devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js
devtools/client/debugger/new/src/actions/breakpoints/syncBreakpoint.js
devtools/client/debugger/new/src/actions/sources/newSources.js
devtools/client/debugger/new/src/reducers/pending-breakpoints.js
devtools/client/debugger/new/src/selectors/visibleColumnBreakpoints.js
--- a/devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js
+++ b/devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js
@@ -11,17 +11,17 @@ import {
   getASTLocation,
   makeBreakpointId,
   makeBreakpointLocation,
   findPosition
 } from "../../utils/breakpoint";
 import { PROMISE } from "../utils/middleware/promise";
 import {
   getSymbols,
-  getFirstVisibleBreakpointPosition,
+  getFirstBreakpointPosition,
   getBreakpointPositionsForSource,
   getSourceFromId
 } from "../../selectors";
 
 import { getTextAtPosition } from "../../utils/source";
 import { recordEvent } from "../../utils/telemetry";
 
 import type {
@@ -100,17 +100,17 @@ export function addBreakpoint(
   options: BreakpointOptions = {}
 ) {
   return async ({ dispatch, getState, sourceMaps, client }: ThunkArgs) => {
     recordEvent("add_breakpoint");
     let position;
     const { sourceId, column } = location;
 
     if (column === undefined) {
-      position = getFirstVisibleBreakpointPosition(getState(), location);
+      position = getFirstBreakpointPosition(getState(), location);
     } else {
       const positions = getBreakpointPositionsForSource(getState(), sourceId);
       position = findPosition(positions, location);
     }
 
     if (!position) {
       return;
     }
--- a/devtools/client/debugger/new/src/actions/breakpoints/syncBreakpoint.js
+++ b/devtools/client/debugger/new/src/actions/breakpoints/syncBreakpoint.js
@@ -13,17 +13,18 @@ import {
   findPosition,
   makeBreakpointLocation
 } from "../../utils/breakpoint";
 
 import { getTextAtPosition } from "../../utils/source";
 import { comparePosition } from "../../utils/location";
 
 import { originalToGeneratedId, isOriginalId } from "devtools-source-map";
-import { getSource } from "../../selectors";
+import { getSource, getBreakpointsList } from "../../selectors";
+import { removeBreakpoint } from ".";
 
 import type { ThunkArgs, Action } from "../types";
 
 import type {
   SourceLocation,
   ASTLocation,
   PendingBreakpoint,
   SourceId,
@@ -82,24 +83,37 @@ function createSyncData(
     { generatedLocation, location },
     overrides
   );
 
   assertBreakpoint(breakpoint);
   return { breakpoint, previousLocation };
 }
 
+// Look for an existing breakpoint at the specified generated location.
+function findExistingBreakpoint(state, generatedLocation) {
+  const breakpoints = getBreakpointsList(state);
+
+  return breakpoints.find(bp => {
+    return (
+      bp.generatedLocation.sourceUrl == generatedLocation.sourceUrl &&
+      bp.generatedLocation.line == generatedLocation.line &&
+      bp.generatedLocation.column == generatedLocation.column
+    );
+  });
+}
+
 // we have three forms of syncing: disabled syncing, existing server syncing
 // and adding a new breakpoint
 export async function syncBreakpointPromise(
   thunkArgs: ThunkArgs,
   sourceId: SourceId,
   pendingBreakpoint: PendingBreakpoint
 ): Promise<?BreakpointSyncData> {
-  const { getState, client } = thunkArgs;
+  const { getState, client, dispatch } = thunkArgs;
   assertPendingBreakpoint(pendingBreakpoint);
 
   const source = getSource(getState(), sourceId);
 
   const generatedSourceId = isOriginalId(sourceId)
     ? originalToGeneratedId(sourceId)
     : sourceId;
 
@@ -123,16 +137,22 @@ export async function syncBreakpointProm
     newLocation
   );
 
   const isSameLocation = comparePosition(
     generatedLocation,
     newGeneratedLocation
   );
 
+  // Clear any breakpoint for the generated location.
+  const bp = findExistingBreakpoint(getState(), generatedLocation);
+  if (bp) {
+    await dispatch(removeBreakpoint(bp));
+  }
+
   /** ******* CASE 1: No server change ***********/
   // early return if breakpoint is disabled or we are in the sameLocation
   if (newGeneratedLocation && (pendingBreakpoint.disabled || isSameLocation)) {
     // Make sure the breakpoint is installed on all source actors.
     if (!pendingBreakpoint.disabled) {
       await client.setBreakpoint(
         makeBreakpointLocation(getState(), newGeneratedLocation),
         pendingBreakpoint.options
@@ -147,19 +167,16 @@ export async function syncBreakpointProm
       newLocation,
       newGeneratedLocation,
       previousLocation,
       text,
       originalText
     );
   }
 
-  // clear server breakpoints if they exist and we have moved
-  await client.removeBreakpoint(generatedLocation);
-
   if (!newGeneratedLocation) {
     return { previousLocation, breakpoint: null };
   }
 
   /** ******* Case 2: Add New Breakpoint ***********/
   // If we are not disabled, set the breakpoint on the server and get
   // that info so we can set it on our breakpoints.
   await client.setBreakpoint(
--- a/devtools/client/debugger/new/src/actions/sources/newSources.js
+++ b/devtools/client/debugger/new/src/actions/sources/newSources.js
@@ -8,17 +8,17 @@
  * Redux actions for the sources state
  * @module actions/sources
  */
 
 import { generatedToOriginalId } from "devtools-source-map";
 import { flatten } from "lodash";
 
 import { toggleBlackBox } from "./blackbox";
-import { syncBreakpoint } from "../breakpoints";
+import { syncBreakpoint, addBreakpoint } from "../breakpoints";
 import { loadSourceText } from "./loadSourceText";
 import { togglePrettyPrint } from "./prettyPrint";
 import { selectLocation } from "../sources";
 import { getRawSourceURL, isPrettyURL, isOriginal } from "../../utils/source";
 import {
   getBlackBoxList,
   getSource,
   getPendingSelectedLocation,
@@ -181,18 +181,29 @@ function checkPendingBreakpoints(sourceI
 
     if (pendingBreakpoints.length === 0) {
       return;
     }
 
     // load the source text if there is a pending breakpoint for it
     await dispatch(loadSourceText(source));
 
+    // Matching pending breakpoints could have either the same generated or the
+    // same original source. We expect the generated source to appear first and
+    // will add a breakpoint at that location initially. If the original source
+    // appears later then we use syncBreakpoint to see if the generated location
+    // changed and we need to remove the breakpoint we added earlier.
     await Promise.all(
-      pendingBreakpoints.map(bp => dispatch(syncBreakpoint(sourceId, bp)))
+      pendingBreakpoints.map(bp => {
+        if (source.url == bp.location.sourceUrl) {
+          return dispatch(syncBreakpoint(sourceId, bp));
+        }
+        const { line, column } = bp.generatedLocation;
+        return dispatch(addBreakpoint({ sourceId, line, column }, bp.options));
+      })
     );
   };
 }
 
 function restoreBlackBoxedSources(sources: Source[]) {
   return async ({ dispatch }: ThunkArgs) => {
     const tabs = getBlackBoxList();
     if (tabs.length == 0) {
--- a/devtools/client/debugger/new/src/reducers/pending-breakpoints.js
+++ b/devtools/client/debugger/new/src/reducers/pending-breakpoints.js
@@ -4,23 +4,20 @@
 
 // @flow
 
 /**
  * Pending breakpoints reducer
  * @module reducers/pending-breakpoints
  */
 
-import { getSourcesByURL } from "./sources";
-
 import {
   createPendingBreakpoint,
   makePendingLocationId
 } from "../utils/breakpoint";
-import { isGenerated } from "../utils/source";
 
 import type { SourcesState } from "./sources";
 import type { PendingBreakpoint, Source } from "../types";
 import type { Action, DonePromiseAction } from "../actions/types";
 
 export type PendingBreakpointsState = { [string]: PendingBreakpoint };
 
 function update(state: PendingBreakpointsState = {}, action: Action) {
@@ -150,20 +147,17 @@ export function getPendingBreakpointList
 ): PendingBreakpoint[] {
   return (Object.values(getPendingBreakpoints(state)): any);
 }
 
 export function getPendingBreakpointsForSource(
   state: OuterState,
   source: Source
 ): PendingBreakpoint[] {
-  const sources = getSourcesByURL(state, source.url);
-  if (sources.length > 1 && isGenerated(source)) {
-    // Don't return pending breakpoints for duplicated generated sources
-    return [];
-  }
-
-  return getPendingBreakpointList(state).filter(
-    pendingBreakpoint => pendingBreakpoint.location.sourceUrl === source.url
-  );
+  return getPendingBreakpointList(state).filter(pendingBreakpoint => {
+    return (
+      pendingBreakpoint.location.sourceUrl === source.url ||
+      pendingBreakpoint.generatedLocation.sourceUrl == source.url
+    );
+  });
 }
 
 export default update;
--- a/devtools/client/debugger/new/src/selectors/visibleColumnBreakpoints.js
+++ b/devtools/client/debugger/new/src/selectors/visibleColumnBreakpoints.js
@@ -168,24 +168,8 @@ export function getFirstBreakpointPositi
   if (!source || !positions) {
     return;
   }
 
   return positions.find(
     position => getSelectedLocation(position, source).line == line
   );
 }
-
-export function getFirstVisibleBreakpointPosition(
-  state: State,
-  { line }: SourceLocation
-) {
-  const positions = getVisibleBreakpointPositions(state);
-  const selectedSource = getSelectedSource(state);
-
-  if (!selectedSource || !positions) {
-    return;
-  }
-
-  return positions.find(
-    position => getSelectedLocation(position, selectedSource).line == line
-  );
-}