Panos Astithas (he/him) [:past] (please ni?) Reporter
	Description • 13 years ago

Setting a breakpoint in a line without code is automatically handled by the debugger by skipping forward to the first available line, returning an actualLocation property in the response. The debugger UI and editor should reflect that.

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 1 • 13 years ago

Jim suggested using an animation to slide the icon to the proper line, which sounds splendid!

	Anton Kovalyov (:anton) Assignee
	Comment 2 • 12 years ago

Spent today going through the debugger code, looking how things are done. As a result, taking this bug to make myself familiar with breakpoints and such in more practical sense.

	Anton Kovalyov (:anton) Assignee
	Comment 3 • 12 years ago

Spent way too much time on this patch. Partially because there are no docs so I was learning with scratchpad and generous usage of |dump| function. But mostly because I couldn't figure out why breakpoints don't go away sometimes.

It turns out that you can add unlimited amount of breakpoints to the same line with the same handler[1] and that created duplicates because we can't check if breakpoint is already there when the line is nonsensical (we have to wait for our callback to be called with |actualLocation| property).

My patch moves the visual breakpoint where needed and handles all duplicates. Asking for feedback to make sure I'm not too far off. After that I'll work on tests and animation.

[1] - https://wiki.mozilla.org/Debugger (see setBreakpoint)

	Mihai Sucan [:msucan]
	Comment 4 • 12 years ago

Comment on attachment 665222 [details] [diff] [review]
Move breakpoint to the actual line

This looks good to me, but I don't know the debugger UI code very well. I'll let Panos take this patch.

Thanks!

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 5 • 12 years ago

Comment on attachment 665222 [details] [diff] [review]
Move breakpoint to the actual line

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1726,5 @@
> +    for each (let breakpoint in this.store) {
> +      if (breakpoint.location.url == aUrl && breakpoint.location.line == aLine) {
> +        breakpoints.push(breakpoint);
> +      }
> +    }

for-each...in statement is deprecated as part of E4X.
So it's better to use other way; for-of statement, or simple for statement.

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 6 • 12 years ago

Comment on attachment 665222 [details] [diff] [review]
Move breakpoint to the actual line

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

Yes, this looks fine at first glance.

	Anton Kovalyov (:anton) Assignee
	Comment 7 • 12 years ago

So I was working on and off for a day on trying to make breakpoint elements to fade but so far I don't think it is (easily) possible. I tried digging into Orion source and getting a link for an element from there but got lost in numerous events and callbacks that delegate actual actions one to another. Then Mihai told me on IRC that I should be looking at source-editor-orion.jsm instead and I found out how to change the class name:

> var ann = editor._getAnnotationsByType("breakpoint", 0,
>   editor.getCharCount())[0];
>
> ann.style.styleClass = "removed";
> editor._annotationModel.modifyAnnotation(ann);

CSS looks like this:

> .annotation.breakpoint {
>   transition: opacity 2s ease-in-out;
>   -moz-transition: opacity 2s ease-in-out;
>   opacity: 1;
> }    

> .annotation.breakpoint.removed {
>   opacity: 0;
> }

However, later I realized that |modifyAnnotation| kills the old element and replaces it with a new one making CSS transitions impossible. Is there some other way to either get a DOM reference to an annotation or update an annotation element without deleting it?

	Mihai Sucan [:msucan]
	Comment 8 • 12 years ago

Unfortunately, it seems you can't do that fade easily - without changes in Orion. Just don't bother about the fade.

	Anton Kovalyov (:anton) Assignee
	Comment 9 • 12 years ago

Added a test for this change. It sucks that we can't do any kind of animation/transformation in the ruler.

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 10 • 12 years ago

Comment on attachment 668682 [details] [diff] [review]
Move breakpoint to the actual line

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

Looks good, just a couple of minor things. I'll file a followup for the case of adjusting the position of breakpoints on scripts not yet present in the debugger.

::: browser/devtools/debugger/test/browser_dbg_bug737803_editor_actual_location.js
@@ +74,5 @@
> +  }
> +
> +  function onBreakpointAdd(aBpClient) {
> +    is(aBpClient.location.url, gScripts.selected, "URL is the same");
> +    is(aBpClient.location.line, 6, "Line numbers is new");

Typo: "Line number"

::: browser/devtools/debugger/test/test-script-switching-02.js
@@ +6,5 @@
>    eval("debugger;");
> +  function foo() {}
> +  if (true) {
> +    foo();
> +  }

What is the point of this change?

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 11 • 12 years ago

One refactoring I would like us to do at some point, is to move the breakpoint store from the controller to the client, so that these smarts can be reused by Firebug et al.

	Victor Porof [:vporof][:vp]
	Comment 12 • 12 years ago

(In reply to Panos Astithas [:past] from comment #11)
> One refactoring I would like us to do at some point, is to move the
> breakpoint store from the controller to the client, so that these smarts can
> be reused by Firebug et al.

A splendid idea.

	Anton Kovalyov (:anton) Assignee
	Comment 13 • 12 years ago

> What is the point of this change?

Some tests were testing breakpoint by setting them on a nonsensical line and checking the line number afterwards. And since my patch changes the line number to reflect |actualLocation| these tests were failing. Basically, these tests needed at least three valid lines to set breakpoints on. This change adds these lines.

	Anton Kovalyov (:anton) Assignee
	Comment 14 • 12 years ago

Fixed a typo.

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 15 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/586a2ae61c95

	Tim Taubert [:ttaubert] (inactive)
	Comment 16 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/586a2ae61c95