834230 - break del command suffers from an off by 1 error

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Rob Campbell [:rc] (:robcee)]

STR:

Go to a page, set three breakpoints.

In the commandline, run "break del 3".

Error returned, breakpoint not found.

break del 2 removes breakpoint 3.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Attachment: break del by one WIP

fixing the tests which don't run. Still working on it.

Comment 2

[Rob Campbell [:rc] (:robcee)]

de-assigning while the unittests get fixed up.

Comment 3

[Joe Walker [:jwalker]]

Assigning to me so I don't forget

Comment 4

[Joe Walker [:jwalker]]

Attachment: v1

The short version is: We were not passing the context around to several places that we should be.

There is a context for the 'exec' of a function. It tells you stuff like the current target, what's on the command line, the current document/window, etc

    gcli.addCommand({
      name: "foo",
      exec: function(args, context) {
        // ...
      }
    });

The same context *should* be available to functions that define parameters, but was missing:

    gcli.addCommand({
      name: "break add line",
      params: [
        {
          name: "file",
          type: {
            name: "selection",
            data: function(context) {
              // return a list of the JS files in the current page
              // we can get the debugger using context.environment.target
            }
          },
        },
        ...
      ],
      exec: function(args, context) {
        // ...
      }
    });

The same goes for max/min functions to numbers:

    gcli.addCommand({
      name: "break del",
      params: [
        {
          name: "breakid",
          type: {
            name: "number",
            min: 0,
            max: function(context) {
              // return the max breakpoint id
              // we can get the debugger using context.environment.target
            },
          },
        }
      ],
      exec: function(args, context) {
        // ...
      }
    });

This patch is basically just passing 'context' to places that need it, and updating the tests to fix some cleanup and bitrot issues.

It works on my machine. I've not pushed to try yes, but will do.

Comment 5

[Rob Campbell [:rc] (:robcee)]

this doesn't apply cleanly.

Comment 6

[Joe Walker [:jwalker]]

Didn't apply cleanly because it depends on bug 839862.

Comment 7

[Joe Walker [:jwalker]]

Attachment: v2

I decided that I didn't like using a number as an identifier for a breakpoint so I changed it to use a label, which should be way nicer.
Also beefed up the unit tests a bit.

Comment 8

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=2669e09ec568

Comment 9

[Victor Porof [:vporof][:vp]]

(In reply to Joe Walker [:jwalker] from comment #7)

I'm rather confused as to what should be passed as an argument to "break del". Why is using an index a bad thing? It looks to me like it's faster than typing anything else.

While dealing with multiple files may currently be awkward with indices, I think a better solution to such scenarios would be a completion list "like break list" (we also have a remove button in the output there, which this patch seems to break).

Comment 10

[Joe Walker [:jwalker]]

(In reply to Victor Porof [:vp] from comment #9)
> (In reply to Joe Walker [:jwalker] from comment #7)
> 
> I'm rather confused as to what should be passed as an argument to "break
> del". Why is using an index a bad thing? It looks to me like it's faster
> than typing anything else.

Using numbers relies on you have *just* typed "break list" and then counted down. Creating or removing a breakpoint from the UI will change the list so your memory could be useless.

Plus there's completion to help you type the new labels, and it's obvious what you're going to do, which you can't say for an index.

> While dealing with multiple files may currently be awkward with indices, I
> think a better solution to such scenarios would be a completion list "like
> break list" (we also have a remove button in the output there, which this
> patch seems to break).

I did just notice a bug with a page that ended with a '/' which I'll fix, but the remove button WFM. What page do you see it on?

Comment 11

[Victor Porof [:vporof][:vp]]

The perpetual debugger debugging page http://htmlpad.org/debugger/ on debugger/. It'd be nice if the labels used in "break del" would be synced with what's shown in the sources list. SourceUtils has a method for that. I'd also be really happy if the documentation for the <breakpoint> param would be updated :) I imagine some cases may be unintuitive.

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Joe Walker [:jwalker] from comment #10)
> 
> Plus there's completion to help you type the new labels, and it's obvious
> what you're going to do, which you can't say for an index.
> 

Completion doesn't seem to work for me. Same test page as previous comment.

Comment 13

[Joe Walker [:jwalker]]

(In reply to Victor Porof [:vp] from comment #11)
> The perpetual debugger debugging page http://htmlpad.org/debugger/ on
> debugger/. It'd be nice if the labels used in "break del" would be synced
> with what's shown in the sources list. SourceUtils has a method for that.

I was using it, but had troubles:
* SourceUtils hard truncates at 20 chars for the sources list, and I can see why, but there's no hard 20 char limit on the cli
* this might not be a problem except that it prepends an elipsis, which is hard to type
* and because the truncate is hard at 20 chars, it gives you some strange labels "...ite_long_filename.js"

Now I considered altering the sources list, but that didn't seem right either. Eventually I just thought that they had different requirements.

> I'd also be really happy if the documentation for the <breakpoint> param
> would be updated :) I imagine some cases may be unintuitive.

Good point, thanks.

Comment 14

[Victor Porof [:vporof][:vp]]

(In reply to Joe Walker [:jwalker] from comment #13)
> 

Just to make sure we're talking about the same thing, I was referring to SourceUtils.getSourceLabel. In which case you can easily add a noTrim option to that method.

> Now I considered altering the sources list, but that didn't seem right
> either. Eventually I just thought that they had different requirements.

I'm glad you avoided altering the sources list :)

What I'm trying to say is that some values for the <breakpoint> param may be frustratingly confusing and very hard to cover in a short documentation. It's a bad idea to assume labels mean one thing in the debugger's sources list and something else in gcli. What do you do about foo/ for example? foo/:23 like everywhere in the debugger, or /:23 with a different implementation? What ?about=request&params? What about #references? And if we're going for making things look and act like in the debugger's sources list, why not reuse the methods in SourceUtils?

Comment 15

[Joe Walker [:jwalker]]

(In reply to Victor Porof [:vp] from comment #14)
> (In reply to Joe Walker [:jwalker] from comment #13)
> > 
> 
> Just to make sure we're talking about the same thing, I was referring to
> SourceUtils.getSourceLabel. In which case you can easily add a noTrim option
> to that method.
> 
> > Now I considered altering the sources list, but that didn't seem right
> > either. Eventually I just thought that they had different requirements.
> 
> I'm glad you avoided altering the sources list :)
> 
> What I'm trying to say is that some values for the <breakpoint> param may be
> frustratingly confusing and very hard to cover in a short documentation.
> It's a bad idea to assume labels mean one thing in the debugger's sources
> list and something else in gcli. What do you do about foo/ for example?
> foo/:23 like everywhere in the debugger, or /:23 with a different
> implementation? What ?about=request&params? What about #references? And if
> we're going for making things look and act like in the debugger's sources
> list, why not reuse the methods in SourceUtils?

So, with completion then you shouldn't need documentation, and worst case all you need to do is to call 'break list' to see the labels.

How would I call SourceUtils to avoid the problems in comment 13? I'm not against using it (in fact I'd prefer to use it) but wasn't aware that I could use it for what we needed.

Comment 16

[Victor Porof [:vporof][:vp]]

(In reply to Joe Walker [:jwalker] from comment #15)
> 
> So, with completion then you shouldn't need documentation, and worst case
> all you need to do is to call 'break list' to see the labels.
> 

Agreed, but let's make sure completion works first. Adding a breakpoint on htmlpad.org/debugger on line 13 and then doing |break del| -> tab/enter/whatever doesn't seem to do anything.

> How would I call SourceUtils to avoid the problems in comment 13? I'm not
> against using it (in fact I'd prefer to use it) but wasn't aware that I
> could use it for what we needed.

Just SourceUtils.getSourceLabel(url) is enough.

Comment 17

[Joe Walker [:jwalker]]

Attachment: v3

This uses SourceUtils, and WFM on http://htmlpad.org/debugger/ will post images showing what I think you should get.

I r?vporof just because he's been doing most of the reviewing so far. Happy either way.

Comment 18

[Joe Walker [:jwalker]]

Attachment: What it looks like for me on http://htmlpad.org/debugger/

Comment 19

[Victor Porof [:vporof][:vp]]

Comment on attachment 749193 [details] [diff] [review]
v3

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

This looks good, just a few small changes below for r+. I did manage to get the completion popup and output from another command to overlap, but I guess this is a different bug and not related to this patch.
1. break list
2. Click "Remove" on an entry
3. Click on the gcli input

::: browser/devtools/debugger/CmdDebugger.jsm
@@ +38,5 @@
> +
> +  for (let source in dbg.panelWin.DebuggerView.Sources) {
> +    for (let { attachment: breakpoint } in source) {
> +      let label = SourceUtils.getSourceLabel(source.value)
> +                                    + ":" + breakpoint.lineNumber;

You know what? You can actually use source.label here, since it's already stored, instead of going through all the SourceUtils.getSourceLabel shenanigans. I've been blind, sorry.

So:
> let label = source.label + ":" + breakpoint.lineNumber;

@@ +47,5 @@
> +        url: source.value,
> +        lineNumber: breakpoint.lineNumber,
> +        lineText: breakpoint.lineText,
> +        truncatedLineText: SourceUtils.trimUrlLength(breakpoint.lineText,
> +                                                  MAX_LINE_TEXT_LENGTH, "end")

Uber nit: defining this as a variable before the object literal would avoid bending whitespace because of timeless wrapping rules.

@@ +52,5 @@
> +      };
> +    }
> +  }
> +
> +  return Object.keys(breakpoints).map(key => { return breakpoints[key]; });

Why go through all this trouble when you can create an array in the first place? I don't see the benefit of populating an object then turning it into an array, but maybe I'm missing something.

@@ +225,5 @@
>      if (!dbg) {
>        return gcli.lookup("debuggerStopped");
>      }
>  
>      let breakpoints = dbg.getAllBreakpoints();

CmdDebugger.jsm is using both dbg.getAllBreakpoints and getAllBreakpoints. I would say this is a recipe for disaster.

In this particular case, a much more elegant solution would be to use the debugger's public breakpoint API:

let breakpoint = dbg.getBreakpoint(args.breakpoint.url, args.breakpoint.lineNumber);

Comment 20

[Victor Porof [:vporof][:vp]]

Attachment: overlap

Comment 21

[Joe Walker [:jwalker]]

(In reply to Victor Porof [:vp] from comment #19)
> Comment on attachment 749193 [details] [diff] [review]
> v3
> 
> Review of attachment 749193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, just a few small changes below for r+. I did manage to get
> the completion popup and output from another command to overlap, but I guess
> this is a different bug and not related to this patch.
> 1. break list
> 2. Click "Remove" on an entry
> 3. Click on the gcli input

Sigh - more fighting with XUL panels. JSTerm integration is forever on the horizon, so these problems keep getting pushed back. Maybe it's time to think about fixing bugs twice. :(

Rob - there are a bunch of issues like this, which is one of the reasons that I'm keen on getting going with JSTerm.

> ::: browser/devtools/debugger/CmdDebugger.jsm
> @@ +38,5 @@
> > +
> > +  for (let source in dbg.panelWin.DebuggerView.Sources) {
> > +    for (let { attachment: breakpoint } in source) {
> > +      let label = SourceUtils.getSourceLabel(source.value)
> > +                                    + ":" + breakpoint.lineNumber;
> 
> You know what? You can actually use source.label here, since it's already
> stored, instead of going through all the SourceUtils.getSourceLabel
> shenanigans. I've been blind, sorry.
> 
> So:
> > let label = source.label + ":" + breakpoint.lineNumber;

Done.

> @@ +47,5 @@
> > +        url: source.value,
> > +        lineNumber: breakpoint.lineNumber,
> > +        lineText: breakpoint.lineText,
> > +        truncatedLineText: SourceUtils.trimUrlLength(breakpoint.lineText,
> > +                                                  MAX_LINE_TEXT_LENGTH, "end")
> 
> Uber nit: defining this as a variable before the object literal would avoid
> bending whitespace because of timeless wrapping rules.

Nit in your Uber nit. No it wouldn't, I tried that ;-)

      var t = SourceUtils.trimUrlLength(breakpoint.lineText, MAX_LINE_TEXT_LENGTH, "end");

Is 91 chars in length

> @@ +52,5 @@
> > +      };
> > +    }
> > +  }
> > +
> > +  return Object.keys(breakpoints).map(key => { return breakpoints[key]; });
> 
> Why go through all this trouble when you can create an array in the first
> place? I don't see the benefit of populating an object then turning it into
> an array, but maybe I'm missing something.

This is a hangover from part of the original point of the code - to ensure that breakpoint.label is unique.

> @@ +225,5 @@
> >      if (!dbg) {
> >        return gcli.lookup("debuggerStopped");
> >      }
> >  
> >      let breakpoints = dbg.getAllBreakpoints();
> 
> CmdDebugger.jsm is using both dbg.getAllBreakpoints and getAllBreakpoints. I
> would say this is a recipe for disaster.
> 
> In this particular case, a much more elegant solution would be to use the
> debugger's public breakpoint API:
> 
> let breakpoint = dbg.getBreakpoint(args.breakpoint.url,
> args.breakpoint.lineNumber);

Ah - much better.

Comment 22

[Joe Walker [:jwalker]]

Attachment: v4

Comment 23

[Victor Porof [:vporof][:vp]]

Comment on attachment 750428 [details] [diff] [review]
v4

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

Smashing!

Comment 24

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=2b5ccd0f6279

Comment 25

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=6cf5ac92530b
https://hg.mozilla.org/integration/fx-team/rev/873bf55af0cd

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/873bf55af0cd