results.html needs some key handling to make test navigation more usable. I plan to add: i/j/k/l for navigating the list e/c/t for expand/collapse/toggle current test some key to "flag" the current test some way to copy out flagged tests
Sounds great!
Created attachment 121777 [details] Patch
Comment on attachment 121777 [details] Patch Attachment 121777 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11183730 New failing tests: fast/harness/results.html
Comment on attachment 121777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121777&action=review This looks great. I have a bunch of cleanup nits. Feel free to not do any that you disagree with. > LayoutTests/fast/harness/results.html:190 > +#flagged-tests { > + padding: 5px; > +} Should this be position:fixed to the bottom of the window? I can't decide if that would be nice or annoying. :) > LayoutTests/fast/harness/results.html:942 > +TestNavigator.scrollToFirstTest = function() Can you make all the properties/methods on TestNavigator except handleKeyEvent "private" (i.e. prefix with an underscore)? > LayoutTests/fast/harness/results.html:945 > + if (this.setCurrentTest(0)) > + this.scrollToCurrentTest(); Here and below, I'd s/this/TestNavigator. While this code does work correctly, I find it confusing when "this" doesn't point to an instance of a class (i.e. something that was new'ed). This way, TestNavigator essentially just acts as a namespace. > LayoutTests/fast/harness/results.html:970 > + var links = visibleExpandLinks(); > + return links[this.currentTestIndex]; You could implement this as "return document.querySelector('.current .expand-button-text')". > LayoutTests/fast/harness/results.html:1014 > + var label = document.createElement('h2'); > + label.innerText = 'Flagged Tests'; > + flaggedTestContainer.appendChild(label); > + > + flaggedTestTextbox = document.createElement('div'); > + flaggedTestTextbox.id = 'flagged-tests'; > + flaggedTestTextbox.setAttribute('contentEditable', ''); > + > + flaggedTestContainer.appendChild(flaggedTestTextbox); It's your call, but I would prefer this to just use innerHTML since it's more concise and readable: flaggedTestContainer.innerHTML = '<h2>Flagged Tests</h2><div id="flagged-tests" contentEditable></div>'; flaggedTestTextbox = document.getElementById('flagged-tests'); > LayoutTests/fast/harness/results.html:1020 > + var flaggedTests = []; > + for (var test in this.flaggedTests) > + flaggedTests.push(test); var flaggedTests = Object.keys(this.flaggedTests); > LayoutTests/fast/harness/results.html:1026 > +TestNavigator.setCurrentTest = function(testIndex) May as well call scrollToCurrentTest at the end of setCurrentTest since you always call it if you return true. Also, then setCurrentTest doesn't need a return value. > LayoutTests/fast/harness/results.html:1039 > + var currExpandButton = links[this.currentTestIndex]; > + if (currExpandButton) > + currExpandButton.parentNode.classList.remove('current'); > + > + this.currentTestIndex = testIndex; > + > + var currExpandButton = links[this.currentTestIndex]; > + currExpandButton.parentNode.classList.add('current'); Nit: s/currExpandButton/currExpandLink/ Feel free to ignore this, but instead of maintaining currentTestIndex manually, you could rely on the fact that you're already adding/removing the appropriate class. So, this could be implemented as: var currExpandButton = document.querySelector('.current'); if (currExpandButton) currExpandButton.classList.remove('current'); links[testIndex].parentNode.classList.add('current'); The only place you really need currentTestIndex is when you're moving to the next/previous test. In that case, you can retrieve the index as: function currentTestIndex() { var current = document.querySelector('.current'); return Array.prototype.indexOf.call(visibleExpandLinks(), current); } > LayoutTests/fast/harness/results.html:1046 > + var failedResultsTable = document.getElementById('results-table'); Dead code. > LayoutTests/fast/harness/results.html:1047 > + // Use visibleExpandLinks because it's already smart about expected failures. In updateExpectedFailures(), we should probably just clear "current" if appropriate. Shouldn't be too hard: if (onlyShowUnexpectedFailures() && parentOfType(TestNavigator.currentTestLink(), 'tbody').classList.contains('expected')) TestNavigator.currentTestLink().classList.remove('current'); > LayoutTests/fast/harness/results.html:1053 > + var links = visibleExpandLinks(); > + var testIndex = this.currentTestIndex; > + if (testIndex < 0 || testIndex >= links.length) > + return; > + > + var targetLink = links[testIndex]; You could implement this as: var targetLink = this.currentTestLink(); if (!targetLink) return;
(In reply to comment #4) > (From update of attachment 121777 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121777&action=review > > This looks great. I have a bunch of cleanup nits. Feel free to not do any that you disagree with. > > > LayoutTests/fast/harness/results.html:190 > > +#flagged-tests { > > + padding: 5px; > > +} > > Should this be position:fixed to the bottom of the window? I can't decide if that would be nice or annoying. :) Super annoying, it can get big. > > LayoutTests/fast/harness/results.html:942 > > +TestNavigator.scrollToFirstTest = function() > > Can you make all the properties/methods on TestNavigator except handleKeyEvent "private" (i.e. prefix with an underscore)? Done. > > LayoutTests/fast/harness/results.html:945 > > + if (this.setCurrentTest(0)) > > + this.scrollToCurrentTest(); > > Here and below, I'd s/this/TestNavigator. While this code does work correctly, I find it confusing when "this" doesn't point to an instance of a class (i.e. something that was new'ed). > > This way, TestNavigator essentially just acts as a namespace. OK. Not really keen on this because if we ever want > 1 TestNavigator I'd have to change it back. That seems unlikely though. > > LayoutTests/fast/harness/results.html:970 > > + var links = visibleExpandLinks(); > > + return links[this.currentTestIndex]; > > You could implement this as "return document.querySelector('.current .expand-button-text')". I could, but I'd rather use the same source of links everywhere. > > LayoutTests/fast/harness/results.html:1014 > > + var label = document.createElement('h2'); > > + label.innerText = 'Flagged Tests'; > > + flaggedTestContainer.appendChild(label); > > + > > + flaggedTestTextbox = document.createElement('div'); > > + flaggedTestTextbox.id = 'flagged-tests'; > > + flaggedTestTextbox.setAttribute('contentEditable', ''); > > + > > + flaggedTestContainer.appendChild(flaggedTestTextbox); > > It's your call, but I would prefer this to just use innerHTML since it's more concise and readable: > flaggedTestContainer.innerHTML = '<h2>Flagged Tests</h2><div id="flagged-tests" contentEditable></div>'; > flaggedTestTextbox = document.getElementById('flagged-tests'); Done. > > LayoutTests/fast/harness/results.html:1020 > > + var flaggedTests = []; > > + for (var test in this.flaggedTests) > > + flaggedTests.push(test); > > var flaggedTests = Object.keys(this.flaggedTests); I was looking for a way to do that! > > LayoutTests/fast/harness/results.html:1026 > > +TestNavigator.setCurrentTest = function(testIndex) > > May as well call scrollToCurrentTest at the end of setCurrentTest since you always call it if you return true. Also, then setCurrentTest doesn't need a return value. I want to keep them separate. > > LayoutTests/fast/harness/results.html:1039 > > + var currExpandButton = links[this.currentTestIndex]; > > + if (currExpandButton) > > + currExpandButton.parentNode.classList.remove('current'); > > + > > + this.currentTestIndex = testIndex; > > + > > + var currExpandButton = links[this.currentTestIndex]; > > + currExpandButton.parentNode.classList.add('current'); > > Nit: s/currExpandButton/currExpandLink/ > > Feel free to ignore this, but instead of maintaining currentTestIndex manually, you could rely on the fact that you're already adding/removing the appropriate class. > > So, this could be implemented as: > var currExpandButton = document.querySelector('.current'); > if (currExpandButton) > currExpandButton.classList.remove('current'); > links[testIndex].parentNode.classList.add('current'); > > The only place you really need currentTestIndex is when you're moving to the next/previous test. In that case, you can retrieve the index as: > function currentTestIndex() { > var current = document.querySelector('.current'); > return Array.prototype.indexOf.call(visibleExpandLinks(), current); > } I didn't want to run a querySelector() every time I access currentTestIndex. > > LayoutTests/fast/harness/results.html:1046 > > + var failedResultsTable = document.getElementById('results-table'); > > Dead code. Removed. > > LayoutTests/fast/harness/results.html:1047 > > + // Use visibleExpandLinks because it's already smart about expected failures. > > In updateExpectedFailures(), we should probably just clear "current" if appropriate. Shouldn't be too hard: > > if (onlyShowUnexpectedFailures() && parentOfType(TestNavigator.currentTestLink(), 'tbody').classList.contains('expected')) > TestNavigator.currentTestLink().classList.remove('current'); Holy encapsulation violation batman! I fixed this a cleaner way. > > LayoutTests/fast/harness/results.html:1053 > > + var links = visibleExpandLinks(); > > + var testIndex = this.currentTestIndex; > > + if (testIndex < 0 || testIndex >= links.length) > > + return; > > + > > + var targetLink = links[testIndex]; > > You could implement this as: > var targetLink = this.currentTestLink(); > if (!targetLink) > return; Fixed.
http://trac.webkit.org/changeset/104606
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 121777 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=121777&action=review > > > > > LayoutTests/fast/harness/results.html:1047 > > > + // Use visibleExpandLinks because it's already smart about expected failures. > > > > In updateExpectedFailures(), we should probably just clear "current" if appropriate. Shouldn't be too hard: > > > > if (onlyShowUnexpectedFailures() && parentOfType(TestNavigator.currentTestLink(), 'tbody').classList.contains('expected')) > > TestNavigator.currentTestLink().classList.remove('current'); > > Holy encapsulation violation batman! I fixed this a cleaner way. Lol. Yeah, I was a bit rushed as I was finishing this review. FYI, you accidentally left in this code commented out in the final checkin. Thanks for adding all this! I'm eager to try it out.
Also, this just occurred to me, should we add a legend or something so this is a bit more discoverable? I already find myself forgetting the key commands each time.
There is a legend.