1141507 - Refactor breakpoint sliding for source mapped sources

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

Breakpoint sliding for source-mapped sources should be performed on original locations, not generated locations.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Some preliminary refactors

This patch contains some preliminary refactors that we need. Among other things:

- The length of the breakpoint method names was starting to get out of hand, so I
  refactored those to something more concise.
- I factored out the code that updates the actor and actor map when the actual
  location differs from the original one into a separate then handler, because this
  code will be the same for both source mapped and non-source mapped actors.
- Even when we're not doing breakpoint sliding, we can still end up with
  an actual location that is different from the original location due to fuzzy
  search. Consequently, _setBreakpointAtOriginalLocation needs to return the
  original location corresponding to the generated location.
- I've added a stub implementation of getAllGeneratedLocations for. Right now this
  will simply return the result of getGeneratedLocation wrapped in an array, but
  eventually this will use allGeneratedPositionsFor to return all mappings that
  correspond to a given line.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor breakpoint sliding for source mapped actors.

Hopefully this patch is fairly self-explanatory. Let me know if you have any questions

Couple of notes:
- We now use getAllGeneratedLocations to get all generated locations for a given
  location in an original source (as opposed to just getting the first generated
  location we can find), and try to set a breakpoint on all of them.

  getAllGeneratedLocations is implemented using allGeneratedPositionsFor, which
  landed as part of the merge of the source-map library with fx-team.

- GeneratedLocation now contains both the first and last column for each mapping,
  so we can set breakpoints on column spans instead of single columns.

  The last column is computed lazily using computeColumnSpans, which landed as part
  of the merge of the source-map library with fx-team.

- We now slide on the original location, not the generated location, when sliding
  in source-mapped sources.

Comment 3

[James Long (:jlongster)]

Comment on attachment 8575275 [details] [diff] [review]
Some preliminary refactors

Review of attachment 8575275 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this patch basically moves around some functions and that's all, and there's nothing in here concerning to me.

::: toolkit/devtools/server/actors/script.js
@@ +3027,5 @@
>  
>      if (entryPoints.length === 0) {
>        return false;
>      }
> +    setBreakpointAtEntryPoints(actor, entryPoints);

+1 for simplifying these names and dropping the "forActor"

Comment 4

[James Long (:jlongster)]

I'll finish looking at the other patch first thing tomorrow; I want to try to understand the changes as best as possible so I can give a good review.

Comment 5

[James Long (:jlongster)]

Comment on attachment 8577295 [details] [diff] [review]
Refactor breakpoint sliding for source mapped actors.

Review of attachment 8577295 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, a little hard to follow the big picture across several patches but that's fine. You remove a lot of code here but I think a lot of that showed up in previous patches in a different form.

::: toolkit/devtools/server/actors/common.js
@@ +438,5 @@
> +  },
> +
> +  equals: function (other) {
> +    return this.generatedSourceActor.url == other.generatedSourceActor.url &&
> +           this.generatedLine === other.originalLine;

How come you don't need to check the column? Seems like you would need that for column sliding, no?

::: toolkit/devtools/server/actors/script.js
@@ +2897,5 @@
>            return originalLocation;
>          }
>        } else {
> +        if (originalColumn === undefined) {
> +          let loop = (actualLocation) => {

nit: you don't need the parens around a single argument

@@ +2922,5 @@
> +                return this.threadActor.sources.getOriginalLocation(generatedLocations[0]);
> +              }
> +
> +              // Try the next line in the original source.
> +              return loop(new OriginalLocation(this, actualLine + 1));

Clever use of `loop` to get around the problems with promise control flow (eventually we should use generators more, but we can do that later)

@@ +5665,5 @@
>        return OriginalLocation.fromGeneratedLocation(generatedLocation);
>      });
>    },
>  
>    getAllGeneratedLocations: function (originalLocation) {

Docs for this function? I know you are going to continue refactoring but this would be a good function to have docs for (and should stay around). Explain when to use this and when to just use `getGeneratedLocation`.

Comment 6

[Eddy Bruel [:ejpbruel]]

Try push for the refactor patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33d120a3710c

Comment 7

[Eddy Bruel [:ejpbruel]]

Try run looks good, pushing to fx-team (after rebase):
https://hg.mozilla.org/integration/fx-team/rev/a10fbddc674c

Comment 8

[Eddy Bruel [:ejpbruel]]

Try push for refactoring breakpoint sliding for source mapped actors:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb072bfda1f0

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a10fbddc674c

Comment 10

[Eddy Bruel [:ejpbruel]]

Try push for the second patch looks good, pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/a752e16a5251

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a752e16a5251