Bug 1770204 - [devtools] Simplify breakpoint list selectors r=nchevobbe

The selector is already sorting the breakpoints,
so there is no need to re-sort them in the component.

Also Array.sort(stringA-stringB) was misbehaving.

Then I'm using getSourcesMap in order to avoid calling getBreakpointList many times.
Otherwise getSourcesForBreakpoints has to receive the state object
in order to retrieve the source objects.

Differential Revision: https://phabricator.services.mozilla.com/D146426

devtools/client/debugger/src/actions/breakpoints/tests/__snapshots__/breakpoints.spec.js.snap

@@ -25,6 +25,7 @@ Array [
         "thread": "FakeThread",
       },
     ],
+    "filename": "a",
     "source": Object {
       "extensionName": null,
       "id": "a",
@@ -42,12 +43,8 @@ Array [
 ]
 `;
 
-exports[`breakpoints should not re-add a breakpoint 1`] = `Array []`;
-
 exports[`breakpoints should not show a breakpoint that does not have text 1`] = `Array []`;
 
-exports[`breakpoints should not show a breakpoint that does not have text 2`] = `Array []`;
-
 exports[`breakpoints should remap breakpoints on pretty print 1`] = `
 Object {
   "disabled": false,
@@ -96,6 +93,7 @@ Array [
         "thread": "FakeThread",
       },
     ],
+    "filename": "a",
     "source": Object {
       "extensionName": null,
       "id": "a",

devtools/client/debugger/src/components/SecondaryPanes/Breakpoints/index.js

@@ -15,10 +15,7 @@ import actions from "../../../actions";
 import { getSelectedLocation } from "../../../utils/selected-location";
 import { createHeadlessEditor } from "../../../utils/editor/create-editor";
 
-import {
-  makeBreakpointId,
-  sortSelectedBreakpoints,
-} from "../../../utils/breakpoint";
+import { makeBreakpointId } from "../../../utils/breakpoint";
 
 import { getSelectedSource, getBreakpointSources } from "../../../selectors";
 
@@ -88,23 +85,18 @@ class Breakpoints extends Component {
     }
 
     const editor = this.getEditor();
-    const sources = [...breakpointSources.map(({ source }) => source)];
+    const sources = breakpointSources.map(({ source }) => source);
 
     return (
       <div className="pane breakpoints-list">
         {breakpointSources.map(({ source, breakpoints }) => {
-          const sortedBreakpoints = sortSelectedBreakpoints(
-            breakpoints,
-            selectedSource
-          );
-
           return [
             <BreakpointHeading
               key={source.id}
               source={source}
               sources={sources}
             />,
-            ...sortedBreakpoints.map(breakpoint => (
+            breakpoints.map(breakpoint => (
               <Breakpoint
                 breakpoint={breakpoint}
                 source={source}

devtools/client/debugger/src/selectors/breakpointSources.js

@@ -3,55 +3,83 @@
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 import { createSelector } from "reselect";
-import { getSelectedSource, getSourceFromId } from "./sources";
+import { getSelectedSource, getSourcesMap } from "./sources";
 import { getBreakpointsList } from "./breakpoints";
 import { getFilename } from "../utils/source";
 import { getSelectedLocation } from "../utils/selected-location";
 import { sortSelectedBreakpoints } from "../utils/breakpoint";
 
-function getBreakpointsForSource(source, selectedSource, breakpoints) {
-  return sortSelectedBreakpoints(breakpoints, selectedSource)
-    .filter(
-      bp =>
-        !bp.options.hidden &&
-        (bp.text || bp.originalText || bp.options.condition || bp.disabled)
-    )
-    .filter(
-      bp => getSelectedLocation(bp, selectedSource).sourceId == source.id
-    );
+// Returns all the breakpoints for the given selected source
+// Depending on the selected source, this will match original or generated
+// location of the given selected source.
+function _getBreakpointsForSource(visibleBreakpoints, source, selectedSource) {
+  return visibleBreakpoints.filter(
+    bp => getSelectedLocation(bp, selectedSource).sourceId == source.id
+  );
 }
 
-const getSourcesForBreakpoints = state => {
-  const selectedSource = getSelectedSource(state);
-  const breakpointSourceIds = getBreakpointsList(state).map(
+// Returns a sorted list of sources for which we have breakpoints
+// We will return generated or original source IDs based on the currently selected source.
+const _getSourcesForBreakpoints = (breakpoints, sourcesMap, selectedSource) => {
+  const breakpointSourceIds = breakpoints.map(
     breakpoint => getSelectedLocation(breakpoint, selectedSource).sourceId
   );
 
-  return [...new Set(breakpointSourceIds)]
-    .map(sourceId => {
-      const source = getSourceFromId(state, sourceId);
-      const filename = getFilename(source);
-      return { source, filename };
-    })
-    .filter(({ source }) => source && !source.isBlackBoxed)
-    .sort((a, b) => a.filename - b.filename)
-    .map(({ source }) => source);
+  const sources = [];
+  // We may have more than one breakpoint per sourceId,
+  // so use a Set to have a unique list of source IDs.
+  for (const sourceId of [...new Set(breakpointSourceIds)]) {
+    const source = sourcesMap.get(sourceId);
+
+    // Ignore any source that is no longer in the sources reducer
+    // or blackboxed sources.
+    if (!source || source.isBlackBoxed) {
+      continue;
+    }
+
+    const bps = _getBreakpointsForSource(breakpoints, source, selectedSource);
+
+    // Ignore sources which have no breakpoints
+    if (bps.length === 0) {
+      continue;
+    }
+
+    sources.push({
+      source,
+      breakpoints: bps,
+      filename: getFilename(source),
+    });
+  }
+
+  return sources.sort((a, b) => a.filename.localeCompare(b.filename));
 };
 
+// Returns a list of sources with their related breakpoints:
+//   [{ source, breakpoints [breakpoint1, ...] }, ...]
+//
+// This only returns sources for which we have a visible breakpoint.
+// This will return either generated or original source based on the currently
+// selected source.
 export const getBreakpointSources = createSelector(
   getBreakpointsList,
-  getSourcesForBreakpoints,
+  getSourcesMap,
   getSelectedSource,
-  (breakpoints, sources, selectedSource) => {
-    return sources
-      .map(source => ({
-        source,
-        breakpoints: getBreakpointsForSource(
-          source,
-          selectedSource,
-          breakpoints
-        ),
-      }))
-      .filter(({ breakpoints: bps }) => bps.length > 0);
+  (breakpoints, sourcesMap, selectedSource) => {
+    const visibleBreakpoints = breakpoints.filter(
+      bp =>
+        !bp.options.hidden &&
+        (bp.text || bp.originalText || bp.options.condition || bp.disabled)
+    );
+
+    const sortedVisibleBreakpoints = sortSelectedBreakpoints(
+      visibleBreakpoints,
+      selectedSource
+    );
+
+    return _getSourcesForBreakpoints(
+      sortedVisibleBreakpoints,
+      sourcesMap,
+      selectedSource
+    );
   }
 );

devtools/client/debugger/src/selectors/sources.js

@@ -140,7 +140,7 @@ export function getHasSiblingOfSameName(state, source) {
   return getSourcesUrlsInSources(state, source.url).length > 1;
 }
 
-// This is only used externaly by tabs selectors
+// This is only used externaly by tabs and breakpointSources selectors
 export function getSourcesMap(state) {
   return state.sources.sources;
 }