Closed Bug 1397563 Opened 7 years ago Closed 7 years ago

Update Debugger frontend (9/6/2017).

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: jlast, Assigned: jlast)

Details

Attachments

(1 file, 4 obsolete files)

      No description provided.
Attached patch 9-6.patch (obsolete) — Splinter Review
Attachment #8905328 - Flags: review?(gl)
Attachment #8905328 - Flags: review?(gl) → review?(jdescottes)
Comment on attachment 8905328 [details] [diff] [review]
9-6.patch

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

Code looks OK (and doesn't need a rebase :)) but I spotted several issues while testing during only 5 minutes. 

- cmd + shift + O conflicts with the shortcut for the devtools options panel, consequently doesn't work
- regression: when restoring breakpoints:
  - go to http://firefox-dev.tools/debugger-examples/examples/todomvc/
  - add a breakpoint in app-view.js :: addOne
  - hit the breakpoint (by adding a todo)
  - resume
  - close devtools, reopen devtools
  (the breakpoint is still visible)
  - add a todo => breakpoint is not hit
- regression: search query is not cleared when using cmd+P
  - go to http://firefox-dev.tools/debugger-examples/examples/todomvc/
  - cmp+P, type app-view, valid with RETURN
  - cmp+P, "app-view" is still in the input field
- UX issue: focus of file search moves to the list of results when results are returned
  - go to http://firefox-dev.tools/debugger-examples/examples/todomvc/
  - cmd+shift+F, type "test"
  - wait for results
  - hit BACKSPACE to correct your query
  => since the focus was moved to the results list the browser navigates to the previous page
  The user feedback for the focused element is very limited in the area, so I would not advise
  moving the focus programmatically here. I think you should map UP/DOWN so that it selects a
  different item in the results list, but keep the focus in the field. Similar to what we do with
  the autocompletes in devtools. Anyway I think this deserves more work otherwise I'm afraid it will
  be frustrating for users. (I failed on it several times before I understood what was happening...)

I think I also spotted several breakpoint issues related to having the debugger running in several tabs at the same time, but that's not a regression so I'll log that separately.
Attachment #8905328 - Flags: review?(jdescottes)
Attached patch 9-6-2.patch (obsolete) — Splinter Review
Attachment #8905328 - Attachment is obsolete: true
Attachment #8905545 - Flags: review?(jdescottes)
I updated the bundle with some fixes. See this branch for the fixes applied: https://github.com/devtools-html/debugger.html/pull/3930
Attached patch 9-6-3.patch (obsolete) — Splinter Review
adding some photon css changes
Attachment #8905545 - Attachment is obsolete: true
Attachment #8905545 - Flags: review?(jdescottes)
Attachment #8905636 - Flags: review?(jdescottes)
Comment on attachment 8905636 [details] [diff] [review]
9-6-3.patch

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

Since we have a test failure, I'll wait for the next bundle to r+.

Most of the issues raised in the previous review have been fixed. Remains:
- keyboard navigation/focus issue with project search
(won't block the review on that but should still be mentioned)

Another thing, I'm not sure how much async stepping is supposed to be ready or if I should test it. 
I quickly tried stepping over await & yield statements but it didn't work.

Asking because I saw the associated pref was flipped to true in this changeset too.
Attachment #8905636 - Flags: review?(jdescottes)
Attached patch 9-6-4.patch (obsolete) — Splinter Review
Attachment #8905636 - Attachment is obsolete: true
Attachment #8905693 - Flags: review?(jdescottes)
Attached patch 9-6-5.patchSplinter Review
Attachment #8905693 - Attachment is obsolete: true
Attachment #8905693 - Flags: review?(jdescottes)
Attachment #8905702 - Flags: review?(jdescottes)
I tested async stepping a bit more. 

I'm not sure if it works for our code (I initially tried with BrowserToolbox on DevTools code using async await). But it does work with content files. 

I found a bug related to breakpoints and async stepping. Basically if you close and reopen the debugger while paused on an async step, the debugger will create a breakpoint.

STRs:
- go to https://fiddle.jshell.net/20zej0hp/1/show/ (by the way what's the least painful way of creating debugger examples? jsfiddle is kind of bad)
- open debugger
- click on the button in the page
- step over some waitFor calls
- close debugger
- open debugger
=> A breakpoint has been created on the last line where you were before closing the debugger.
Comment on attachment 8905702 [details] [diff] [review]
9-6-5.patch

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

Code is fine so I'll r+ this. I shared the issues I found. 
I'll leave it up to you to decide if you want to correct them or land as is.

Known issues with this bundle:
- async stepping creates breakpoints when closing and reopening the debugger
- keyboard navigation issues with the project search
- keyboard shortcut conflict between the debugger and the options panel
Attachment #8905702 - Flags: review?(jdescottes) → review+
Thanks for the review Julian. 

> I'm not sure if it works for our code (I initially tried with BrowserToolbox on DevTools code using async await). 

I wonder if this is a pref thing...


> if you close and reopen the debugger while paused on an async step

I think this is an edge case we can fix as a follow up



Lets move forward and land the bundle. We can fix these issues going forward.
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaec8440ef8f
Update Debugger Frontend (9/6/2017). r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/eaec8440ef8f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
Assignee: nobody → jlaster
You need to log in before you can comment on or make changes to this bug.