1439753 - Uplift fixes for Debugger (part 2)

Status: VERIFIED FIXED

(DevTools :: Debugger, defect)

Comment 1

[Jason Laster [:jlast]]

Attachment: upl-1.patch

[Feature/Bug causing the regression]: 

- Modifying search field moves cursor to the end #1430672
- [Breakpoints] address syncing edgecases https://github.com/devtools-html/debugger.html/issues/3982

[User impact if declined]: the debugger will show breakpoints in the wrong location when users change their code and reload.

[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

Comment 2

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74f1077245a6e03a7b65bec0769bbe95a58608ed

Comment 3

[Jason Laster [:jlast]]

https://github.com/devtools-html/debugger.html/compare/release-9-2...release-9-3

Comment 4

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53210aaa424553e15596888bd9c1745b8a26d028

a new try that should be better rebased

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: uplift-debugger-v9.3.patch

(not sure what happened with the previous patch? took this one from the try push)

[Feature/Bug causing the regression]: 

- Modifying search field moves cursor to the end #1430672
- [Breakpoints] address syncing edgecases https://github.com/devtools-html/debugger.html/issues/3982

[User impact if declined]: the debugger will show breakpoints in the wrong location when users change their code and reload.

[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

Comment 6

[Julian Descottes [:jdescottes]]

I want to highlight that in this patch only the changes to debugger.js are actual code changes that impact Firefox, all the rest is purely code change to support and validate the changes.

It's a bit hard to navigate back to the actual changes enabled here (reverts of reverts). The main issues being fixed are:
- https://github.com/devtools-html/debugger.html/issues/5140
- https://github.com/devtools-html/debugger.html/issues/3982

There is also https://github.com/devtools-html/debugger.html/commit/1211a0e41384d35d78ca4ba4e222dc9f763c9a2b which removes some code in SearchInput.js, but I can't find a matching issue. Jason, can you give more details about what this one?

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8952656 [details] [diff] [review]
uplift-debugger-v9.3.patch

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

LGTM, but this brings over one regression from the current nightly:
- open debugger
- open any source
- CMD+F to open search input
- type something
- click on the editor to blur the field
- press CMF+F again to focus the search input

ER: Field is cleared and focused, I can start typing to search.
AR: Field is cleared but not focused, I need to manually click on it before typing.

Comment 8

[Jason Laster [:jlast]]

Alright, i'll take the search input change out so that we don't have the regression.

Comment 9

[Jason Laster [:jlast]]

Attachment: upl-2.patch

Approval Request Comment
[Feature/Bug causing the regression]:

Just this change

- [Breakpoints] address syncing edgecases https://github.com/devtools-html/debugger.html/issues/3982

(same as before)

[User impact if declined]: 
[Is this code covered by automated tests?]: 
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8952753 [details] [diff] [review]
upl-2.patch

Fix for debugger syncing problems, includes updates for tests. 
Let's take this for beta 12.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Any plans for testing this again/verifying the fixes before release?

Comment 12

[Jason Laster [:jlast]]

yep, I'll do some more testing tomorrow before it is checkin-needed and we'll definitely do some testing/verifying after this lands before release. We don't anticipate any other patches though.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ec40d01788c

Comment 14

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1438052#c17, I will mark this as verified fixed.