Last Comment Bug 683503 - GCLI needs commands to control the debugger
: GCLI needs commands to control the debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Panos Astithas [:past]
:
Mentors:
: 683505 (view as bug list)
Depends on: 697762
Blocks: 723923
  Show dependency treegraph
 
Reported: 2011-08-31 07:40 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-02-10 07:27 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
some old code that pretended to work quite well (6.00 KB, text/plain)
2011-11-09 06:29 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details
Working patch (7.37 KB, patch)
2011-12-05 09:55 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v2 (11.89 KB, patch)
2011-12-06 09:09 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v3 (14.03 KB, patch)
2011-12-07 09:50 PST, Panos Astithas [:past]
jwalker: review+
Details | Diff | Splinter Review
Working patch v4 (13.99 KB, patch)
2012-01-10 07:15 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v5 (13.99 KB, patch)
2012-01-23 00:53 PST, Panos Astithas [:past]
dcamp: review+
jwalker: review+
Details | Diff | Splinter Review
Working patch v6 (14.15 KB, patch)
2012-02-03 06:17 PST, Panos Astithas [:past]
past: review+
Details | Diff | Splinter Review
Diff between v5 and v6 (3.22 KB, patch)
2012-02-03 06:19 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v7 (14.31 KB, patch)
2012-02-07 04:33 PST, Panos Astithas [:past]
past: review+
Details | Diff | Splinter Review
[in-fx-team] Working patch v8 (14.37 KB, patch)
2012-02-08 08:55 PST, Panos Astithas [:past]
jwalker: review+
Details | Diff | Splinter Review
Diff between v7 and v8 (1.84 KB, patch)
2012-02-08 08:57 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-08-31 07:40:53 PDT
This command allows adding, listing, and removing breakpoinst
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-09 06:28:46 PST
*** Bug 683505 has been marked as a duplicate of this bug. ***
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-09 06:29:57 PST
Created attachment 573178 [details]
some old code that pretended to work quite well
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-11 07:35:00 PST
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
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 10:00:01 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 5 Panos Astithas [:past] 2011-12-05 09:55:32 PST
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.
Comment 6 Panos Astithas [:past] 2011-12-06 07:49:42 PST
This patch obviously depends on the debugger code.
Comment 7 Panos Astithas [:past] 2011-12-06 09:09:11 PST
Created attachment 579325 [details] [diff] [review]
Working patch v2

Available scripts now auto-complete, and there is a half-finished test in there.
Comment 8 Panos Astithas [:past] 2011-12-07 09:50:15 PST
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?).
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-09 01:59:27 PST
(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 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-09 11:26:49 PST
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
Comment 11 Panos Astithas [:past] 2011-12-12 01:08:16 PST
(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 12 Panos Astithas [:past] 2011-12-12 01:12:36 PST
Comment on attachment 579727 [details] [diff] [review]
Working patch v3

We also need a devtools peer review.
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-12 02:21:03 PST
(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!
Comment 14 Panos Astithas [:past] 2011-12-12 02:47:44 PST
(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.
Comment 15 Panos Astithas [:past] 2012-01-10 07:15:04 PST
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.
Comment 16 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-13 09:27:27 PST
Triage. Filter on PEGASUS.
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-13 09:30:01 PST
Triage. Filter on PEGASUS.
Comment 18 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-13 09:31:37 PST
Triage. Filter on PEGASUS.
Comment 19 Panos Astithas [:past] 2012-01-23 00:53:48 PST
Created attachment 590648 [details] [diff] [review]
Working patch v5

Rebased.
Comment 20 Dave Camp (:dcamp) 2012-02-02 18:04:39 PST
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...)
Comment 21 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-03 00:13:09 PST
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...
Comment 22 Panos Astithas [:past] 2012-02-03 02:54:31 PST
(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.
Comment 23 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-03 03:35:31 PST
(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.
Comment 24 Panos Astithas [:past] 2012-02-03 05:33:44 PST
(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.
Comment 25 Panos Astithas [:past] 2012-02-03 06:17:40 PST
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.
Comment 26 Panos Astithas [:past] 2012-02-03 06:19:16 PST
Created attachment 594158 [details] [diff] [review]
Diff between v5 and v6

...in case you want to glance over it.
Comment 27 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-03 06:52:04 PST
(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.
Comment 28 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-03 06:53:30 PST
(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.
Comment 29 Panos Astithas [:past] 2012-02-07 04:33:02 PST
Created attachment 594989 [details] [diff] [review]
Working patch v7

Rebased.
Comment 30 Panos Astithas [:past] 2012-02-08 08:55:55 PST
Created attachment 595427 [details] [diff] [review]
[in-fx-team] Working patch v8

Small fix for the test that broke after the recent merge.
Comment 31 Panos Astithas [:past] 2012-02-08 08:57:27 PST
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
Comment 32 Panos Astithas [:past] 2012-02-10 02:07:15 PST
https://hg.mozilla.org/integration/fx-team/rev/7a0c7a6339e0
Comment 33 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-10 07:27:57 PST
https://hg.mozilla.org/mozilla-central/rev/7a0c7a6339e0

Note You need to log in before you can comment on or make changes to this bug.