Last Comment Bug 723069 - The debugger should allow breakpoints to be set in the embedded editor
: The debugger should allow breakpoints to be set in the embedded editor
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
Depends on: 707987 734641
Blocks: minotaur 723071
  Show dependency treegraph
 
Reported: 2012-02-01 05:15 PST by Panos Astithas [:past]
Modified: 2012-03-10 07:02 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip 1 (15.43 KB, patch)
2012-02-23 12:11 PST, Mihai Sucan [:msucan]
past: feedback+
Details | Diff | Splinter Review
wip 2 (18.01 KB, patch)
2012-02-24 12:04 PST, Mihai Sucan [:msucan]
past: feedback+
Details | Diff | Splinter Review
proposed patch (33.76 KB, patch)
2012-03-02 12:40 PST, Mihai Sucan [:msucan]
past: review+
Details | Diff | Splinter Review
[in-fx-team] updated patch (35.05 KB, patch)
2012-03-05 10:01 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-02-01 05:15:10 PST
After bug 707987 lands, the debugger UI should expose this functionality to the user. Setting and clearing breakpoints should not require a GCLI command.
Comment 1 Mihai Sucan [:msucan] 2012-02-23 12:11:11 PST
Created attachment 600127 [details] [diff] [review]
wip 1

This is the WIP patch. Everything should work fine, except I didn't write the code comments and I didn't write the test.

Asking for feedback since it's my first patch in the debugger land. Please let me know if the patch is fine. Thanks!

I found what was breaking the debugger yesterday. Please also test if it works fine for you. All tests pass, so, hopefully, I didn't introduce regressions.
Comment 2 Panos Astithas [:past] 2012-02-24 02:06:00 PST
Comment on attachment 600127 [details] [diff] [review]
wip 1

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

I love it! feedback++! Note that bug 690404 might change a couple of things this patch depends on, but nothing big. Some general comments below.

It would be a good idea I think to remove all breakpoints in DP_close. See if you get any (new) leaks in tests without that.

I get this error sometimes when I remove a breakpoint or click on a line number:
JavaScript error: chrome://browser/content/orion.js, line 2676: invalid 'instanceof' operand mProjectionTextModel.ProjectionTextModel

Also, a probably unrelated bug: even though the editor is read-only, I can add newlines in it at will. I tried clicking on every key on my keyboard, but ENTER was the only one that modified the editor.

At one point I saw red markers in the overview ruler (probably when I added newlines). What do these stand for? We haven't integrated their linter, have we?

The breakpoint icon is ugly, it may be that it does not blend well with our background. I get the feeling that it was designed for a darker background and the outer circle shows badly.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +65,5 @@
>  }
>  
>  DebuggerPane.prototype = {
> +  _handlingEditorBreakpointEvent: false,
> +  _externalBreakpointsChange: false,

Wouldn't it be nice to make these two more homogenous? _internalBreakpointsChange or _handlingExternalBreakpointEvent maybe?

@@ +187,5 @@
> +    this.activeThread.setBreakpoint(aLocation, function(aResponse, aBpClient) {
> +      if (!aResponse.error) {
> +        breakpoint.id = aBpClient.actor;
> +        breakpoint.client = aBpClient;
> +        this._breakpoints[breakpoint.id] = breakpoint;

If I understand this correctly, you don't need the container breakpoint object. Just use aBpClient.actor as the key and aBpClient as the value in the map. Then you can pass the client to removeBreakpoint to avoid looking it up twice.

@@ +216,5 @@
> +    }
> +
> +    delete this._breakpoints[aBreakpointId];
> +
> +    if (!this._handlingEditorBreakpointEvent) {

Shouldn't this guard the whole removal process (cache+editor+protocol op)? That way at least the editor will not be out of sync with the cache and server.

@@ +227,5 @@
> +    }
> +
> +    try {
> +      breakpoint.client.remove(aCallback);
> +    } catch (ex) { }

This can't throw.

@@ +231,5 @@
> +    } catch (ex) { }
> +  },
> +
> +  getBreakpoints: function DP_getBreakpoints() {
> +    return this._breakpoints;

What is the benefit of not making this.breakpoints public? Callers can mess with the returned array anyway.

@@ +235,5 @@
> +    return this._breakpoints;
> +  },
> +
> +  getBreakpoint: function DP_getBreakpoint(aUrl, aLine) {
> +    for each (let breakpoint in this._breakpoints) {

What, no for...of? :-P

::: browser/devtools/debugger/debugger-view.js
@@ +1114,5 @@
>  
> +   selectedScript: function DVS_selectedScript() {
> +    return this._scripts.selectedItem ?
> +           this._scripts.selectedItem.value : null;
> +   },

This should be a getter and DebuggerView.Scripts.selected seems simpler.
Comment 3 Mihai Sucan [:msucan] 2012-02-24 07:59:21 PST
(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 600127 [details] [diff] [review]
> wip 1
> 
> Review of attachment 600127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I love it! feedback++! Note that bug 690404 might change a couple of things
> this patch depends on, but nothing big. Some general comments below.

Thanks!


> It would be a good idea I think to remove all breakpoints in DP_close. See
> if you get any (new) leaks in tests without that.

I will remove them. I just kept the same approach as the one in GCLI.


> I get this error sometimes when I remove a breakpoint or click on a line
> number:
> JavaScript error: chrome://browser/content/orion.js, line 2676: invalid
> 'instanceof' operand mProjectionTextModel.ProjectionTextModel

I didn't see this error. Can you please provide a STR?

> Also, a probably unrelated bug: even though the editor is read-only, I can
> add newlines in it at will. I tried clicking on every key on my keyboard,
> but ENTER was the only one that modified the editor.

Sounds like a bug. Will test and fix. Thanks!

> At one point I saw red markers in the overview ruler (probably when I added
> newlines). What do these stand for? We haven't integrated their linter, have
> we?

No linter. You also have TODOs marked in the overview ruler. Just click them to jump to them.


> The breakpoint icon is ugly, it may be that it does not blend well with our
> background. I get the feeling that it was designed for a darker background
> and the outer circle shows badly.

This is from upstream. I think we should keep it as is now. The debugger redesign should include the redesign of the breakpoint icon. Paul or shorlander can do this.

> ::: browser/devtools/debugger/DebuggerUI.jsm
> @@ +65,5 @@
> >  }
> >  
> >  DebuggerPane.prototype = {
> > +  _handlingEditorBreakpointEvent: false,
> > +  _externalBreakpointsChange: false,
> 
> Wouldn't it be nice to make these two more homogenous?
> _internalBreakpointsChange or _handlingExternalBreakpointEvent maybe?

Sure. Will do!

> @@ +187,5 @@
> > +    this.activeThread.setBreakpoint(aLocation, function(aResponse, aBpClient) {
> > +      if (!aResponse.error) {
> > +        breakpoint.id = aBpClient.actor;
> > +        breakpoint.client = aBpClient;
> > +        this._breakpoints[breakpoint.id] = breakpoint;
> 
> If I understand this correctly, you don't need the container breakpoint
> object. Just use aBpClient.actor as the key and aBpClient as the value in
> the map. Then you can pass the client to removeBreakpoint to avoid looking
> it up twice.

I need to keep the bp.line, bp.url and possibly the bp.condition (in the future). Afaik, the bpClient object doesn't include them.

I need this for GCLI breakpoints list and for when the editor needs to be updated to display the breakpoints of the selected script.


> @@ +216,5 @@
> > +    }
> > +
> > +    delete this._breakpoints[aBreakpointId];
> > +
> > +    if (!this._handlingEditorBreakpointEvent) {
> 
> Shouldn't this guard the whole removal process (cache+editor+protocol op)?
> That way at least the editor will not be out of sync with the cache and
> server.

No. This is meant to allow GCLI or other code to add/delete breakpoints and have the source editor updated as well. If the source editor does the action itself, then _handlingEditorBreakpointEvent is set to true to not create a loop - to guard against adding/deleting again from the editor.


> @@ +227,5 @@
> > +    }
> > +
> > +    try {
> > +      breakpoint.client.remove(aCallback);
> > +    } catch (ex) { }
> 
> This can't throw.

Will fix.

> @@ +231,5 @@
> > +    } catch (ex) { }
> > +  },
> > +
> > +  getBreakpoints: function DP_getBreakpoints() {
> > +    return this._breakpoints;
> 
> What is the benefit of not making this.breakpoints public? Callers can mess
> with the returned array anyway.

Good point. This code evolved differently and I forgot to make _breakpoints public.

> @@ +235,5 @@
> > +    return this._breakpoints;
> > +  },
> > +
> > +  getBreakpoint: function DP_getBreakpoint(aUrl, aLine) {
> > +    for each (let breakpoint in this._breakpoints) {
> 
> What, no for...of? :-P

Hehe. I used what I am used to. ;)

> ::: browser/devtools/debugger/debugger-view.js
> @@ +1114,5 @@
> >  
> > +   selectedScript: function DVS_selectedScript() {
> > +    return this._scripts.selectedItem ?
> > +           this._scripts.selectedItem.value : null;
> > +   },
> 
> This should be a getter and DebuggerView.Scripts.selected seems simpler.

Sounds good!


Thanks for your feedback! Will update the patch.
Comment 4 Panos Astithas [:past] 2012-02-24 10:18:45 PST
(In reply to Mihai Sucan [:msucan] from comment #3)
> (In reply to Panos Astithas [:past] from comment #2)
> > I get this error sometimes when I remove a breakpoint or click on a line
> > number:
> > JavaScript error: chrome://browser/content/orion.js, line 2676: invalid
> > 'instanceof' operand mProjectionTextModel.ProjectionTextModel
> 
> I didn't see this error. Can you please provide a STR?

- Visit http://orion.eclipse.org/examples/textview/demo.html
- Select textView.js
- Set a breakpoint in line 40.
- Remove that breakpoint.

A few moments later I get the above error in the shell.

> > At one point I saw red markers in the overview ruler (probably when I added
> > newlines). What do these stand for? We haven't integrated their linter, have
> > we?
> 
> No linter. You also have TODOs marked in the overview ruler. Just click them
> to jump to them.

I saw those, they are blue. I was referring to red markers, but it's not important anyway.

> > The breakpoint icon is ugly, it may be that it does not blend well with our
> > background. I get the feeling that it was designed for a darker background
> > and the outer circle shows badly.
> 
> This is from upstream. I think we should keep it as is now. The debugger
> redesign should include the redesign of the breakpoint icon. Paul or
> shorlander can do this.

Definitely, not fodder for this bug!

> > @@ +187,5 @@
> > > +    this.activeThread.setBreakpoint(aLocation, function(aResponse, aBpClient) {
> > > +      if (!aResponse.error) {
> > > +        breakpoint.id = aBpClient.actor;
> > > +        breakpoint.client = aBpClient;
> > > +        this._breakpoints[breakpoint.id] = breakpoint;
> > 
> > If I understand this correctly, you don't need the container breakpoint
> > object. Just use aBpClient.actor as the key and aBpClient as the value in
> > the map. Then you can pass the client to removeBreakpoint to avoid looking
> > it up twice.
> 
> I need to keep the bp.line, bp.url and possibly the bp.condition (in the
> future). Afaik, the bpClient object doesn't include them.
> 
> I need this for GCLI breakpoints list and for when the editor needs to be
> updated to display the breakpoints of the selected script.

You can patch the BreakpointClient object, too, it's just a regular object. Or, better yet, we could also add these fields on construction, in TC_setBreakpoint.

> > @@ +216,5 @@
> > > +    }
> > > +
> > > +    delete this._breakpoints[aBreakpointId];
> > > +
> > > +    if (!this._handlingEditorBreakpointEvent) {
> > 
> > Shouldn't this guard the whole removal process (cache+editor+protocol op)?
> > That way at least the editor will not be out of sync with the cache and
> > server.
> 
> No. This is meant to allow GCLI or other code to add/delete breakpoints and
> have the source editor updated as well. If the source editor does the action
> itself, then _handlingEditorBreakpointEvent is set to true to not create a
> loop - to guard against adding/deleting again from the editor.

Right, the scenario I'm envisioning is this: user removes a breakpoint via GCLI and at the same time clicks on the editor to add a breakpoint (or GCLI takes a long time to complete due to high CPU load, user gets tired of waiting and clicks to remove but misses by 1 line). In this case we remove the cache entry, can't modify the editor because the flag is raised and proceed to remove the breakpoint from the server. Now the editor is out of sync with the cache and the server, isn't it?
Comment 5 Mihai Sucan [:msucan] 2012-02-24 10:40:00 PST
(In reply to Panos Astithas [:past] from comment #4)
> (In reply to Mihai Sucan [:msucan] from comment #3)
> > (In reply to Panos Astithas [:past] from comment #2)
> > > I get this error sometimes when I remove a breakpoint or click on a line
> > > number:
> > > JavaScript error: chrome://browser/content/orion.js, line 2676: invalid
> > > 'instanceof' operand mProjectionTextModel.ProjectionTextModel
> > 
> > I didn't see this error. Can you please provide a STR?
> 
> - Visit http://orion.eclipse.org/examples/textview/demo.html
> - Select textView.js
> - Set a breakpoint in line 40.
> - Remove that breakpoint.
> 
> A few moments later I get the above error in the shell.

Will test. Thanks for the STR.


> > > At one point I saw red markers in the overview ruler (probably when I added
> > > newlines). What do these stand for? We haven't integrated their linter, have
> > > we?
> > 
> > No linter. You also have TODOs marked in the overview ruler. Just click them
> > to jump to them.
> 
> I saw those, they are blue. I was referring to red markers, but it's not
> important anyway.

Weird. Can you please provide a STR for this as well?


> > I need this for GCLI breakpoints list and for when the editor needs to be
> > updated to display the breakpoints of the selected script.
> 
> You can patch the BreakpointClient object, too, it's just a regular object.
> Or, better yet, we could also add these fields on construction, in
> TC_setBreakpoint.

Ah, fancy. Then I'll look into patching the BreakpointClient to hold the URL and the line number, so we just store bpclients in the .breakpoints list.


> > > @@ +216,5 @@
> > > > +    }
> > > > +
> > > > +    delete this._breakpoints[aBreakpointId];
> > > > +
> > > > +    if (!this._handlingEditorBreakpointEvent) {
> > > 
> > > Shouldn't this guard the whole removal process (cache+editor+protocol op)?
> > > That way at least the editor will not be out of sync with the cache and
> > > server.
> > 
> > No. This is meant to allow GCLI or other code to add/delete breakpoints and
> > have the source editor updated as well. If the source editor does the action
> > itself, then _handlingEditorBreakpointEvent is set to true to not create a
> > loop - to guard against adding/deleting again from the editor.
> 
> Right, the scenario I'm envisioning is this: user removes a breakpoint via
> GCLI and at the same time clicks on the editor to add a breakpoint (or GCLI
> takes a long time to complete due to high CPU load, user gets tired of
> waiting and clicks to remove but misses by 1 line). In this case we remove
> the cache entry, can't modify the editor because the flag is raised and
> proceed to remove the breakpoint from the server. Now the editor is out of
> sync with the cache and the server, isn't it?

Yeah, I looked into this now. So, how I understand: when GCLI adds a breakpoint, no flag so things continue wotk correctly. Once the response is received, then locally the editor is updated to show the breakpoint, and this is when the flag is set before editor.add/removeBreakpoint() and removed after the call. So this can't break even if the breakpoint change operation takes a long time - when it happens from GCLI.

What can break is when things happen from the editor: _handlingEditorBreakpointEvent is set to true before the call to DP_addBreakpoint() and to false after that. Which is broken, actually, since DP_addBreakpoint() is async, the message is only sent to the server, and if the response takes a lot of time to come back, then _handlingEditorBreakpointEvent is set back to false before the response arrives, meaning that the callback adds the breakpoint again to the editor.

Will fix! Thanks for pointing out the problem.
Comment 6 Panos Astithas [:past] 2012-02-24 10:49:06 PST
(In reply to Mihai Sucan [:msucan] from comment #5)
> (In reply to Panos Astithas [:past] from comment #4)
> > > > At one point I saw red markers in the overview ruler (probably when I added
> > > > newlines). What do these stand for? We haven't integrated their linter, have
> > > > we?
> > > 
> > > No linter. You also have TODOs marked in the overview ruler. Just click them
> > > to jump to them.
> > 
> > I saw those, they are blue. I was referring to red markers, but it's not
> > important anyway.
> 
> Weird. Can you please provide a STR for this as well?

Sorry, I only saw it once, after adding newlines in the editor.
Comment 7 Mihai Sucan [:msucan] 2012-02-24 12:04:56 PST
Created attachment 600476 [details] [diff] [review]
wip 2

Addressed your comments. Please let me know if I properly addressed them and any changes you might want.

Wrt. source editor bugs: I took notes and added to my TODO list to open separate bugs and submit patches to fix the problems you mentioned.

Thank you!

(no test yet - I have to do some more feedback rounds for some other patches)
Comment 8 Panos Astithas [:past] 2012-02-28 02:13:04 PST
Comment on attachment 600476 [details] [diff] [review]
wip 2

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

Want!
Comment 9 Mihai Sucan [:msucan] 2012-03-02 12:40:03 PST
Created attachment 602441 [details] [diff] [review]
proposed patch

Various fixes and a test.

Looking forward to your review! Thanks!
Comment 10 Panos Astithas [:past] 2012-03-05 02:16:38 PST
Comment on attachment 602441 [details] [diff] [review]
proposed patch

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

Good stuff! Some minor issues only. Let's get this baby in pronto!

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +65,4 @@
>  }
>  
>  DebuggerPane.prototype = {
> +  _skipEditorBreakpointChange: false,

Comment please.

@@ +245,5 @@
> +   *          - aResponseError - if there was any error.
> +   */
> +  addBreakpoint: function DP_addBreakpoint(aLocation, aCallback) {
> +    this._addBreakpoint(aLocation, aCallback);
> +  },

What is the point of this function? Why not just make _addBreakpoint public?

@@ +294,5 @@
> +   *        object which holds the url and line properties.
> +   */
> +  removeBreakpoint: function DP__removeBreakpoint(aBreakpoint, aCallback) {
> +    this._removeBreakpoint(aBreakpoint, aCallback);
> +  },

Same here.

::: browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js
@@ +5,5 @@
> + * Test the debugger breakpoint API and connection to the source editor.
> + */
> +
> +const TAB_URL = "http://example.com/browser/browser/devtools/debugger/" +
> +                "test/browser_dbg_script-switching.html";

You can write this now as EXAMPLE_URL + "browser_dbg_script-switching.html".

@@ +71,5 @@
> +  let editorBreakpointChanges = 0;
> +
> +  function onEditorBreakpointAddFirst(aEvent)
> +  {
> +    dump("\n\n onEditorBreakpointAddFirst \n\n");

Forgotten dump.

@@ +87,5 @@
> +  }
> +
> +  function onBreakpointAddFirst(aBreakpointClient, aResponseError)
> +  {
> +    dump("\n\n onBreakpointAddFirst '" + aBreakpointClient + "' '" + aResponseError + "' \n\n");

...and another one.

@@ +162,5 @@
> +    // is added to a script that is not currently selected.
> +    gEditor.removeEventListener(SourceEditor.EVENTS.BREAKPOINT_CHANGE,
> +                                onEditorBreakpointAddBackgroundTrap);
> +    editorBreakpointChanges++;
> +    ok(!aEvent, "breakpoint2 must not be added to the editor");

ok(false, "...") might be more explicit and bulletproof.

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +301,5 @@
> +
> +    let breakpoints = dbg.breakpoints;
> +    let id = Object.keys(dbg.breakpoints)[args.breakid];
> +    if (!id || !(id in breakpoints)) {
> +      return gcli.lookup("breakdelRemoved");

It would be better not to lie in this case and inform the user that no breakpoint was found.
Comment 11 Mihai Sucan [:msucan] 2012-03-05 10:01:38 PST
Created attachment 602951 [details] [diff] [review]
[in-fx-team] updated patch

Thanks for your r+ Panos!

Updated the patch as requested.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=b144a212454f
Comment 12 Panos Astithas [:past] 2012-03-06 03:42:19 PST
https://hg.mozilla.org/integration/fx-team/rev/837c55af9fb9
Comment 13 Rob Campbell [:rc] (:robcee) 2012-03-08 06:43:35 PST
https://hg.mozilla.org/mozilla-central/rev/837c55af9fb9

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