Closed Bug 1288511 Opened 4 years ago Closed 4 years ago

Land new debugger behind a pref

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jlong, Assigned: jlong)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

We are developing the new debugger on github, and we intend to land it as a webpack-compiled bundle as an experiment. It will be behind a pref and we won't market it; we just want to land it early so we can figure things out over the next month or so.
An astute mozillian will note that initially the bundle a bit large, but that's because we include both React and CodeMirror. One of the first things we plan to do is figure out how to load both of those libraries from mozilla-central (shared with the rest of devtools), so we will solve that quickly. Sharing those modules will solve a bunch of compat problems (we'll automatically get CodeMirror themes/modes/etc).
Assignee: nobody → jlong
Attached patch 1288511.patch (obsolete) — Splinter Review
Big patch. We will substantially reduce the size of it by not bundling in react, codemirror, and other common deps. 

This works, but I need to fix a few other toolbox integration points.
Attached patch 1288511.patch (obsolete) — Splinter Review
I think this is ready to go. Going to push a try run and find someone to review it.
Attached patch 1288511.patchSplinter Review
Attachment #8774460 - Attachment is obsolete: true
Attachment #8777406 - Attachment is obsolete: true
Attachment #8777409 - Flags: review?(jsantell)
Comment on attachment 8777409 [details] [diff] [review]
1288511.patch

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

Reviewing for the components that wires everything up, not the actual new debugger code, but looks good!

::: devtools/client/debugger/new/index.html
@@ +1,1 @@
> +<!DOCTYPE html>

nit: missing license info

::: devtools/client/debugger/new/panel.js
@@ +1,1 @@
> +

nit: missing license info

::: devtools/client/shared/view-source.js
@@ +57,5 @@
>    let debuggerAlreadyOpen = toolbox.getPanel("jsdebugger");
>    let { panelWin: dbg } = yield toolbox.loadTool("jsdebugger");
>  
> +  // New debugger frontend
> +  if(Services.prefs.getBoolPref("devtools.debugger.new-debugger-frontend")) {

nit space after `if`
Attachment #8777409 - Flags: review?(jsantell) → review+
Still a lot of unrelated errors... I cloned central and doing a new try push based off it. I don't think any of those errors are mine. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2c47ded8d82
jryans helped me figure this out. I needed to blacklist our CSS file because it has properties that Firefox doesn't understand, and we have a test that fails on that. New run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72ded62c939f
None of those errors look related, pushing!
Pushed by jlong@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/8a57ce4bdb7f
land new debugger frontend behind a pref r=jsantell
Yay!
https://hg.mozilla.org/mozilla-central/rev/8a57ce4bdb7f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1293325
Blocks: 980529
Blocks: 883702
Blocks: 882790
Blocks: 1184160
Blocks: 1033555
Blocks: 1070898
Blocks: 1097706
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.