The default bug view has changed. See this FAQ.

GCLI needs commands to control the debugger

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Console
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwalker, Assigned: past)

Tracking

unspecified
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

This command allows adding, listing, and removing breakpoinst
Blocks: 659052
No longer blocks: 671406
OS: Windows 7 → All
Hardware: x86_64 → All
Blocks: 689605
No longer blocks: 659052
Summary: GCLI needs a 'break' command to handle debugger breakpoints → GCLI needs commands to control the debugger
Duplicate of this bug: 683505
Created attachment 573178 [details]
some old code that pretended to work quite well
Assignee: nobody → past
For what it's worth these are the l10n strings for /devtools/browser/locales/en-US/chrome/browser/devtools/gclicommands.properties


breakDesc=Manage breakpoints
breakManual=Commands to list, add and remove breakpoints

breaklistDesc=Display known breakpoints
breaklistNone=No breakpoints set
breaklistIntro=The following breakpoints are set:

breakaddIfDesc=Break condition
breakaddAdded=Added breakpoint
breakaddDesc=Add a breakpoint
breakaddManual=Breakpoint types supported: line, function, xhr, event

breakaddlineDesc=Add a line breakpoint
breakaddlineFileDesc=JS file
breakaddlineLineDesc=Line number
breakaddfuncDesc=Add a function breakpoint
breakaddfuncFunctionDesc=Function
breakaddxhrDesc=Add an XHR breakpoint
breakaddeventDesc=Add an event breakpoint
breakaddeventEventDesc=Event type
breakaddeventNodeDesc=Node

breaknextDesc=Break at next line
breaknextReply=Breaking at next line

breakdelDesc=Remove a breakpoint
breakdelBreakidDesc=Index of breakpoint
breakdelRemoved=Breakpoint removed

stepDesc=Move the debugger on
stepManual=When the debugger is stopped, move it to a new place
stepInsult=The debugger is not stopped. How do you expect to start it? Duh!

stepupDesc=Step out of the current function
stepupManual=Move the debug position up one stack frame by running to the end of the current function

stepinDesc=Step into the current function
stepinManual=Move the debug position down one stack frame into the current function call

stepnextDesc=Step to the next line
stepnextManual=Move the debug position to the start of the next line

steponDesc=Continue running
steponManual=Resume the debugger running normally

steptoDesc=Run to the given line
steptoManual=Move the debug position to the start of the specified line
steptoLineDesc=Line to run to
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Blocks: 659052
No longer blocks: 689605
Created attachment 579100 [details] [diff] [review]
Working patch

This patch implements all the breakpoint handling that is currently supported by the debugger, which amounts to just list, add and del. No tests yet.
This patch obviously depends on the debugger code.
Depends on: 697762
Created attachment 579325 [details] [diff] [review]
Working patch v2

Available scripts now auto-complete, and there is a half-finished test in there.
Attachment #579100 - Attachment is obsolete: true
Created attachment 579727 [details] [diff] [review]
Working patch v3

Completed test and also made a few simplifications to the code. There is a funny artifact in the 'break list' output that I didn't fix, because I think it must be some gcli style issue that you might be aware of (do <ol> and <li> get some special treatment?).
Attachment #579325 - Attachment is obsolete: true
Attachment #579727 - Flags: review?(jwalker)
(In reply to Panos Astithas [:past] from comment #8)
> Created attachment 579727 [details] [diff] [review]
> Working patch v3
> 
> Completed test and also made a few simplifications to the code. There is a
> funny artifact in the 'break list' output that I didn't fix, because I think
> it must be some gcli style issue that you might be aware of (do <ol> and
> <li> get some special treatment?).

The li bizarreness is fixed waiting for review. It's an artifact from web console styling.
Comment on attachment 579727 [details] [diff] [review]
Working patch v3

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

I've not tried the code out and I'd like to - are there instructions on doing this anywhere?

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +194,5 @@
> +    {
> +      name: "file",
> +      type: {
> +        name: "selection",
> +        data: function() {

We should factor this out into a separate type at some stage. I raised bug 709223 for this.

@@ +239,5 @@
> +          };
> +          args.client = aBpClient;
> +          breakpoints.push(args);
> +          promise.resolve(gcli.lookup("breakaddAdded"));
> +        });

Someone picky might complain about 2 space indents here

::: browser/devtools/webconsole/test/browser/browser_gcli_break.js
@@ +52,5 @@
> +  ok(requisition, "We have a Requisition");
> +}
> +
> +function testCreateCommands() {
> +  type("brea");

This could be fragile if we were to get another command called 'bread' etc. I think that's unlikey enough to not bother me though
Attachment #579727 - Flags: review?(jwalker) → review+
(In reply to Joe Walker from comment #10)
> Comment on attachment 579727 [details] [diff] [review]
> Working patch v3
> 
> Review of attachment 579727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've not tried the code out and I'd like to - are there instructions on
> doing this anywhere?

Oh yes, my mistake for not posting instructions. Applying this is not straightforward.

1) Either (a) clone the remote-debug repo, or (b) checkout rev. 677c4e284a9d in fx-team and apply the big patch in bug 697762.
2) Apply the patches in bug 703667, bug 700639 and bug 702939.
3) Apply the review-requested changes patch in bug 697762.

Steps 2 and 3 are only required to get this patch to apply cleanly. If you apply the small part of this patch on debugger-view.js manually, you can get away with that.

As for testing this code, I'd suggest using htmlpad.org/debugger (or something similar) as a test page. You can use it to get the debugger to a paused state (the page includes debugger statements) where setting a breakpoint will be allowed. If you only want to test the script completion code, loading any page will be fine.

Work on making this more useful is underway, this is the first time breakpoints can be set in the UI.

> @@ +239,5 @@
> > +          };
> > +          args.client = aBpClient;
> > +          breakpoints.push(args);
> > +          promise.resolve(gcli.lookup("breakaddAdded"));
> > +        });
> 
> Someone picky might complain about 2 space indents here

I'm not sure I get this, do you mean this indentation a few lines above?

+    dbg.activeThread.setBreakpoint({ url: args.file, line: args.line },
+        function(aResponse, aBpClient) {

In that case, I tried to follow the rule about lining arguments in the same column (in this case 'f' from function right below the opening curly bracket), but it got so far right that almost all lines needed folding. I decided to break that rule and indent 'function' with 2 levels (4 spaces) in order to avoid confusing it with a nested statement. Maybe I should have put .setBreakpoint below .activeThread and used the official rule, but it seemed weird.

I might have cried while making this decision.

> ::: browser/devtools/webconsole/test/browser/browser_gcli_break.js
> @@ +52,5 @@
> > +  ok(requisition, "We have a Requisition");
> > +}
> > +
> > +function testCreateCommands() {
> > +  type("brea");
> 
> This could be fragile if we were to get another command called 'bread' etc.
> I think that's unlikey enough to not bother me though

Agreed, but also note that we have a similar issue in the inspect command test that I borrowed heavily from.
Comment on attachment 579727 [details] [diff] [review]
Working patch v3

We also need a devtools peer review.
Attachment #579727 - Flags: review?(dcamp)
(In reply to Panos Astithas [:past] from comment #11)
> (In reply to Joe Walker from comment #10)
> > Comment on attachment 579727 [details] [diff] [review]
> > Working patch v3
> > 
> > Review of attachment 579727 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I've not tried the code out and I'd like to - are there instructions on
> > doing this anywhere?
> 
> Oh yes, my mistake for not posting instructions. Applying this is not
> straightforward.
> 
> 1) Either (a) clone the remote-debug repo, or (b) checkout rev. 677c4e284a9d
> in fx-team and apply the big patch in bug 697762.
> 2) Apply the patches in bug 703667, bug 700639 and bug 702939.
> 3) Apply the review-requested changes patch in bug 697762.
> 
> Steps 2 and 3 are only required to get this patch to apply cleanly. If you
> apply the small part of this patch on debugger-view.js manually, you can get
> away with that.
> 
> As for testing this code, I'd suggest using htmlpad.org/debugger (or
> something similar) as a test page. You can use it to get the debugger to a
> paused state (the page includes debugger statements) where setting a
> breakpoint will be allowed. If you only want to test the script completion
> code, loading any page will be fine.
> 
> Work on making this more useful is underway, this is the first time
> breakpoints can be set in the UI.

Thanks. Will play.

> > @@ +239,5 @@
> > > +          };
> > > +          args.client = aBpClient;
> > > +          breakpoints.push(args);
> > > +          promise.resolve(gcli.lookup("breakaddAdded"));
> > > +        });
> > 
> > Someone picky might complain about 2 space indents here
> 
> I'm not sure I get this, do you mean this indentation a few lines above?
> 
> +    dbg.activeThread.setBreakpoint({ url: args.file, line: args.line },
> +        function(aResponse, aBpClient) {
> 
> In that case, I tried to follow the rule about lining arguments in the same
> column (in this case 'f' from function right below the opening curly
> bracket), but it got so far right that almost all lines needed folding. I
> decided to break that rule and indent 'function' with 2 levels (4 spaces) in
> order to avoid confusing it with a nested statement. Maybe I should have put
> .setBreakpoint below .activeThread and used the official rule, but it seemed
> weird.
> 
> I might have cried while making this decision.

:)

I should, perhaps, have said this:

> > Someone picky might complain about 2 space indents here
> > Fortunately I'm not picky.

I agree that this is one of those cases where indentation gets tricky. And I *really* don't care about how, or even if, you 'solve' it.

I might have 'solved' the problem like this:

   let position = { url: args.file, line: args.line };
   dbg.activeThread.setBreakpoint(position, function(aResponse, aBpClient) {
     if (aResponse.error)
     ...
   });

Feel free to 'solve' the issue by doing nothing.

> > ::: browser/devtools/webconsole/test/browser/browser_gcli_break.js
> > @@ +52,5 @@
> > > +  ok(requisition, "We have a Requisition");
> > > +}
> > > +
> > > +function testCreateCommands() {
> > > +  type("brea");
> > 
> > This could be fragile if we were to get another command called 'bread' etc.
> > I think that's unlikey enough to not bother me though
> 
> Agreed, but also note that we have a similar issue in the inspect command
> test that I borrowed heavily from.

Ah! Looks like it didn't bother me there either!
(In reply to Joe Walker from comment #13)
> (In reply to Panos Astithas [:past] from comment #11)
> > (In reply to Joe Walker from comment #10)
> > > @@ +239,5 @@
> > > > +          };
> > > > +          args.client = aBpClient;
> > > > +          breakpoints.push(args);
> > > > +          promise.resolve(gcli.lookup("breakaddAdded"));
> > > > +        });
> > > 
> > > Someone picky might complain about 2 space indents here
> > 
> > I'm not sure I get this, do you mean this indentation a few lines above?
> > 
> > +    dbg.activeThread.setBreakpoint({ url: args.file, line: args.line },
> > +        function(aResponse, aBpClient) {
> > 
> > In that case, I tried to follow the rule about lining arguments in the same
> > column (in this case 'f' from function right below the opening curly
> > bracket), but it got so far right that almost all lines needed folding. I
> > decided to break that rule and indent 'function' with 2 levels (4 spaces) in
> > order to avoid confusing it with a nested statement. Maybe I should have put
> > .setBreakpoint below .activeThread and used the official rule, but it seemed
> > weird.
> > 
> > I might have cried while making this decision.
> 
> :)
> 
> I should, perhaps, have said this:
> 
> > > Someone picky might complain about 2 space indents here
> > > Fortunately I'm not picky.

Oh yeah, I totally got that you weren't fussed about it.

> I agree that this is one of those cases where indentation gets tricky. And I
> *really* don't care about how, or even if, you 'solve' it.
> 
> I might have 'solved' the problem like this:
> 
>    let position = { url: args.file, line: args.line };
>    dbg.activeThread.setBreakpoint(position, function(aResponse, aBpClient) {
>      if (aResponse.error)
>      ...
>    });
> 
> Feel free to 'solve' the issue by doing nothing.

Ah yes, I've seen this 'fix' before, but it somehow keeps eluding me.
Created attachment 587298 [details] [diff] [review]
Working patch v4

Rebased on top of the latest change sets in remote-debug and made the change suggested in comment 13.
Attachment #579727 - Attachment is obsolete: true
Attachment #579727 - Flags: review?(dcamp)
Attachment #587298 - Flags: review?(dcamp)
Triage. Filter on PEGASUS.
Priority: -- → P2
Triage. Filter on PEGASUS.
Triage. Filter on PEGASUS.
No longer blocks: 659052
Created attachment 590648 [details] [diff] [review]
Working patch v5

Rebased.
Attachment #587298 - Attachment is obsolete: true
Attachment #587298 - Flags: review?(dcamp)
Attachment #590648 - Flags: review?(dcamp)

Comment 20

5 years ago
Comment on attachment 590648 [details] [diff] [review]
Working patch v5

Looks good to me.  Pinging joe to double-check gcli parts (looks like he already took a look, so it should be quick...)
Attachment #590648 - Flags: review?(jwalker)
Attachment #590648 - Flags: review?(dcamp)
Attachment #590648 - Flags: review+
Attachment #590648 - Attachment is patch: true
Comment on attachment 590648 [details] [diff] [review]
Working patch v5

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

Most of this is theoretical, we should probably fix the min:0/1 thing though, and ensure we're not doing anything disastrous in storing breakpoints locally.
r+ with those out of the way.

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +105,5 @@
>      InspectorUI.openInspectorUI(args.node);
>    }
>  });
> +
> +let breakpoints = [];

This method of storing breakpoints was a bit of a hack for a POC - is there a way we could use the debugger's store of breakpoints rather than duplicating the list?

What if breakpoints pre-exist from an earlier session?

@@ +132,5 @@
> +
> +    let reply = gcli.lookup("breaklistIntro");
> +    reply += "<ol>";
> +    breakpoints.forEach(function(breakpoint) {
> +      reply += "<li>" + JSON.stringify(breakpoint) + "</li>";

Is JSON.stringify the best way we have of representing a breakpoint to a user?

@@ +170,5 @@
> +            for each (let script in scriptsView.scriptLocations()) {
> +              files.push(script);
> +            }
> +          }
> +          return files;

Hmm. GCLI has another way to do this:
https://github.com/joewalker/gcli/blob/resource-709223/lib/gcli/types/resource.js#L186

Your version looks like it's probably going to be more accurate? Please could you take a look at the GCLI version and comment? I suppose the real question is - what are the benefits of changing the GCLI code to match this?

@@ +177,5 @@
> +      description: gcli.lookup("breakaddlineFileDesc")
> +    },
> +    {
> +      name: "line",
> +      type: { name: "number", min: 0, step: 10 },

Shouldn't min be 1?

For discussion - we could also do:

  type: {
    name: "number",
    min: 0,
    max: function(args) {
      return something.getNumberOfLinesInFile(args.file)
    }
    step: 10
  },

(Or at least we could if GCLI passed args to max() - currently it doesn't, and I've been delaying fixing bug 722727 until it was needed.)

IF that bug was fixed, would it be easy to get the number of lines in a file?

@@ +183,5 @@
> +    }
> +  ],
> +  returnType: "html",
> +  exec: function(args, context) {
> +    args.type = "line";

I was just thinking 'what if (for performance reasons) args was a proxy object which would calculate it's values on demand'.

And then you demonstrate why that could be risky.

I'm not sure what I think of this scenario. Who is in the right? Did GCLI pass you a something overly clever or did this code misuse GCLI resources?

Note: I'm not asking you to change this code (probably). I'm mulling the rights and wrongs of the situation.

It seems inevitable that people will do this kind of thing, so GCLI would probably be wrong to assume they wouldn't, I think...
Attachment #590648 - Flags: review?(jwalker) → review+
(In reply to Joe Walker from comment #21)
> Comment on attachment 590648 [details] [diff] [review]
> Working patch v5
> 
> Review of attachment 590648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Most of this is theoretical, we should probably fix the min:0/1 thing
> though, and ensure we're not doing anything disastrous in storing
> breakpoints locally.
> r+ with those out of the way.
> 
> ::: browser/devtools/webconsole/GcliCommands.jsm
> @@ +105,5 @@
> >      InspectorUI.openInspectorUI(args.node);
> >    }
> >  });
> > +
> > +let breakpoints = [];
> 
> This method of storing breakpoints was a bit of a hack for a POC - is there
> a way we could use the debugger's store of breakpoints rather than
> duplicating the list?

This is mostly a list of the front objects that correspond to breakpoint actors on the server. As such they are just plain old javascript objects (POJOs!) without any special lifetime requirements. On the other hand, the debugger UI currently does not maintain any list of breakpoints, since the relevant functionality is not there yet (bug 723069). When it does it may make sense to have GCLI tap into a facility provided by the debugger UI.

> What if breakpoints pre-exist from an earlier session?

That's another reasonable argument for having GCLI use a debugger UI-provided API, but we haven't even filed the bugs for a breakpoint store yet. Can we get this in and refactor once the necessary API is there?

> @@ +132,5 @@
> > +
> > +    let reply = gcli.lookup("breaklistIntro");
> > +    reply += "<ol>";
> > +    breakpoints.forEach(function(breakpoint) {
> > +      reply += "<li>" + JSON.stringify(breakpoint) + "</li>";
> 
> Is JSON.stringify the best way we have of representing a breakpoint to a
> user?

It seems OK to me for now, but I'm open to suggestions. Would you prefer something like "Line breakpoint at <url>:<line>"? When we get the breakpoint view (bug 723071) it would be nice to present the list in the same way for the sake of consistency.

> @@ +170,5 @@
> > +            for each (let script in scriptsView.scriptLocations()) {
> > +              files.push(script);
> > +            }
> > +          }
> > +          return files;
> 
> Hmm. GCLI has another way to do this:
> https://github.com/joewalker/gcli/blob/resource-709223/lib/gcli/types/
> resource.js#L186
> 
> Your version looks like it's probably going to be more accurate? Please
> could you take a look at the GCLI version and comment? I suppose the real
> question is - what are the benefits of changing the GCLI code to match this?

I've already seen it and I plan on using it in the future. It's not going to work right now, because the debugger doesn't always know about the scripts loaded in the page, due to the lack of getAllScripts (bug 723563 and bug 697040). As it is, the user is bound to get noScript errors when the debugger's list doesn't mach GCLI's, unless GCLI asks the debugger for the list.

One other concern I have with the resource type is with HTML files containing more than one script, but the debugger is probably in a better position to handle this ambiguity than GCLI.

> @@ +177,5 @@
> > +      description: gcli.lookup("breakaddlineFileDesc")
> > +    },
> > +    {
> > +      name: "line",
> > +      type: { name: "number", min: 0, step: 10 },
> 
> Shouldn't min be 1?

The reason I put 0 is that in some cases SpiderMonkey considers 0 to be the starting line, with a line count of 1. I think that these cases concern dynamically generated DOM event handlers, which don't make much sense for setting a breakpoint. It would be easy to find out if we had decompile() (bug 690377).

> For discussion - we could also do:
> 
>   type: {
>     name: "number",
>     min: 0,
>     max: function(args) {
>       return something.getNumberOfLinesInFile(args.file)
>     }
>     step: 10
>   },
> 
> (Or at least we could if GCLI passed args to max() - currently it doesn't,
> and I've been delaying fixing bug 722727 until it was needed.)
> 
> IF that bug was fixed, would it be easy to get the number of lines in a file?

Yes, as long as args.file is a resource type and contains a property with the number of lines of the file. Alternatively, with the current solution where GCLI gets the source list from the debugger, I could return startLine and lineCount along with the URL and let them affect min and max. 

Multiple script tags in an HTML file will complicate this, but at least we could have min=min(script1.startLine, script2.startLine, ...) and max=max(script1.startLine+script1.lineCount, script2.startLine+script2.lineCount, ...).

> @@ +183,5 @@
> > +    }
> > +  ],
> > +  returnType: "html",
> > +  exec: function(args, context) {
> > +    args.type = "line";
> 
> I was just thinking 'what if (for performance reasons) args was a proxy
> object which would calculate it's values on demand'.
> 
> And then you demonstrate why that could be risky.
> 
> I'm not sure what I think of this scenario. Who is in the right? Did GCLI
> pass you a something overly clever or did this code misuse GCLI resources?
> 
> Note: I'm not asking you to change this code (probably). I'm mulling the
> rights and wrongs of the situation.
> 
> It seems inevitable that people will do this kind of thing, so GCLI would
> probably be wrong to assume they wouldn't, I think...

What would be the risk with using a proxy? Can't proxies contain own properties that aren't present in their receivers? I admit that monkey patching an argument is not very tasteful, but I think you're right that it's pretty common behavior in JavaScript.
(In reply to Panos Astithas [:past] from comment #22)
> (In reply to Joe Walker from comment #21)
> > Comment on attachment 590648 [details] [diff] [review]
> > Working patch v5
> > 
> > Review of attachment 590648 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Most of this is theoretical, we should probably fix the min:0/1 thing
> > though, and ensure we're not doing anything disastrous in storing
> > breakpoints locally.
> > r+ with those out of the way.
> > 
> > ::: browser/devtools/webconsole/GcliCommands.jsm
> > @@ +105,5 @@
> > >      InspectorUI.openInspectorUI(args.node);
> > >    }
> > >  });
> > > +
> > > +let breakpoints = [];
> > 
> > This method of storing breakpoints was a bit of a hack for a POC - is there
> > a way we could use the debugger's store of breakpoints rather than
> > duplicating the list?
> 
> This is mostly a list of the front objects that correspond to breakpoint
> actors on the server. As such they are just plain old javascript objects
> (POJOs!) without any special lifetime requirements. On the other hand, the
> debugger UI currently does not maintain any list of breakpoints, since the
> relevant functionality is not there yet (bug 723069). When it does it may
> make sense to have GCLI tap into a facility provided by the debugger UI.
> 
> > What if breakpoints pre-exist from an earlier session?
> 
> That's another reasonable argument for having GCLI use a debugger
> UI-provided API, but we haven't even filed the bugs for a breakpoint store
> yet. Can we get this in and refactor once the necessary API is there?

Makes sense.

> > @@ +132,5 @@
> > > +
> > > +    let reply = gcli.lookup("breaklistIntro");
> > > +    reply += "<ol>";
> > > +    breakpoints.forEach(function(breakpoint) {
> > > +      reply += "<li>" + JSON.stringify(breakpoint) + "</li>";
> > 
> > Is JSON.stringify the best way we have of representing a breakpoint to a
> > user?
> 
> It seems OK to me for now, but I'm open to suggestions. Would you prefer
> something like "Line breakpoint at <url>:<line>"? When we get the breakpoint
> view (bug 723071) it would be nice to present the list in the same way for
> the sake of consistency.

Something like  "Line breakpoint at <url>:<line>" would be nicer.

Even better would be a table, with 4 columns: Type, URL, Line and Actions. Where the actions are 'locate' and 'remove', when clicked these run the command to open the source file at the specified line, or remove the breakpoint.

Shall I open a bug for that?

> > @@ +170,5 @@
> > > +            for each (let script in scriptsView.scriptLocations()) {
> > > +              files.push(script);
> > > +            }
> > > +          }
> > > +          return files;
> > 
> > Hmm. GCLI has another way to do this:
> > https://github.com/joewalker/gcli/blob/resource-709223/lib/gcli/types/
> > resource.js#L186
> > 
> > Your version looks like it's probably going to be more accurate? Please
> > could you take a look at the GCLI version and comment? I suppose the real
> > question is - what are the benefits of changing the GCLI code to match this?
> 
> I've already seen it and I plan on using it in the future. It's not going to
> work right now, because the debugger doesn't always know about the scripts
> loaded in the page, due to the lack of getAllScripts (bug 723563 and bug
> 697040). As it is, the user is bound to get noScript errors when the
> debugger's list doesn't mach GCLI's, unless GCLI asks the debugger for the
> list.
> 
> One other concern I have with the resource type is with HTML files
> containing more than one script, but the debugger is probably in a better
> position to handle this ambiguity than GCLI.

That makes sense.
Currently resource.js is designed to work on the web, but we're going to need a moz customized version fairly soon.

> > @@ +177,5 @@
> > > +      description: gcli.lookup("breakaddlineFileDesc")
> > > +    },
> > > +    {
> > > +      name: "line",
> > > +      type: { name: "number", min: 0, step: 10 },
> > 
> > Shouldn't min be 1?
> 
> The reason I put 0 is that in some cases SpiderMonkey considers 0 to be the
> starting line, with a line count of 1. I think that these cases concern
> dynamically generated DOM event handlers, which don't make much sense for
> setting a breakpoint. It would be easy to find out if we had decompile()
> (bug 690377).

Ah - I get it.

> > For discussion - we could also do:
> > 
> >   type: {
> >     name: "number",
> >     min: 0,
> >     max: function(args) {
> >       return something.getNumberOfLinesInFile(args.file)
> >     }
> >     step: 10
> >   },
> > 
> > (Or at least we could if GCLI passed args to max() - currently it doesn't,
> > and I've been delaying fixing bug 722727 until it was needed.)
> > 
> > IF that bug was fixed, would it be easy to get the number of lines in a file?
> 
> Yes, as long as args.file is a resource type and contains a property with
> the number of lines of the file. Alternatively, with the current solution
> where GCLI gets the source list from the debugger, I could return startLine
> and lineCount along with the URL and let them affect min and max. 
> 
> Multiple script tags in an HTML file will complicate this, but at least we
> could have min=min(script1.startLine, script2.startLine, ...) and
> max=max(script1.startLine+script1.lineCount,
> script2.startLine+script2.lineCount, ...).

There is also bug 659268 for a custom validation function, which could allow us to check that the line number was within a script tag.

> > @@ +183,5 @@
> > > +    }
> > > +  ],
> > > +  returnType: "html",
> > > +  exec: function(args, context) {
> > > +    args.type = "line";
> > 
> > I was just thinking 'what if (for performance reasons) args was a proxy
> > object which would calculate it's values on demand'.
> > 
> > And then you demonstrate why that could be risky.
> > 
> > I'm not sure what I think of this scenario. Who is in the right? Did GCLI
> > pass you a something overly clever or did this code misuse GCLI resources?
> > 
> > Note: I'm not asking you to change this code (probably). I'm mulling the
> > rights and wrongs of the situation.
> > 
> > It seems inevitable that people will do this kind of thing, so GCLI would
> > probably be wrong to assume they wouldn't, I think...
> 
> What would be the risk with using a proxy? Can't proxies contain own
> properties that aren't present in their receivers? I admit that monkey
> patching an argument is not very tasteful, but I think you're right that
> it's pretty common behavior in JavaScript.

I can't think of one right now - actually I've thought of 2, but then realized that they didn't quite work. But ask the question another way - are you sure this isn't going to go wrong? And if it is going to go wrong, it will probably happen in a bizarre and inexplicable way that is very implementation specific.

Either way, I think we agree that it's academic right now.

Thanks.
(In reply to Joe Walker from comment #23)
> (In reply to Panos Astithas [:past] from comment #22)
> > (In reply to Joe Walker from comment #21)
> > > @@ +132,5 @@
> > > > +
> > > > +    let reply = gcli.lookup("breaklistIntro");
> > > > +    reply += "<ol>";
> > > > +    breakpoints.forEach(function(breakpoint) {
> > > > +      reply += "<li>" + JSON.stringify(breakpoint) + "</li>";
> > > 
> > > Is JSON.stringify the best way we have of representing a breakpoint to a
> > > user?
> > 
> > It seems OK to me for now, but I'm open to suggestions. Would you prefer
> > something like "Line breakpoint at <url>:<line>"? When we get the breakpoint
> > view (bug 723071) it would be nice to present the list in the same way for
> > the sake of consistency.
> 
> Something like  "Line breakpoint at <url>:<line>" would be nicer.
> 
> Even better would be a table, with 4 columns: Type, URL, Line and Actions.
> Where the actions are 'locate' and 'remove', when clicked these run the
> command to open the source file at the specified line, or remove the
> breakpoint.
> 
> Shall I open a bug for that?

Yes, please.
Created attachment 594157 [details] [diff] [review]
Working patch v6

(carrying over the r+)
Changed the min line to 1 and improved the breakpoint list display as suggested.
Attachment #590648 - Attachment is obsolete: true
Attachment #594157 - Flags: review+
Created attachment 594158 [details] [diff] [review]
Diff between v5 and v6

...in case you want to glance over it.
Blocks: 723923
(In reply to Panos Astithas [:past] from comment #24)
> (In reply to Joe Walker from comment #23)
> > (In reply to Panos Astithas [:past] from comment #22)
> > > (In reply to Joe Walker from comment #21)
> > > > @@ +132,5 @@
> > > > > +
> > > > > +    let reply = gcli.lookup("breaklistIntro");
> > > > > +    reply += "<ol>";
> > > > > +    breakpoints.forEach(function(breakpoint) {
> > > > > +      reply += "<li>" + JSON.stringify(breakpoint) + "</li>";
> > > > 
> > > > Is JSON.stringify the best way we have of representing a breakpoint to a
> > > > user?
> > > 
> > > It seems OK to me for now, but I'm open to suggestions. Would you prefer
> > > something like "Line breakpoint at <url>:<line>"? When we get the breakpoint
> > > view (bug 723071) it would be nice to present the list in the same way for
> > > the sake of consistency.
> > 
> > Something like  "Line breakpoint at <url>:<line>" would be nicer.
> > 
> > Even better would be a table, with 4 columns: Type, URL, Line and Actions.
> > Where the actions are 'locate' and 'remove', when clicked these run the
> > command to open the source file at the specified line, or remove the
> > breakpoint.
> > 
> > Shall I open a bug for that?
> 
> Yes, please.

Bug 723923.
(In reply to Panos Astithas [:past] from comment #26)
> Created attachment 594158 [details] [diff] [review]
> Diff between v5 and v6
> 
> ...in case you want to glance over it.

Thanks.
Created attachment 594989 [details] [diff] [review]
Working patch v7

Rebased.
Attachment #594157 - Attachment is obsolete: true
Attachment #594158 - Attachment is obsolete: true
Attachment #594989 - Flags: review+
Created attachment 595427 [details] [diff] [review]
[in-fx-team] Working patch v8

Small fix for the test that broke after the recent merge.
Attachment #594989 - Attachment is obsolete: true
Attachment #595427 - Flags: review?(rcampbell)
Created attachment 595428 [details] [diff] [review]
Diff between v7 and v8

The changes from the previous version.

Try run:

https://tbpl.mozilla.org/?tree=Try&rev=e808fa2475c6
Attachment #595427 - Flags: review?(rcampbell) → review+
Status: NEW → ASSIGNED
Whiteboard: [land-in-fx-team]
Attachment #595427 - Attachment description: Working patch v8 → [in-fx-team] Working patch v8
https://hg.mozilla.org/integration/fx-team/rev/7a0c7a6339e0
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7a0c7a6339e0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.