723923 - Debugger 'breakpoint list' GCLI command should have extra nice output

Status: RESOLVED FIXED

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

Description

[Joe Walker [:jwalker]]

For more detail, see bug 683503 comment 24.

Comment 1

[Kevin Dangoor]

I'll add here that the "break list" command should just open the debugger rather than displaying an error.

Comment 2

[Abhishek Potnis [:avp]]

Hi All,

I would like to work on this bug. Can anybody please guide me on getting started with this bug ?

Thanks.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Hi, thanks for taking an interest in this!

The place where the 'break list' command is implemented is here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/GcliCommands.jsm#542

The main task is to display the list in a more user-friendly way as Joe describes in bug 683503 comment 24. Also, as mentioned in comment 1, you should open the debugger UI if it's not already open. Victor has a reusable code snippet somewhere (gist?) that will show you how to do it.

Comment 4

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

And here's the gist:
https://gist.github.com/3415894

Let me know if you have any trouble with it.

Comment 5

[Joe Walker [:jwalker]]

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

Comment 6

[Josh Marchán]

Attachment: auto-open debugger on 'break list', format the list similarly to 'cookie list'

I was torn between making the breakpoint list work like the debugger's listing (with a checkbox for enable/disable), but figured it might be better to just go with what 'cookie list' does.

This patch makes it so only 'break list' shows the debugger if it's not already open -- if you want this for the rest of the 'break' commands, that's easy enough.

Hopefully I formatted everything correctly. :)

Comment 7

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

Comment on attachment 723235 [details] [diff] [review]
auto-open debugger on 'break list', format the list similarly to 'cookie list'

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +382,5 @@
> +          let breakpoint = breakpoints[breakpointKeys[i]];
> +          breakpointArray.push({
> +            id: i,
> +            url: breakpoint.location.url,
> +            label: breakpoint.lineInfo,

Just a quick note here: lineInfo has been removed in bug 812083. If you want something similar, you now need to build the string yourself.

Comment 8

[Josh Marchán]

Attachment: same as before, but without relying on lineInfo

I assume it's alright to navigate to SourceUtils from here?

Comment 9

[Josh Marchán]

Attachment: fixing issues from review and other issues found later

There's a few more updates after the previous patch.

Comment 10

[Josh Marchán]

Attachment: removes copypasted withCommand-related code from BuiltinCommands.jsm

The cookie list and break list commands both had a copypasted blob of withCommand. This code already exists in gcli. This patch requires a (separately-uploaded) gcli patch that exports updateCommand and executeCommand from gcli/util.

This patch applies on top of 723297. I'm keeping it separate because I'm not sure whether it belongs with this bug.

Comment 11

[Josh Marchán]

Attachment: exports updateCommand and executeCommand through gcli

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 723297 [details] [diff] [review]
fixing issues from review and other issues found later

Victor practically begged me to take these reviews. I didn't even have to ask him. At all.

Comment 13

[Joe Walker [:jwalker]]

There is a change on it's way which is relevant (but shouldn't stop this from landing). We're allowing people to split data from formatting.

Details: https://github.com/MikeRatcliffe/gcli/pull/1

Example, before:

    gcli.addCommand({
      name: 'example',
      returnType: 'html',
      exec: function() {
        var stuff = {
          property: 1,
          anotherProperty: 2
        };
        return "<p>" + stuff.property + "</p>";
      }
    });

After that lands, you can also do:

    gcli.addCommand({
      name: 'example',
      returnType: 'stuff',
      exec: function() {
        return {
          property: 1,
          anotherProperty: 2
        };
      }
    });

    gcli.addConverter({
      from: 'stuff',
      to: 'html',
      exec: function(stuff) {
        return "<p>" + stuff.property + "</p>";
      }
    });

This will allow us to pipe stuff from one command to another without needing to get into screen scraping.

I don't think we should stop this from landing, but it's worth noting.

Comment 14

[Josh Marchán]

(In reply to Joe Walker [:jwalker] from comment #13)
> There is a change on it's way which is relevant (but shouldn't stop this
> from landing). We're allowing people to split data from formatting.
> 
> Details: https://github.com/MikeRatcliffe/gcli/pull/1
> This will allow us to pipe stuff from one command to another without needing
> to get into screen scraping.
> 
> I don't think we should stop this from landing, but it's worth noting.

Would it make sense to make a separate ticket for converting (all?) the existing devtools commands to use addCommand/addConverter? I saw something in that pull request about type: "view". Is "html" still a valid type when returning createView() objects, or should those be converted as well?

Comment 15

[Joe Walker [:jwalker]]

(In reply to Josh Marchán from comment #14)
> (In reply to Joe Walker [:jwalker] from comment #13)
> > There is a change on it's way which is relevant (but shouldn't stop this
> > from landing). We're allowing people to split data from formatting.
> > 
> > Details: https://github.com/MikeRatcliffe/gcli/pull/1
> > This will allow us to pipe stuff from one command to another without needing
> > to get into screen scraping.
> > 
> > I don't think we should stop this from landing, but it's worth noting.
> 
> Would it make sense to make a separate ticket for converting (all?) the
> existing devtools commands to use addCommand/addConverter?

Yes. I raised bug 851801.

> I saw something
> in that pull request about type: "view". Is "html" still a valid type when
> returning createView() objects, or should those be converted as well?

Ah thanks - I'd not thought about that too much. The old system for working out what conversions were needed was really quite hacky and loose. When bug 657595 lands we'll need to sort that out because it requires us to be more specific about return types.

Comment 16

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

Comment on attachment 723297 [details] [diff] [review]
fixing issues from review and other issues found later

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

This looks good to me, but I'd like Joe to have another pass as well to make sure the GCLI changes are ok.
R+ with the slight change in building the breakpoints array as described below. Let me know if that works as it should.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +378,3 @@
>  
> +        let breakpointArray = [];
> +        let breakpointKeys = Object.keys(breakpoints);

Instead of all these shenanigans, it'd be much easier to do this:
(untested, but should work)

let SourceUtils = dbg.panelWin.SourceUtils;
let SourcesView = dbg.panelWin.DebuggerView.Sources;
let breakpointArray = [];

for (let { label, value, attachment } in SourcesView) {
  breakpointArray.push({
    id: attachment.actor,
    url: value,
    label: label + ":" + attachment.lineNumber,
    lineText: attachment.lineText,
    truncatedLineText: SourceUtils.trimUrlLength(attachment.lineText,
      MAX_LINE_TEXT_LENGTH, "end")
  });
}

This has the benefit of reusing the displayed source label and automatically using an intl.ellipsis instead of a "..." when trimming the length.

@@ +530,5 @@
> +    // debugger, this first case should be the only return value and all
> +    // other commands must be changed accordingly.
> +    if (opts && opts.ensure_opened) {
> +      return gDevTools.showToolbox(target, id).then(function(toolbox) {
> +          return toolbox.getPanel(id);

Nit: there are 2 extra spaces before this return statement.

Comment 17

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

(In reply to Victor Porof [:vp] from comment #16)
> R+ with the slight change in building the breakpoints array as described
> below. Let me know if that works as it should.

I just tested those to make your life easier and it's like this:

let SourceUtils = dbg.panelWin.SourceUtils;
let SourcesView = dbg.panelWin.DebuggerView.Sources;
let breakpointArray = [];

for (let source in SourcesView) {
  for (let { attachment: breakpoint } in source) {
    breakpointArray.push({
      id: breakpoint.actor,
      url: source.value,
      label: source.label + ":" + breakpoint.lineNumber,
      lineText: breakpoint.lineText,
      truncatedLineText: SourceUtils.trimUrlLength(breakpoint.lineText,
        MAX_LINE_TEXT_LENGTH, "end")
    });
  }
}

Comment 18

[Joe Walker [:jwalker]]

On PTO: Added to list for Thursday.

Comment 19

[Josh Marchán]

Attachment: updated patch

I reapplied the patch to the current HEAD of mozilla-central. I have not been able to test the patch, though, since I can't seem to get the branch to build right now and/or the debugger is currently borked.

Comment 20

[Josh Marchán]

Attachment: updated patch to use updateCommand/executeCommand in break and cookie commands.

depends on the gcli patch

Comment 21

[Joe Walker [:jwalker]]

Comment on attachment 730522 [details] [diff] [review]
updated patch

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +390,5 @@
> +            truncatedLineText: breakpoint.lineText.length > MAX_LINE_TEXT_LENGTH ?
> +                  breakpoint.lineText.substr(0, MAX_LINE_TEXT_LENGTH - 3) + "..." :
> +                  breakpoint.lineText
> +          });
> +        }

I think it would be a good idea to split out the data from the formatting like the cookie command does:

https://hg.mozilla.org/integration/fx-team/file/2bcc54a65d95/browser/devtools/commandline/BuiltinCommands.jsm#l777

That way when we have pipe and similar, we can pipe the unformatted data around.

I think you could just:

    return breakpointArray;

At this point and leave the rest up to a converter.

Comment 22

[Joe Walker [:jwalker]]

Comment on attachment 730524 [details] [diff] [review]
updated patch to use updateCommand/executeCommand in break and cookie commands.

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

I've batted this around in my head a lot, and I'm wondering if there might be a better solution in bug 856002. Going to have a very quick play.

Comment 23

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

Comment on attachment 730522 [details] [diff] [review]
updated patch

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

This patch doesn't seem to have addressed my previous review comment. Did you forget a qref? See comment #17.

Comment 24

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

Comment on attachment 730524 [details] [diff] [review]
updated patch to use updateCommand/executeCommand in break and cookie commands.

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

I don't have much to say regarding this. Let's see what Joe comes up with after playing with bug 856002.

Comment 25

[Josh Marchán]

Ahh! I blew them away when fixing rebase conflicts. So sorry!

Comment 26

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

(In reply to Josh Marchán from comment #25)
> Ahh! I blew them away when fixing rebase conflicts. So sorry!

No worries!

Comment 27

[Joe Walker [:jwalker]]

(In reply to Victor Porof [:vp] from comment #24)
> Comment on attachment 730524 [details] [diff] [review]
> updated patch to use updateCommand/executeCommand in break and cookie
> commands.
> 
> Review of attachment 730524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have much to say regarding this. Let's see what Joe comes up with
> after playing with bug 856002.

Short version:

Lets make context.update do something smart if it's passed a DOM element. That way we don't have to expose more functions, and people don't have to import new stuff. Can we land with the duplication, and I'll remove it when I fix bug 856002?

Comment 28

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #27)
> Lets make context.update do something smart if it's passed a DOM element.
> That way we don't have to expose more functions, and people don't have to
> import new stuff. Can we land with the duplication, and I'll remove it when
> I fix bug 856002?

I should add - I have code! (or at least something vaguely right) so that wasn't a request.

Comment 29

[Josh Marchán]

Attachment: Converters, trimUrlLength, new style of breakpoint id, ponies

Victor: The situation was even more dire than I thought. The commit survived the rebase just fine. I had just uploaded the wrong patch. :)

Either way, there was a lot to improve still. This patch makes the general flow cleaner, uses addConverter to separate the presentation from the command, and uses your suggested for loop for setting up the breakpoint array.

I noticed while testing that the syntax for breakpoint ids has changed with the recent changes made to remote debugging, so I've played around with a more compact syntax for representing them. The action button still spits out the "full" id, even though break del is currently broken, so neither the full nor the short versions of the breakpoint ids will be valid until that's fixed. Please let me know what you think of the shorter ids -- it's kinda hard to fit all this information in the message box without it.

Comment 30

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

Comment on attachment 731509 [details] [diff] [review]
Converters, trimUrlLength, new style of breakpoint id, ponies

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

Debugger usage looks good to me. R+ with the comment below addressed.

::: browser/devtools/debugger/CmdDebugger.jsm
@@ +69,5 @@
> +        data: {
> +          breakpoints: breakpoints.map(function(breakpoint) {
> +            let [_full, conn, num] =
> +                  /conn(\d+)\.breakpoint(\d+)/.exec(breakpoint.id)
> +                  || [null, "?", "?"];

I'm not sure if we really need to go into that much detail. Do we really need to output the breakpoint id?

Especially given the current scenario of having to regex our way into these identifiers, I don't think showing the ids is a good idea. Come to think about it, it doesn't actually offer any useful information to the user, so remove them for now.

@@ +122,5 @@
> +
> +var breakListCss = "" +
> +      ".breakpoint-label { font-weight: bold; }\n" +
> +      ".breakpoint-lineText { font-family: monospace; }" +
> +      "";

Is it ok to have CSS here? Joe?

Comment 31

[Josh Marchán]

The breakpoint ids are used in 'break del'. Any other future commands that would edit breakpoints would also need to use the ids. Alternately, if we just use some index for break del/break editwhatever, then no, these ids will be of no use.

Comment 32

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

(In reply to Josh Marchán from comment #31)
> The breakpoint ids are used in 'break del'. Any other future commands that
> would edit breakpoints would also need to use the ids. Alternately, if we
> just use some index for break del/break editwhatever, then no, these ids
> will be of no use.

It's not really the id in there, but the index (as in: the n-th breakpoint which exists in the current debugger). And I'm not sure we should ever design commands that use ids.

Comment 33

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

(documentation for that command isn't particularly helpful, maybe we should rename "brekaid" to "index", and suggest using "break list" for finding the index; alternatively, a new command that looks like "break del <file> <line>" (with --column once that's available) may also be useful, but I think it's a bit premature to think about it at this point).

Comment 34

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

(In reply to Josh Marchán from comment #31)
> The breakpoint ids are used in 'break del'. Any other future commands that
> would edit breakpoints would also need to use the ids. Alternately, if we
> just use some index for break del/break editwhatever, then no, these ids
> will be of no use.

Sorry, you're totally right, I missed that. Disregard comment #32. Let's make "break del" work with indices in a followup, it's quite uncomfortable dealing with ids directly.

Comment 35

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

(In reply to Victor Porof [:vp] from comment #34)

I'm retarded :)
That command *is* using indices, not ids, so I was right in comment #32.

Comment 36

[Joe Walker [:jwalker]]

Comment on attachment 723301 [details] [diff] [review]
exports updateCommand and executeCommand through gcli

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

The upshot of https://github.com/joewalker/gcli/commit/0f6b0eb9e6912737540abdff89f00371a0383cef is that once this lands we should be able to do:
    context.update(ev.currentTarget);

Which is much cleaner.
Lets accept some code duplication now, and fix when this lands.

Comment 37

[Joe Walker [:jwalker]]

Comment on attachment 731509 [details] [diff] [review]
Converters, trimUrlLength, new style of breakpoint id, ponies

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

I'm torn on the CSS thing. On the one hand I like that commands are self contained. On the other hand theming. Injecting CSS here isn't themable. Personal thoughts aside, Mozilla prefers things to be themable, so I think we should have this in browser/themes/[osx|linux|windows]/commandline.css

Comment 38

[Josh Marchán]

(In reply to Victor Porof [:vp] from comment #33)
> (documentation for that command isn't particularly helpful, maybe we should
> rename "brekaid" to "index", and suggest using "break list" for finding the
> index; alternatively, a new command that looks like "break del <file>
> <line>" (with --column once that's available) may also be useful, but I
> think it's a bit premature to think about it at this point).

iirc, breakpoint ids used to be simple indices. It changed recently. I agree that using indices is probably more useful for the command than some kind of wrapper around the new breakpoint ids. Using <file> <line> for break del in the future is also a good idea. Should it get its own ticket?

I propose leaving the id column there, but making the data a simple (0-based?) index on the list of current breakpoints. Once break del <file> <line> is available, we can remove the index altogether.

At the same time, considering how there's a delete button on each row of the table, it might not be bad to already drop the id column altogether. Thoughts? (potential downside: removing fun let-destructuring code :<)

(In reply to Joe Walker [:jwalker] from comment #37)
> Comment on attachment 731509 [details] [diff] [review]
> Converters, trimUrlLength, new style of breakpoint id, ponies
> 
> Review of attachment 731509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm torn on the CSS thing. On the one hand I like that commands are self
> contained. On the other hand theming. Injecting CSS here isn't themable.
> Personal thoughts aside, Mozilla prefers things to be themable, so I think
> we should have this in browser/themes/[osx|linux|windows]/commandline.css

You r+ed the patch. Do you still want me to move the css to commandline.css?

(In reply to Joe Walker [:jwalker] from comment #36)
> Comment on attachment 723301 [details] [diff] [review]
> exports updateCommand and executeCommand through gcli
> 
> Review of attachment 723301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The upshot of
> https://github.com/joewalker/gcli/commit/
> 0f6b0eb9e6912737540abdff89f00371a0383cef is that once this lands we should
> be able to do:
>     context.update(ev.currentTarget);
> 
> Which is much cleaner.
> Lets accept some code duplication now, and fix when this lands.

Sounds good! :)

Comment 39

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

(In reply to Josh Marchán from comment #38)
> 
> At the same time, considering how there's a delete button on each row of the
> table, it might not be bad to already drop the id column altogether.
> Thoughts? (potential downside: removing fun let-destructuring code :<)
> 

Let's keep things as simple as possible for now. Since breakpoint ids aren't necessary for any other commands ("break del" uses *index*), remove them from the output.

Comment 40

[Joe Walker [:jwalker]]

> (In reply to Joe Walker [:jwalker] from comment #37)
> > Comment on attachment 731509 [details] [diff] [review]
> > Converters, trimUrlLength, new style of breakpoint id, ponies
> > 
> > Review of attachment 731509 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm torn on the CSS thing. On the one hand I like that commands are self
> > contained. On the other hand theming. Injecting CSS here isn't themable.
> > Personal thoughts aside, Mozilla prefers things to be themable, so I think
> > we should have this in browser/themes/[osx|linux|windows]/commandline.css
> 
> You r+ed the patch. Do you still want me to move the css to commandline.css?

Yes please.

If I r+ with comments, I'm saying "if you do whatever-i-just-said then don't feel the need to r? me again".

Comment 41

[Josh Marchán]

Attachment: vN+1 - replace ids with indices, move css to browser/themes

I believe this addresses all the comments.

One last thing: I noticed that there's a browser/themes/shared/devtools/commandline.inc.css. I would have guessed this to be a better place to put this CSS, as opposed to copying it into three separate files, but I used [osx|windows|linux]/devtools/commandline.css anyway, specially since addons have already set a precedent there.

Comment 42

[Joe Walker [:jwalker]]

I'll find out about when we should be using the inc files. They're new to me.

Thanks for the patch. Will add to my landing queue.

Comment 43

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/998804965b3f

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/998804965b3f