Closed Bug 723923 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: jwalker, Assigned: sykopomp)

References

Details

Attachments

(3 files, 6 obsolete files)

Depends on: 683503
Priority: -- → P3
Whiteboard: [good first bug]
I'll add here that the "break list" command should just open the debugger rather than displaying an error.
Hi All,

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

Thanks.
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.
And here's the gist:
https://gist.github.com/3415894

Let me know if you have any trouble with it.
New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar
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 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.
I assume it's alright to navigate to SourceUtils from here?
Attachment #723235 - Attachment is obsolete: true
There's a few more updates after the previous patch.
Attachment #723256 - Attachment is obsolete: true
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.
Attachment #723297 - Flags: review?
Attachment #723300 - Flags: review?
Attachment #723301 - Flags: review?
Attachment #723297 - Flags: review? → review?(past)
Attachment #723300 - Flags: review? → review?(past)
Attachment #723301 - Flags: review? → review?(past)
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.
Attachment #723297 - Flags: review?(past) → review?(vporof)
Attachment #723300 - Flags: review?(past) → review?(vporof)
Attachment #723301 - Flags: review?(past) → review?(vporof)
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.
(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?
(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 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.
Attachment #723297 - Flags: review?(vporof)
Attachment #723297 - Flags: review?(jwalker)
Attachment #723297 - Flags: review+
Attachment #723300 - Flags: review?(vporof) → review+
Attachment #723301 - Flags: review?(vporof) → review+
(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")
    });
  }
}
On PTO: Added to list for Thursday.
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #723297 - Attachment is obsolete: true
Attachment #723297 - Flags: review?(jwalker)
Attachment #730522 - Flags: review?(vporof)
depends on the gcli patch
Attachment #723300 - Attachment is obsolete: true
Attachment #730524 - Flags: review?(vporof)
Attachment #730522 - Flags: review?(jwalker)
Attachment #723301 - Flags: review+ → review?(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.
Attachment #730522 - Flags: review?(jwalker) → review+
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 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.
Attachment #730522 - Flags: review?(vporof) → review-
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.
Attachment #730524 - Flags: review?(vporof)
Ahh! I blew them away when fixing rebase conflicts. So sorry!
(In reply to Josh Marchán from comment #25)
> Ahh! I blew them away when fixing rebase conflicts. So sorry!

No worries!
(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?
(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.
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.
Attachment #730522 - Attachment is obsolete: true
Attachment #731509 - Flags: review?(vporof)
Attachment #731509 - Flags: review?(jwalker)
Assignee: nobody → sykopomp
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?
Attachment #731509 - Flags: review?(vporof) → review+
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.
(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.
(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).
(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.
(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 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.
Attachment #723301 - Flags: review?(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
Attachment #731509 - Flags: review?(jwalker) → review+
(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! :)
(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.
> (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".
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.
Attachment #731509 - Attachment is obsolete: true
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.
Whiteboard: [good first bug] → [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/998804965b3f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.