Update Debugger frontend (9/6/2017).

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: jlast, Unassigned)

Tracking

unspecified
Firefox 57

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

a year ago
Created attachment 8905328 [details] [diff] [review]
9-6.patch
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)
(Reporter)

Comment 4

a year ago
Created attachment 8905545 [details] [diff] [review]
9-6-2.patch
Attachment #8905328 - Attachment is obsolete: true
Attachment #8905545 - Flags: review?(jdescottes)
(Reporter)

Comment 5

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

Comment 7

a year ago
Created attachment 8905636 [details] [diff] [review]
9-6-3.patch

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)
(Reporter)

Comment 10

a year ago
Created attachment 8905693 [details] [diff] [review]
9-6-4.patch
Attachment #8905636 - Attachment is obsolete: true
Attachment #8905693 - Flags: review?(jdescottes)
(Reporter)

Comment 12

a year ago
Created attachment 8905702 [details] [diff] [review]
9-6-5.patch
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+
(Reporter)

Comment 15

a year ago
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

a year ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaec8440ef8f
Update Debugger Frontend (9/6/2017). r=jdescottes

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eaec8440ef8f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.