Closed
Bug 1397563
Opened 6 years ago
Closed 6 years ago
Update Debugger frontend (9/6/2017).
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jlast, Assigned: jlast)
Details
Attachments
(1 file, 4 obsolete files)
286.61 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8905328 -
Flags: review?(gl)
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02187773c10b18724a64ee72a35ce52d57cf948a
Updated•6 years ago
|
Attachment #8905328 -
Flags: review?(gl) → review?(jdescottes)
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #8905328 -
Attachment is obsolete: true
Attachment #8905545 -
Flags: review?(jdescottes)
Assignee | ||
Comment 5•6 years ago
|
||
I updated the bundle with some fixes. See this branch for the fixes applied: https://github.com/devtools-html/debugger.html/pull/3930
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd4ad0d28efe5073a4e0294dafccc61cbbf5dd1d
Assignee | ||
Comment 7•6 years ago
|
||
adding some photon css changes
Attachment #8905545 -
Attachment is obsolete: true
Attachment #8905545 -
Flags: review?(jdescottes)
Attachment #8905636 -
Flags: review?(jdescottes)
Assignee | ||
Comment 8•6 years ago
|
||
i'm seeing a windows/linux issue w/ dbg-breakpoints: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd4ad0d28efe5073a4e0294dafccc61cbbf5dd1d&selectedJob=129319301
Comment 9•6 years ago
|
||
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)
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8905636 -
Attachment is obsolete: true
Attachment #8905693 -
Flags: review?(jdescottes)
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3a70e2055d76d0c199f2337d8f5ba1b9130db07
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8905693 -
Attachment is obsolete: true
Attachment #8905693 -
Flags: review?(jdescottes)
Attachment #8905702 -
Flags: review?(jdescottes)
Comment 13•6 years ago
|
||
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•6 years ago
|
||
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+
Assignee | ||
Comment 15•6 years 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•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eaec8440ef8f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•5 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Assignee: nobody → jlaster
You need to log in
before you can comment on or make changes to this bug.
Description
•