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 521898 92d7192b7d69
parent 521835 71aaa0c1b7d8
child 521899 b3ac7a20c462
push id10870
push usernbeleuzu@mozilla.com
push dateFri, 15 Mar 2019 20:00:07 +0000
treeherdermozilla-beta@c594aee5b7a4 [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
-  );
-}