Refactor Breakpoints to be location symmetric #3294

jasonLaster · 2017-07-10T14:43:03Z

Summary of Changes

This is a follow up refactor. The goal is to provide some abstractions so that BPs are better represented in the components:

Components don't know/nor care if they're original or generated
Components only care about showing BPs
Toggle BP takes a line and column and does the rest
The reducer formats a generic BP with the "right" location

codehag · 2017-07-10T14:49:46Z

src/actions/breakpoints.js

+    const { sourceId } = selectedLocation;
+
+    if (bp) {
+      // NOTE: it's possible the breakpoint has slid to a column


you can use the equalize location util!

codehag · 2017-07-10T14:50:40Z

src/actions/breakpoints.js

+  };
+}
+
+export function toggleBreakpointDisabledStatus(line) {


i like this, might need a simpler name like "toggleDisabledBreakpoint"

codehag

nice direction!

codehag · 2017-07-10T14:51:05Z

src/actions/breakpoints.js

+
+    const { sourceId } = selectedLocation;
+
+    if (!bp) {


i wonder if we can avoid this somehow... 🤔

codehag · 2017-07-10T14:52:23Z

src/components/Editor/index.js

@@ -476,75 +475,6 @@ class Editor extends PureComponent {
    return !!this.cbPanel;
  }

-  toggleBreakpoint(line, column = undefined) {


👍 niiice

codehag · 2017-07-10T14:52:41Z

src/components/Editor/index.js

@@ -661,44 +591,6 @@ class Editor extends PureComponent {
    });
  }

-  renderBreakpoints() {


codecov · 2017-07-11T18:24:08Z

Codecov Report

Merging #3294 into master will increase coverage by 0.46%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #3294      +/-   ##
==========================================
+ Coverage   48.46%   48.92%   +0.46%     
==========================================
  Files         102      105       +3     
  Lines        4288     4319      +31     
  Branches      891      893       +2     
==========================================
+ Hits         2078     2113      +35     
+ Misses       2210     2206       -4

Impacted Files	Coverage Δ
src/selectors.js	`100% <ø> (ø)`	⬆️
src/components/Editor/CallSites.js	`2.12% <ø> (ø)`	⬆️
src/selectors/linesInScope.js	`93.75% <ø> (ø)`	⬆️
src/utils/editor/index.js	`14.63% <0%> (-0.76%)`	⬇️
src/components/Editor/GutterMenu.js	`0% <0%> (ø)`	⬆️
src/utils/editor/expression.js	`0% <0%> (ø)`	⬆️
src/selectors/visibleBreakpoints.js	`0% <0%> (ø)`
src/components/Editor/Breakpoint.js	`6.38% <0%> (+0.92%)`	⬆️
src/actions/utils/breakpoints.js	`78.26% <100%> (-13.05%)`	⬇️
src/components/Editor/Breakpoints.js	`13.33% <13.33%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dc9f00...2df507c. Read the comment docs.

jasonLaster · 2017-07-12T03:00:06Z

src/actions/breakpoints.js

@@ -127,7 +133,6 @@ export function disableBreakpoint(location: Location) {
    const action = {
      type: "DISABLE_BREAKPOINT",
      breakpoint: bp,
-      disabled: true,


yup, not being used...

jasonLaster · 2017-07-12T03:00:29Z

src/actions/tests/helpers/breakpoints.js

+    source: "function foo2(x, y) {\n  return x + y;\n}",
+    contentType: "text/javascript"
+  }
+};


perhaps there's a better way to keep this dummy data...

could it make sense to start using a factory library?

yeah - that might be a nice cleanup in a separate PR.

jasonLaster · 2017-07-12T03:01:34Z

src/components/Editor/Breakpoints.js

+    selectedSource: getSelectedSource(state)
+  }),
+  dispatch => bindActionCreators(actions, dispatch)
+)(Breakpoints);


I like the pattern of splitting out the editor children components. The separation makes it easier to follow

@asolove @codehag

jasonLaster · 2017-07-12T03:02:13Z

src/components/Editor/Breakpoint.js


-    editor.setGutterMarker(
+    showSourceText(editor, selectedSource.toJS());


This is important for making sure that we're updating the correct codemirror document.

jasonLaster · 2017-07-12T03:08:47Z

src/selectors/breakpointAtLocation.js

+}
+
+/*
+ *


I should include a quick description

jasonLaster · 2017-07-12T03:09:06Z

src/selectors/visibleBreakpoints.js

+    : breakpoint.location;
+}
+
+export default function getVisibleBreakpoints(state: OuterState) {


There should be a quick comment to explain how this is used

is this so that we can check if a breakpoint is also represented in a generated file? can we use it in all cases?

This will get the breakpoints for a file at the "correct" location regardless of original / generated

jasonLaster · 2017-07-12T03:09:28Z

src/test/mochitest/browser_dbg-sourcemaps2.js

@@ -24,7 +24,7 @@ add_task(function*() {
  yield addBreakpoint(dbg, mainSrc, 4);
  is(getBreakpoints(getState()).size, 1, "One breakpoint exists");
  ok(
-    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4 }),
+    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4, column: 2 }),


not sure why this changed :/

It is probably alright though

this might be a regression - before we were making sure that we didn't turn line breakpoints into column breakpoints...

jasonLaster · 2017-07-12T03:09:42Z

src/utils/breakpoint/index.js

@@ -14,8 +13,7 @@ export function firstString(...args) {

 export function locationMoved(location, newLocation) {
  return (
-    location.line !== newLocation.line ||
-    (location.column != null && location.column !== newLocation.column)
+    location.line !== newLocation.line || location.column !== newLocation.column


this is a nice clean up

jasonLaster · 2017-07-12T03:09:56Z

src/utils/breakpoint/index.js

-      : breakpoint.location;
-
-    const sameLine = location.line === line + 1;
+    const sameLine = breakpoint.location.line === line + 1;


i love these cleanups.

codehag

Great work so far!

codehag · 2017-07-12T09:07:59Z

src/actions/breakpoints.js

+    }
+
+    if (bp) {
+      // NOTE: it's possible the breakpoint has slid to a column


do we want to slide a breakpoint to a column if the user is expecting a line breakpoint?

codehag · 2017-07-12T09:08:27Z

src/actions/tests/helpers/breakpoints.js

+    source: "function foo2(x, y) {\n  return x + y;\n}",
+    contentType: "text/javascript"
+  }
+};


could it make sense to start using a factory library?

codehag · 2017-07-12T09:33:11Z

src/components/Editor/Breakpoints.js

+      return null;
+    }
+
+    return dom.div(


the Breakpoint component returns null; so, do we need to wrap it in a div, or just return null, with the mapping done in the body of the function?

codehag · 2017-07-12T09:34:15Z

src/components/Editor/GutterMenu.js

@@ -1,47 +1,45 @@
 import { showMenu } from "devtools-launchpad";

 export default function GutterMenu({
-  bp,
+  breakpoint,


codehag · 2017-07-12T09:38:45Z

src/selectors/breakpointAtLocation.js

+  return isGeneratedId(sourceId);
+}
+
+function getColumn(column, selectedSource) {


I like this pattern better than keeping the selectors in the reducer. maybe we could have a folder structure here:

selectors/ |- breakpoints/ |- index.js |- atLocation.js

codehag · 2017-07-12T09:40:04Z

src/selectors/visibleBreakpoints.js

+    : breakpoint.location;
+}
+
+export default function getVisibleBreakpoints(state: OuterState) {


is this so that we can check if a breakpoint is also represented in a generated file? can we use it in all cases?

codehag · 2017-07-12T09:40:34Z

src/test/mochitest/browser_dbg-sourcemaps2.js

@@ -24,7 +24,7 @@ add_task(function*() {
  yield addBreakpoint(dbg, mainSrc, 4);
  is(getBreakpoints(getState()).size, 1, "One breakpoint exists");
  ok(
-    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4 }),
+    getBreakpoint(getState(), { sourceId: mainSrc.id, line: 4, column: 2 }),


this might be a regression - before we were making sure that we didn't turn line breakpoints into column breakpoints...

codehag · 2017-07-12T09:41:00Z

src/utils/breakpoint/index.js

-      : breakpoint.location;
-
-    const sameLine = location.line === line + 1;
+    const sameLine = breakpoint.location.line === line + 1;


codehag reviewed Jul 10, 2017

View reviewed changes

jasonLaster mentioned this pull request Jul 11, 2017

Cannot set debugger breakpoint on generatedSource #3167

Closed

jasonLaster added this to the July 25th milestone Jul 11, 2017

jasonLaster changed the title ~~Refactor Breakpoints~~ Refactor Breakpoints to be location symmetric Jul 11, 2017

jasonLaster force-pushed the refactor-bps2 branch 2 times, most recently from 6eb8bee to b2f407c Compare July 12, 2017 02:59

jasonLaster commented Jul 12, 2017

View reviewed changes

codehag reviewed Jul 12, 2017

View reviewed changes

codehag approved these changes Jul 12, 2017

View reviewed changes

jasonLaster force-pushed the refactor-bps2 branch from b2f407c to 28a8e09 Compare July 12, 2017 15:11

Refactor toggling breakpoints logic

2df507c

jasonLaster force-pushed the refactor-bps2 branch from 28a8e09 to 2df507c Compare July 12, 2017 16:18

jasonLaster merged commit d66e4c0 into firefox-devtools:master Jul 12, 2017

codehag mentioned this pull request Nov 3, 2017

Improve JSX syntax highlighting #4539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Breakpoints to be location symmetric #3294

Refactor Breakpoints to be location symmetric #3294

jasonLaster commented Jul 10, 2017

codehag Jul 10, 2017

codehag Jul 10, 2017

codehag left a comment

codehag Jul 10, 2017

codehag Jul 10, 2017

codehag Jul 10, 2017

codecov bot commented Jul 11, 2017 •

edited

Loading

jasonLaster Jul 12, 2017

jasonLaster Jul 12, 2017

codehag Jul 12, 2017

jasonLaster Jul 12, 2017

jasonLaster Jul 12, 2017

jasonLaster Jul 12, 2017

jasonLaster Jul 12, 2017

jasonLaster Jul 12, 2017

codehag Jul 12, 2017

jasonLaster Jul 12, 2017

jasonLaster Jul 12, 2017

codehag Jul 12, 2017

jasonLaster Jul 12, 2017

jasonLaster Jul 12, 2017

codehag Jul 12, 2017

codehag left a comment

codehag Jul 12, 2017

codehag Jul 12, 2017

codehag Jul 12, 2017

codehag Jul 12, 2017

codehag Jul 12, 2017

codehag Jul 12, 2017

codehag Jul 12, 2017

codehag Jul 12, 2017


		editor.setGutterMarker(
		showSourceText(editor, selectedSource.toJS());

+              }
+              /*
+               *

Refactor Breakpoints to be location symmetric #3294

Refactor Breakpoints to be location symmetric #3294

Conversation

jasonLaster commented Jul 10, 2017

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codehag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codehag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2017 •

edited

Loading