Create some kind of chrome to turn source mapping on/off in the debugger

RESOLVED FIXED in Firefox 23

Status

DevTools
Debugger
P2
normal
RESOLVED FIXED
5 years ago
8 days ago

People

(Reporter: fitzgen, Assigned: past)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 23
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 6 obsolete attachments)

As of bug 765993 whether you are getting source mapped sources or actual sources is dependent on a pref, instead we should have some chrome to let the user turn this on and off.
Assignee: nobody → nfitzgerald
(Assignee)

Updated

5 years ago
Assignee: nfitzgerald → past
Status: NEW → ASSIGNED
Depends on: 772119
(Assignee)

Updated

5 years ago
Priority: -- → P2
(Assignee)

Comment 1

5 years ago
Created attachment 725168 [details] [diff] [review]
Patch v1

Works, but needs tests.
Attachment #725168 - Flags: feedback?(nfitzgerald)
(Assignee)

Comment 2

5 years ago
Created attachment 727548 [details] [diff] [review]
Patch v2

Now the stack frame list is updated correctly. Still needs tests.

I think I'll file a followup for displaying breakpoints after switching over to the other source representation, but we should either add a new protocol request, or use the sourcemaps library in the frontend to find the corresponding lines.

Also, followups for figuring out what to do with the variables view, watch expressions and conditional breakpoints (unless coffeescript expressions are always valid/untranslated JS expressions?).
Attachment #725168 - Attachment is obsolete: true
Attachment #725168 - Flags: feedback?(nfitzgerald)
Attachment #727548 - Flags: feedback?(nfitzgerald)
(Assignee)

Comment 3

5 years ago
Created attachment 727770 [details] [diff] [review]
Patch v3

Simplified onReconfigure() and added a leaky test.
Not on purpose.
Attachment #727548 - Attachment is obsolete: true
Attachment #727548 - Flags: feedback?(nfitzgerald)
Attachment #727770 - Flags: feedback?(nfitzgerald)
Comment on attachment 727770 [details] [diff] [review]
Patch v3

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

::: browser/devtools/debugger/debugger-view.js
@@ +56,5 @@
>      this.Variables.searchPlaceholder = L10N.getStr("emptyVariablesFilterText");
>      this.Variables.emptyText = L10N.getStr("emptyVariablesText");
>      this.Variables.onlyEnumVisible = Prefs.variablesOnlyEnumVisible;
>      this.Variables.searchEnabled = Prefs.variablesSearchboxVisible;
> +    this.Variables.originalSource = Prefs.sourceMapsEnabled;

Why are you attaching this property here (just curious)? Isn't it a bit premature, since nothing is happening with the variables view yet?
(Assignee)

Comment 5

5 years ago
(In reply to Victor Porof [:vp] from comment #4)
> ::: browser/devtools/debugger/debugger-view.js
> @@ +56,5 @@
> >      this.Variables.searchPlaceholder = L10N.getStr("emptyVariablesFilterText");
> >      this.Variables.emptyText = L10N.getStr("emptyVariablesText");
> >      this.Variables.onlyEnumVisible = Prefs.variablesOnlyEnumVisible;
> >      this.Variables.searchEnabled = Prefs.variablesSearchboxVisible;
> > +    this.Variables.originalSource = Prefs.sourceMapsEnabled;
> 
> Why are you attaching this property here (just curious)? Isn't it a bit
> premature, since nothing is happening with the variables view yet?

No reason, plain oversight, thanks.
(Assignee)

Comment 6

5 years ago
Created attachment 729772 [details] [diff] [review]
Patch v4

A couple of fixes, but the test still leaks.
Attachment #727770 - Attachment is obsolete: true
Attachment #727770 - Flags: feedback?(nfitzgerald)
(Assignee)

Comment 7

5 years ago
I've been sidetracked from the leak today, trying to figure out why browser_dbg_bug740825_conditional-breakpoints-01.js fails intermittently (more often than not) with this patch. I don't see any relation to the failure, but the test depends on Debugger:AfterFramesCleared, which fires after a timeout, and furthermore conditional breakpoints cause extra protocol requests (due to bug 812172), so it must somehow affect the timing of those events.

I'm seriously considering fixing bug 812172, just to avoid this quagmire.
(Assignee)

Comment 8

5 years ago
Created attachment 731319 [details] [diff] [review]
Patch v5

I finally fixed the intermittent failures in the conditional breakpoints test. Getting back to the leak now.
Attachment #729772 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 731362 [details] [diff] [review]
Patch v6

Some cleanups, some arrow functions and sending of the sourcemap pref in chrome debugging, too. Because I might want to port the devtools to CoffeeScript next week.
Attachment #731319 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 738032 [details] [diff] [review]
Patch v1

Apparently the leaks I was experiencing were caused by the main source maps patch in bug 772119. Rebased, and now all devtools xpcshell and b-c tests, as well as browser m-oth tests pass.

Rob, this is the UI part that was demoed during the work week. The toggle in the gear menu switches between the original and generated sources.
Attachment #731362 - Attachment is obsolete: true
Attachment #738032 - Flags: review?(rcampbell)
Attachment #738032 - Flags: review?(rcampbell) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/bd899fcceeab
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bd899fcceeab
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

8 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.