Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1145053003:
DevTools: Support keyboard arrows navigation on flame chart (Closed)
|
Created:
5 years, 7 months ago by alph Modified:
5 years, 7 months ago CC:
blink-reviews, caseq+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDevTools: Support keyboard arrows navigation on flame chart
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195771
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressing comments #
Total comments: 3
Patch Set 3 : for landing #Messages
Total messages: 14 (5 generated)
alph@chromium.org changed reviewers: + caseq@chromium.org, yurys@chromium.org
https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/FlameChart.js (right): https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:46: requestSelectionChange: function(entryIndex) { }, How is that different from FlameChart.Events.EntrySelected? https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:782: _handleSelectionNav: function(e) no abbreviations pls. https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:816: if (e.keyIdentifier === "Left" || e.keyIdentifier === "Right") { Let's use constants from WebInspector.KeyboardShortcut.Keys? https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:830: delta *= -1; delta = -delta perhaps?
https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/FlameChart.js (right): https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:46: requestSelectionChange: function(entryIndex) { }, On 2015/05/22 00:09:19, caseq wrote: > How is that different from FlameChart.Events.EntrySelected? Ahh, thanks. Missed that. https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:782: _handleSelectionNav: function(e) On 2015/05/22 00:09:18, caseq wrote: > no abbreviations pls. Done. https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:816: if (e.keyIdentifier === "Left" || e.keyIdentifier === "Right") { On 2015/05/22 00:09:18, caseq wrote: > Let's use constants from WebInspector.KeyboardShortcut.Keys? Done. https://codereview.chromium.org/1145053003/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/FlameChart.js:830: delta *= -1; On 2015/05/22 00:09:18, caseq wrote: > delta = -delta perhaps? Done.
lgtm https://codereview.chromium.org/1145053003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/ui_lazy/FlameChart.js (right): https://codereview.chromium.org/1145053003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui_lazy/FlameChart.js:820: this.dispatchEventToListeners(WebInspector.FlameChart.Events.EntrySelected, levelIndexes[indexOnLevel]); nit: return here... https://codereview.chromium.org/1145053003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui_lazy/FlameChart.js:821: } else if (e.keyCode === keys.Up.code || e.keyCode === keys.Down.code) { ... and then remove else here. (I think this is more readable than having both returns from within ifs and elses)
https://codereview.chromium.org/1145053003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/ui_lazy/FlameChart.js (right): https://codereview.chromium.org/1145053003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui_lazy/FlameChart.js:820: this.dispatchEventToListeners(WebInspector.FlameChart.Events.EntrySelected, levelIndexes[indexOnLevel]); On 2015/05/22 01:14:03, caseq wrote: > nit: return here... Done.
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/1145053003/#ps40001 (title: "for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1145053003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1145053003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195771 |
