Closed
Bug 1397563
Opened 8 years ago
Closed 8 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•8 years ago
|
||
Attachment #8905328 -
Flags: review?(gl)
Assignee | ||
Comment 2•8 years ago
|
||
Updated•8 years ago
|
Attachment #8905328 -
Flags: review?(gl) → review?(jdescottes)
Comment 3•8 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•8 years ago
|
||
Attachment #8905328 -
Attachment is obsolete: true
Attachment #8905545 -
Flags: review?(jdescottes)
Assignee | ||
Comment 5•8 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•8 years ago
|
||
Assignee | ||
Comment 7•8 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•8 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•8 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•8 years ago
|
||
Attachment #8905636 -
Attachment is obsolete: true
Attachment #8905693 -
Flags: review?(jdescottes)
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8905693 -
Attachment is obsolete: true
Attachment #8905693 -
Flags: review?(jdescottes)
Attachment #8905702 -
Flags: review?(jdescottes)
Comment 13•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
Product: Firefox → DevTools
Updated•7 years ago
|
Assignee: nobody → jlaster
You need to log in
before you can comment on or make changes to this bug.
Description
•