Closed Bug 1438052 Opened 6 years ago Closed 6 years ago

Uplift fixes for Debugger

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(firefox59 verified)

VERIFIED FIXED
Firefox 59
Tracking Status
firefox59 --- verified

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached patch uplift.patch (obsolete) — Splinter Review
Attachment #8950827 - Flags: review?(jdescottes)
this patch is still a bit wip, so it doesnt have the beta flag yet

here's a comparison though
https://github.com/devtools-html/debugger.html/compare/release-9-1...release-9-2
We already uplifted https://bugzilla.mozilla.org/show_bug.cgi?id=1431127, so we need https://github.com/devtools-html/debugger.html/commit/525e87121c6ccbb7f1caa3ab7a76fab412bc4c49 included in the release.

The patch attached here is empty, but I quickly looked at the patch on try.

This uplift does not revert the "pause buttons" popup that introduced and that we want to remove for sure from beta. Is this coming in another uplift?
Attached patch uplift-2.patchSplinter Review
[Feature/Bug causing the regression]: 

jsx syntax highlighting - https://github.com/devtools-html/debugger.html/pull/5174
disabled breakpoints are not synced - https://github.com/devtools-html/debugger.html/issues/5140
stepping over await calls should not require an ast - https://github.com/devtools-html/debugger.html/issues/5320
stepping should scroll to the correct location - https://github.com/devtools-html/debugger.html/issues/5184

[User impact if declined]: the debugger will be missing some helpful performance and usability fixes 

[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: it only affects the debugger
[String changes made/needed]: none
Attachment #8950827 - Attachment is obsolete: true
Attachment #8950827 - Flags: review?(jdescottes)
Attachment #8950981 - Flags: review?(jdescottes)
Attachment #8950981 - Flags: approval-mozilla-beta?
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Comment on attachment 8950981 [details] [diff] [review]
uplift-2.patch

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

Patch looks good to me, thanks Jason!

The number of commits being requested for uplift here is relatively small [1] and it 
disables the breakpoint popup as expected.

[1] https://github.com/devtools-html/debugger.html/compare/release-9-1...release-9-2
Attachment #8950981 - Flags: review?(jdescottes) → review+
Comment on attachment 8950981 [details] [diff] [review]
uplift-2.patch

Improvements to Debugger, we still have ~3 weeks of stabilizing on Beta before RC week, Beta59+
Attachment #8950981 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Andrei, Tom, could we get a smoke test on Debugger to ensure these changes (which aren't small) do not regression quality? Thanks!

Hi Julian, are we expecting more uplifts for debugger in 59? Just wondering about the plans and on what to expect as we get closer to RC week.
Flags: needinfo?(tgrabowski)
Flags: needinfo?(jdescottes)
Flags: needinfo?(andrei.vaida)
> Hi Julian, are we expecting more uplifts for debugger in 59? Just wondering about the plans and on what to expect as we get closer to RC week.

We might have another, as we just removed some commits from the initial patch here to make it smaller. Forwarding the question to Jason.
Flags: needinfo?(jdescottes) → needinfo?(jlaster)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
It would be nice to do an additional uplift for some breakpoint saving bugs, I'll try and put something together soon. If we don't get to it, it's alright.
Flags: needinfo?(jlaster)
I am redirecting Andrei's ni to myself, as I will start the verification process as soon as we have green light from Jason after the additional uplift is done.
Flags: needinfo?(andrei.vaida) → needinfo?(iulia.cristescu)
Flags: needinfo?(tgrabowski)
(fyi, new uplift bug created at bug 1439753)
See Also: → 1439753
Apart from a smoke test across platforms (Windows 10 x64, Ubuntu 16.04 x64, macOs 10.13.2) that focused the main debugger functionality and pottential regressions, I investigated certain scenarios for the breakpoint sync and search. I found some potential problematic cases, but only one of them seems related to these particular fixes. See  https://goo.gl/ryh5tV. 
Please let me know what is your opinion regarding these.
Flags: needinfo?(iulia.cristescu) → needinfo?(jlaster)
Thanks Julia!

The overview is fantastic. I think this uplift improved syncing, but there is atleast one case that we had not considered. I've created an issue in github for follow up improvements.

https://github.com/devtools-html/debugger.html/issues/5570
Flags: needinfo?(jlaster)
Confirmed with Jason that the above issue (https://github.com/devtools-html/debugger.html/issues/5570) will be assessed separately, so, based on the previous comments, I will mark this (and also bug 1439753) as verified fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.