From 18757715aee014431c5b628e0c9bb997a0fda8d4 Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Wed, 11 Oct 2023 10:46:20 +0000 Subject: [PATCH] Bug 1854423 - [devtools] Optimize and simplify getVisibleColumnBreakpoint selector. r=devtools-reviewers,bomsy * `filterByLineCount` was just a very costly non-sense filtering nothing. (comes from unreviewed code https://github.com/firefox-devtools/debugger/pull/7349) * The filter using lineText isn't clear either. Why would breakpoint position include breakpoint after the end of the actual line text content?? Differential Revision: https://phabricator.services.mozilla.com/D188891 --- .../visibleColumnBreakpoints.spec.js.snap | 6 +- .../test/visibleColumnBreakpoints.spec.js | 8 +- .../src/selectors/visibleColumnBreakpoints.js | 202 +++++++----------- 3 files changed, 90 insertions(+), 126 deletions(-) diff --git a/devtools/client/debugger/src/selectors/test/__snapshots__/visibleColumnBreakpoints.spec.js.snap b/devtools/client/debugger/src/selectors/test/__snapshots__/visibleColumnBreakpoints.spec.js.snap index 027a95e887fe..e4b18538cfdd 100644 --- a/devtools/client/debugger/src/selectors/test/__snapshots__/visibleColumnBreakpoints.spec.js.snap +++ b/devtools/client/debugger/src/selectors/test/__snapshots__/visibleColumnBreakpoints.spec.js.snap @@ -120,7 +120,7 @@ Array [ }, }, Object { - "breakpoint": null, + "breakpoint": undefined, "location": Object { "column": 3, "line": 1, @@ -189,7 +189,7 @@ Array [ }, }, Object { - "breakpoint": null, + "breakpoint": undefined, "location": Object { "column": 3, "line": 1, @@ -258,7 +258,7 @@ Array [ }, }, Object { - "breakpoint": null, + "breakpoint": undefined, "location": Object { "column": 5, "line": 1, diff --git a/devtools/client/debugger/src/selectors/test/visibleColumnBreakpoints.spec.js b/devtools/client/debugger/src/selectors/test/visibleColumnBreakpoints.spec.js index 276851b1ef0e..873bf35ae2c0 100644 --- a/devtools/client/debugger/src/selectors/test/visibleColumnBreakpoints.spec.js +++ b/devtools/client/debugger/src/selectors/test/visibleColumnBreakpoints.spec.js @@ -47,7 +47,7 @@ describe("visible column breakpoints", () => { start: { line: 1, column: 0 }, end: { line: 10, column: 10 }, }; - const pausePoints = [pp(1, 1), pp(1, 5), pp(3, 1)]; + const pausePoints = { 1: [pp(1, 1), pp(1, 5)], 3: [pp(3, 1)] }; const breakpoints = [bp(1, 1), bp(4, 0), bp(4, 3)]; const columnBps = getColumnBreakpoints( @@ -65,7 +65,7 @@ describe("visible column breakpoints", () => { start: { line: 1, column: 0 }, end: { line: 10, column: 10 }, }; - const pausePoints = [pp(1, 1), pp(1, 3), pp(2, 1)]; + const pausePoints = { 1: [pp(1, 1), pp(1, 3)], 2: [pp(2, 1)] }; const breakpoints = [bp(1, 1)]; const columnBps = getColumnBreakpoints( pausePoints, @@ -82,7 +82,7 @@ describe("visible column breakpoints", () => { start: { line: 1, column: 0 }, end: { line: 10, column: 10 }, }; - const pausePoints = [pp(1, 1), pp(1, 3), pp(20, 1)]; + const pausePoints = { 1: [pp(1, 1), pp(1, 3)], 20: [pp(20, 1)] }; const breakpoints = [bp(1, 1)]; const columnBps = getColumnBreakpoints( @@ -100,7 +100,7 @@ describe("visible column breakpoints", () => { start: { line: 1, column: 0 }, end: { line: 10, column: 10 }, }; - const pausePoints = [pp(1, 1), pp(1, 15), pp(20, 1)]; + const pausePoints = { 1: [pp(1, 1), pp(1, 15)], 20: [pp(20, 1)] }; const breakpoints = [bp(1, 1), bp(1, 15)]; const columnBps = getColumnBreakpoints( diff --git a/devtools/client/debugger/src/selectors/visibleColumnBreakpoints.js b/devtools/client/debugger/src/selectors/visibleColumnBreakpoints.js index a7f66f75f959..587dec1a06f1 100644 --- a/devtools/client/debugger/src/selectors/visibleColumnBreakpoints.js +++ b/devtools/client/debugger/src/selectors/visibleColumnBreakpoints.js @@ -25,102 +25,23 @@ function contains(location, range) { ); } -function groupBreakpoints(breakpoints, selectedSource) { - const breakpointsMap = {}; - if (!breakpoints) { - return breakpointsMap; - } - - for (const breakpoint of breakpoints) { - if (breakpoint.options.hidden) { - continue; - } - const location = getSelectedLocation(breakpoint, selectedSource); - const { line, column } = location; - - if (!breakpointsMap[line]) { - breakpointsMap[line] = {}; - } - - if (!breakpointsMap[line][column]) { - breakpointsMap[line][column] = []; - } - - breakpointsMap[line][column].push(breakpoint); - } - - return breakpointsMap; -} - -function findBreakpoint(location, breakpointMap) { - const { line, column } = location; - const breakpoints = breakpointMap[line]?.[column]; - - if (!breakpoints) { - return null; - } - return breakpoints[0]; -} - -function filterByLineCount(positions, selectedSource) { - const lineCount = {}; - - for (const breakpoint of positions) { - const { line } = getSelectedLocation(breakpoint, selectedSource); - if (!lineCount[line]) { - lineCount[line] = 0; - } - lineCount[line] = lineCount[line] + 1; - } - - return positions.filter( - breakpoint => - lineCount[getSelectedLocation(breakpoint, selectedSource).line] > 1 - ); -} - -function filterVisible(positions, selectedSource, viewport) { - return positions.filter(columnBreakpoint => { - const location = getSelectedLocation(columnBreakpoint, selectedSource); - return viewport && contains(location, viewport); - }); -} - -function filterByBreakpoints(positions, selectedSource, breakpointMap) { - return positions.filter(position => { - const location = getSelectedLocation(position, selectedSource); - return breakpointMap[location.line]; - }); -} - -// Filters out breakpoints to the right of the line. (bug 1552039) -function filterInLine(positions, selectedSource, selectedContent) { - return positions.filter(position => { - const location = getSelectedLocation(position, selectedSource); - const lineText = getLineText( - selectedSource.id, - selectedContent, - location.line - ); - - return lineText.length >= (location.column || 0); - }); -} - -function formatPositions(positions, selectedSource, breakpointMap) { - return positions.map(position => { - const location = getSelectedLocation(position, selectedSource); - return { - location, - breakpoint: findBreakpoint(location, breakpointMap), - }; - }); -} - function convertToList(breakpointPositions) { return [].concat(...Object.values(breakpointPositions)); } +/** + * Retrieve the list of column breakpoints to be displayed. + * This ignores lines without any breakpoint, but also lines with a single possible breakpoint. + * So that we only return breakpoints where there is at least two possible breakpoint on a given line. + * Also, this only consider lines currently visible in CodeMirror editor. + * + * This method returns an array whose elements are objects having two attributes: + * - breakpoint: A breakpoint object refering to a precise column location + * - location: The location object in an active source where the breakpoint location matched. + * This location may be the generated or original source based on the currently selected source type. + * + * See `visibleColumnBreakpoints()` for the definition of arguments. + */ export function getColumnBreakpoints( positions, breakpoints, @@ -128,40 +49,83 @@ export function getColumnBreakpoints( selectedSource, selectedSourceTextContent ) { - if (!positions || !selectedSource) { + if (!positions || !selectedSource || !breakpoints.length || !viewport) { return []; } - // We only want to show a column breakpoint if several conditions are matched - // - it is the first breakpoint to appear at an the original location - // - the position is in the current viewport - // - there is atleast one other breakpoint on that line - // - there is a breakpoint on that line - const breakpointMap = groupBreakpoints(breakpoints, selectedSource); - positions = filterByLineCount(positions, selectedSource); - positions = filterVisible(positions, selectedSource, viewport); - positions = filterInLine( - positions, - selectedSource, - selectedSourceTextContent - ); - positions = filterByBreakpoints(positions, selectedSource, breakpointMap); + const breakpointsPerLine = new Map(); + for (const breakpoint of breakpoints) { + if (breakpoint.options.hidden) { + continue; + } + const location = getSelectedLocation(breakpoint, selectedSource); + const { line } = location; + let breakpointsPerColumn = breakpointsPerLine.get(line); + if (!breakpointsPerColumn) { + breakpointsPerColumn = new Map(); + breakpointsPerLine.set(line, breakpointsPerColumn); + } + breakpointsPerColumn.set(location.column, breakpoint); + } - return formatPositions(positions, selectedSource, breakpointMap); + const columnBreakpoints = []; + for (const keyLine in positions) { + const positionsPerLine = positions[keyLine]; + // Only consider positions where there is more than one breakable position per line. + // When there is only one breakpoint, this isn't a column breakpoint. + if (positionsPerLine.length <= 1) { + continue; + } + for (const breakpointPosition of positionsPerLine) { + const location = getSelectedLocation(breakpointPosition, selectedSource); + const { line } = location; + + // Ignore any further computation if there is no breakpoint on that line. + const breakpointsPerColumn = breakpointsPerLine.get(line); + if (!breakpointsPerColumn) { + continue; + } + + // Only consider positions visible in the current CodeMirror viewport + if (!contains(location, viewport)) { + continue; + } + + // Filters out breakpoints to the right of the line. (bug 1552039) + // XXX Not really clear why we get such positions?? + const { column } = location; + if (column) { + const lineText = getLineText( + selectedSource.id, + selectedSourceTextContent, + line + ); + if (column > lineText.length) { + continue; + } + } + + // Finally, return the expected format output for this selector. + // Location of each column breakpoint + a reference to the breakpoint object (if one is set on that column, it can be null). + const breakpoint = breakpointsPerColumn.get(column); + + columnBreakpoints.push({ + location, + breakpoint, + }); + } + } + + return columnBreakpoints; } -const getVisibleBreakpointPositions = createSelector( - state => { - const source = getSelectedSource(state); - if (!source) { - return null; - } - return getBreakpointPositionsForSource(state, source.id); - }, - sourcePositions => { - return convertToList(sourcePositions || []); +function getVisibleBreakpointPositions(state) { + const source = getSelectedSource(state); + if (!source) { + return null; } -); + return getBreakpointPositionsForSource(state, source.id); +} export const visibleColumnBreakpoints = createSelector( getVisibleBreakpointPositions,