Closed
Bug 1111020
Opened 8 years ago
Closed 8 years ago
UP/DOWN keys don't work to navigate the call tree in the new performance tool
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox38 fixed)
RESOLVED
FIXED
Firefox 38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: vporof, Assigned: jsantell)
References
Details
Attachments
(1 file, 1 obsolete file)
8.50 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•8 years ago
|
Blocks: perf-tool-v2
Reporter | ||
Comment 1•8 years ago
|
||
This is more annoying that beetlejuice, damn it.
Reporter | ||
Updated•8 years ago
|
Blocks: enable-perf-tool
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c461e087c02
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8560692 [details] [diff] [review] 1111020-fix-key-events.patch Review of attachment 8560692 [details] [diff] [review]: ----------------------------------------------------------------- Wow ::: browser/devtools/performance/test/browser_perf-events-calltree.js @@ +34,5 @@ > + fireKey("VK_DOWN"); > + > + JsCallTreeView.off("focus", focusEvent); > + > + ok(focus > 1, "several focus events are fired for the js calltree."); Shouldn't we check for an exact number here? @@ +52,5 @@ > + fireKey("VK_UP"); > + fireKey("VK_DOWN"); > + > + MemoryCallTreeView.off("focus", focusEvent); > + ok(focus > 1, "several focus events are fired for the memory calltree."); Ditto. ::: browser/devtools/shared/widgets/AbstractTreeItem.jsm @@ +447,5 @@ > > switch (e.keyCode) { > case e.DOM_VK_UP: > this._focusPrevNode(); > + e.preventDefault(); Huh? Why?
Attachment #8560692 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8560692 [details] [diff] [review] 1111020-fix-key-events.patch Review of attachment 8560692 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/test/browser_perf-events-calltree.js @@ +34,5 @@ > + fireKey("VK_DOWN"); > + > + JsCallTreeView.off("focus", focusEvent); > + > + ok(focus > 1, "several focus events are fired for the js calltree."); Will we always have sufficient enough call tree items? That's my concern as the stack generated isn't deterministic, I believe ::: browser/devtools/shared/widgets/AbstractTreeItem.jsm @@ +447,5 @@ > > switch (e.keyCode) { > case e.DOM_VK_UP: > this._focusPrevNode(); > + e.preventDefault(); Not necessary for this patch, but to prevent sideeffects in other views from occuring possibly -- want me to remove?
Reporter | ||
Comment 5•8 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4) > Comment on attachment 8560692 [details] [diff] [review] > 1111020-fix-key-events.patch > > Review of attachment 8560692 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/performance/test/browser_perf-events-calltree.js > @@ +34,5 @@ > > + fireKey("VK_DOWN"); > > + > > + JsCallTreeView.off("focus", focusEvent); > > + > > + ok(focus > 1, "several focus events are fired for the js calltree."); > > Will we always have sufficient enough call tree items? That's my concern as > the stack generated isn't deterministic, I believe > Sounds like we should populate the tree manually. > ::: browser/devtools/shared/widgets/AbstractTreeItem.jsm > @@ +447,5 @@ > > > > switch (e.keyCode) { > > case e.DOM_VK_UP: > > this._focusPrevNode(); > > + e.preventDefault(); > > Not necessary for this patch, but to prevent sideeffects in other views from > occuring possibly -- want me to remove? "possibly" is a weird reason. I'd steer clear from changes like these, but it's up to you.
Assignee | ||
Comment 6•8 years ago
|
||
Manually populating the call tree view, and removed the preventDefaults
Attachment #8560692 -
Attachment is obsolete: true
Attachment #8561584 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0dab1771c9b7
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0dab1771c9b7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox38:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•