1397563 - Update Debugger frontend (9/6/2017).

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Comment 1

[Jason Laster [:jlast]]

Attachment: 9-6.patch

Comment 2

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02187773c10b18724a64ee72a35ce52d57cf948a

Comment 3

[Julian Descottes [: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.

Comment 4

[Jason Laster [:jlast]]

Attachment: 9-6-2.patch

Comment 5

[Jason Laster [:jlast]]

I updated the bundle with some fixes. See this branch for the fixes applied: https://github.com/devtools-html/debugger.html/pull/3930

Comment 6

[Jason Laster [:jlast]]

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

Comment 7

[Jason Laster [:jlast]]

Attachment: 9-6-3.patch

adding some photon css changes

Comment 8

[Jason Laster [:jlast]]

i'm seeing a windows/linux issue w/ dbg-breakpoints: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd4ad0d28efe5073a4e0294dafccc61cbbf5dd1d&selectedJob=129319301

Comment 9

[Julian Descottes [: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.

Comment 10

[Jason Laster [:jlast]]

Attachment: 9-6-4.patch

Comment 11

[Jason Laster [:jlast]]

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

Comment 12

[Jason Laster [:jlast]]

Attachment: 9-6-5.patch

Comment 13

[Julian Descottes [: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 14

[Julian Descottes [:jdescottes]]

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

Comment 15

[Jason Laster [:jlast]]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaec8440ef8f
Update Debugger Frontend (9/6/2017). r=jdescottes

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eaec8440ef8f