Closed Bug 1439753 Opened 2 years ago Closed 2 years ago

Uplift fixes for Debugger (part 2)

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox59 verified)

VERIFIED FIXED
Firefox 59
Tracking Status
firefox59 --- verified

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch upl-1.patch (obsolete) — Splinter Review
[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
Attachment #8952550 - Flags: review?(jdescottes)
Attachment #8952550 - Flags: approval-mozilla-beta?
See Also: → 1438052
Attached patch uplift-debugger-v9.3.patch (obsolete) — Splinter Review
(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
Attachment #8952550 - Attachment is obsolete: true
Attachment #8952550 - Flags: review?(jdescottes)
Attachment #8952550 - Flags: approval-mozilla-beta?
Attachment #8952656 - Flags: review?(jdescottes)
Attachment #8952656 - Flags: approval-mozilla-beta?
Attachment #8952656 - Attachment is patch: true
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
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?
Flags: needinfo?(jlaster)
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.
Attachment #8952656 - Flags: review?(jdescottes) → review+
Alright, i'll take the search input change out so that we don't have the regression.
Flags: needinfo?(jlaster)
Attached patch upl-2.patchSplinter Review
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]:
Attachment #8952656 - Attachment is obsolete: true
Attachment #8952656 - Flags: approval-mozilla-beta?
Attachment #8952753 - Flags: review+
Attachment #8952753 - Flags: approval-mozilla-beta?
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.
Attachment #8952753 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Any plans for testing this again/verifying the fixes before release?
Flags: needinfo?(jlaster)
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.
Flags: needinfo?(jlaster)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.