Closed Bug 1294139 Opened 3 years ago Closed 2 years ago

[debugger.html] Turn new debugger on by default for all channels

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox52 fixed, firefox56 verified)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox52 --- fixed
firefox56 --- verified

People

(Reporter: clarkbw, Assigned: jlast)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

This bug is for tracking the change required to turn the debugger.html on as well as the work that is required to turn on the new debugger on by default.

bug 1288511 landed the new debugger off by default behind the pref: devtools.debugger.new-debugger-frontend
I'm assuming this is something we are working on right now, so tentatively marking this as P1. James, if that is not correct, feel free to change to whatever you think is appropriate.
Priority: -- → P1
Yep, that sounds right. Sorry I haven't been triaging!
Depends on: 1294685
Depends on: 1294686
Depends on: 1294749
Depends on: 1295318
No longer depends on: 1295318
Depends on: 1295321
See Also: → 1295696
No longer depends on: 1294685
Depends on: 1295753
Depends on: 1296048
Depends on: 1299602
Depends on: 1300866
Depends on: 1301147
Depends on: 1301207
Depends on: 1301719
Depends on: 1302833
Depends on: 1302872
Depends on: 1302862
Depends on: 1303177
Depends on: 1303045
Depends on: 1303866
Depends on: 1303862
Updating title as the new debugger is already turned on by default in Nightly only.
Summary: [debugger.html] Turn new debugger on by default → [debugger.html] Turn new debugger on by default for all channels
Depends on: 1304416
Depends on: 1305079
Depends on: 1306459
No longer depends on: 1307037
Depends on: 1310330
Depends on: 1313197
Adding bug 1314057 for removing the old debugger which requires this to be done plus the other work that will block that bug as well.
Blocks: 1314057
No longer depends on: 1301705
Attached patch 1294139.patch (obsolete) — Splinter Review
Attachment #8809136 - Flags: review?(bgrinstead)
Comment on attachment 8809136 [details] [diff] [review]
1294139.patch

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

Congrats - nice work all!
Attachment #8809136 - Flags: review?(bgrinstead) → review+
Pushed by jlong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bab8ca566708
Turn new debugger on by default for all channels r=bgrins
https://hg.mozilla.org/mozilla-central/rev/bab8ca566708
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Depends on: 1319743
The fix was somewhat reverted by bug 1332114. Re-opening to be resolved for release channels too.
Status: RESOLVED → REOPENED
Depends on: 1332114
Resolution: FIXED → ---
Depends on: 1335901
Blocks: 980529
Blocks: 883702
Blocks: 1026455
I know we're very close to enabling the debugger on all channels, which is really great. All blocker bugs here are marked as resolved.

I have 2 questions:

- Can we take a product decision on the 2 remaining features (that I know of) are missing still: blackboxing and project search? Blackboxing in particular first appeared in Firefox's old debugger and then other devtools adopted it. So it seems like something we'd want to have in our new debugger right from the start.

- Getting more manual QA on this might uncover some things we've missed. The change is so big though that we would need to create a few specific cases that people could test. Has the qe-verify+ flag been set on other (more specific) bugs already? Is this wanted?
Flags: needinfo?(jlaster)
Flags: needinfo?(clarkbw)
(In reply to Patrick Brosset <:pbro> from comment #11)
> - Can we take a product decision on the 2 remaining features (that I know
> of) are missing still: blackboxing and project search? 

For project search I think we can ship without for now.  Its a good feature but I think we have a much better file and function search available than before so I'm comfortable not requiring this at point.

> Blackboxing in
> particular first appeared in Firefox's old debugger and then other devtools
> adopted it. So it seems like something we'd want to have in our new debugger
> right from the start.

I agree.  I think I'm convinced that at least shipping an MVP of the black boxing feature should be required.

> - Getting more manual QA on this might uncover some things we've missed. The
> change is so big though that we would need to create a few specific cases
> that people could test. Has the qe-verify+ flag been set on other (more
> specific) bugs already? Is this wanted?

Yes.  We have a couple critical areas where it would be good to get some QA.  Off the top of my head its this:

* Breakpoints
* Watch Expressions
* Call Stack
* Scopes
* File Search
* Function Search
Flags: needinfo?(clarkbw)
As discussed in comment 12, let's see if we can get some QA for these areas of the new debugger:
* Breakpoints
* Watch Expressions
* Call Stack
* Scopes
* File Search
* Function Search
This is a rather large functional area, so we should describe in more details what should be tested here, but for now, let's use the qe-verify flag here and let's figure things out later.
Flags: qe-verify+
I'm very glad to see we setup some process to ship the new debugger!

I think we should also associate manual QA with a branch, or limit the kind of patches that gets in master.
Otherwise it will be very easy to slice in new regressions while QA is proceeding.

I know it is unpleasant and sounds like doing the things we don't like from mozilla-central,
but I do not see how we could ensure a good level of quality without such process.
re: releasing, we discussed releasing a version of the "Enable new frontend" settings checkbox. This will let us soft launch and get valuable user feedback.

It'd be great to get a QA process setup. 

* I can help write a spec 
* How do we want to setup a branch for QA? Could we do branch off of github? I believe it's been discussed
Flags: needinfo?(jlaster)
Assignee: jlong → nobody
Setting target for Fx 56
Assignee: nobody → jlaster
Target Milestone: Firefox 51 → Firefox 56
Depends on: 1346902
With Nightly 55 cycle being twice as long, I'd like this to be resolved as soon as possible so we have time for manual QA in this cycle before we hit Beta 55.
If bug 1346902 is the last blocker, then what's blocking bug 1346902?
I think we should run QA right away. The browser toolbox is an edge case. And tbh I'm not sure it is a blocker to push the new debugger on release channel.

During the workweek we were told the new debugger was ready to ship to release,
but the day after I looked at it very briefly and found various obvious bugs:
 - https://github.com/devtools-html/debugger.html/issues/2393
 - https://github.com/devtools-html/debugger.html/issues/2394
 - https://github.com/devtools-html/debugger.html/issues/2395

All has been fixed now (except the last), but I imagine similar issues may still exists.
We should spend time chasing and fixing these kind of bugs and shouldn't wait for it to be enabled on release to do that!
(In reply to Alexandre Poirot [:ochameau] from comment #18)
> I think we should run QA right away. The browser toolbox is an edge case.
> And tbh I'm not sure it is a blocker to push the new debugger on release
> channel.
> 
> During the workweek we were told the new debugger was ready to ship to
> release,
> but the day after I looked at it very briefly and found various obvious bugs:
>  - https://github.com/devtools-html/debugger.html/issues/2393
>  - https://github.com/devtools-html/debugger.html/issues/2394
>  - https://github.com/devtools-html/debugger.html/issues/2395
> 
> All has been fixed now (except the last), but I imagine similar issues may
> still exists.
> We should spend time chasing and fixing these kind of bugs and shouldn't
> wait for it to be enabled on release to do that!
Good point. We've already reached out to QA to work out a plan for them to start testing as soon as possible, just waiting on a final confirmation.
You are right that enabling the debugger by default doesn't need to happen first though, and anyway it's already enabled in Nightly. I just want this to happen in the 55 timeframe, so the sooner the better.
Depends on: 1365565
Depends on: 1365571
Depends on: 1365603
Depends on: 1366745
Depends on: 1366754
Depends on: 1366800
Depends on: 1367777
Depends on: 1376266
Depends on: 1376727
Depends on: 1379958
Depends on: 1379978
Depends on: 1380007
Attached patch enable-1.patch (obsolete) — Splinter Review
Attachment #8885759 - Flags: review?(jdescottes)
Comment on attachment 8885759 [details] [diff] [review]
enable-1.patch

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

Thanks, looks good! 

I only reviewed the code change and I assume that the debugger project matches all the criteria needed to enable it in beta & release.
Attachment #8885759 - Flags: review?(jdescottes) → review+
See Also: → 1368931
Attached patch enable-1.patchSplinter Review
Attachment #8809136 - Attachment is obsolete: true
Attachment #8885759 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5687797261d
[debugger.html] Turn new debugger on by default for all channels. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5687797261d
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Depends on: 1384536
Depends on: 1384061
Depends on: 1384933
Depends on: 1388798
Depends on: 1389013
Depends on: 1389075
Depends on: 1389091
Depends on: 1389522
Depends on: 1391240
Depends on: 1391267
Depends on: 1392277
Depends on: 1395851
Depends on: 1396523
Depends on: 1396841
Depends on: 1397678
Depends on: 1397697
Depends on: 1398064
Product: Firefox → DevTools
This was covered by manual testing efforts and received pre-Release sign off report in Fx 56. 

Removing the qe-verify+ flag.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.