The console should feel more like an interactive terminal. It should have tab completion and an input prompt that sits right below the last output line.
Created attachment 19787 [details] Patch
Comment on attachment 19787 [details] Patch + Bug 17870: Web Inspector console should fell more like a terminal Typo: fell -> feel ChangeLog has a couple of tabs + function focusPromptSoon() + { + if (!this._caretInsidePrompt()) + this._moveCaretToEndOfPrompt(); + } This function should just be called focusPrompt, since the function itself is synchronous. A number of the new functions in this patch have their opening brace on the same line as the declaration. I don't really have a preference either way, but it's definitely more consistent with our existing Inspector code to put the brace on the next line. + clearAutoComplete: function(includeTimeout) { + if (includeTimeout && "completeTimeout" in this) { + clearTimeout(this.completeTimeout); + delete this.completeTimeout; + } + + if (this.autoCompleteElement) { + if (this.autoCompleteElement.parentNode) + this.autoCompleteElement.parentNode.removeChild(this.autoCompleteElement); + delete this.autoCompleteElement; + } + }, You can return early if (!this.autoCompleteElement). + if (auto) { + if (!selection.isCollapsed) + return; + + var node = selectionRange.startContainer; + if (node.nodeType === Node.TEXT_NODE && selectionRange.startOffset < node.nodeValue.length) + return; + + var foundNextText = false; + while (node) { + if (node.nodeType === Node.TEXT_NODE && node.nodeValue.length) { + if (foundNextText) + return; + foundNextText = true; + } + + node = node.traverseNextNode(this.promptElement); + } + } It might be nice to put this in a named helper function to make it clearer what its purpose is. I also think you're missing an argument to traverseNextNode (I assume you meant for this.promptElement to be the stayWithin parameter, not the skipWhitespace parameter). + var currentText = fullWordRange.toString(); + for (var i = (currentText.length - 1); i > 0; --i) + if (currentText[i] !== " ") + break; + + currentText = currentText.substring(0, (i + 1)); Is this not the same as: var currentText = fullWordRange.toString().trimTrailingWhitespace(); + if (window.event && window.event.altKey) + completionText += " "; Do you have a specific use case for this in mind? (This won't work as-is on Windows because Alt-Tab is a system keyboard shortcut for the app/window switcher). + var expression = this._backwardsRange(" =:({;", wordRange.startContainer, wordRange.startOffset, this.promptElement); There are two calls to _backwardsRange in this patch, but they each pass a slightly different first parameter. Is that intentional? Should we just build in the first parameter to _backwardsRange, or make a wrapper function that builds it in? + var expressionString = expression.toString(); + for (var i = (expressionString.length - 1); i > 0; --i) + if (expressionString[i] !== ".") + break; + + expressionString = expressionString.substring(0, (i + 1)); I think this would be clearer as: var expressionString = expression.toString().replace(/\.+$/, ""); + if (property.substring(0, prefix.length) !== prefix) I think this is equivalent to: if (property.indexOf(prefix) !== 0) but maybe your version is faster? + if (selectionRange.startContainer !== this.promptElement && !selectionRange.startContainer.isDescendant(this.promptElement)) + return false; + return true; You can change this to: return selectionRange.startContainer === this.promptElement && selectionRange.startContainer.isDescendant(this.promptElement); +WebInspector.ConsoleCommand = function(command, result, formattedResultElement, level) { - this.input = input; - this.output = output; + this.command = command; + this.result = result; + this.formattedResultElement = formattedResultElement; + this.level = level; Looks like this.result is never used. Maybe we should remove it. Is this patch missing changes to inspector.css? r=me
Landed in r31079.
(In reply to comment #2) > (From update of attachment 19787 [details] [edit]) > + var expression = this._backwardsRange(" =:({;", > wordRange.startContainer, wordRange.startOffset, this.promptElement); > > There are two calls to _backwardsRange in this patch, but they each pass a > slightly different first parameter. Is that intentional? Should we just build > in the first parameter to _backwardsRange, or make a wrapper function that > builds it in? So are the different first parameters correct? Should we add a comment about them?
(In reply to comment #2) > (From update of attachment 19787 [details] [edit]) > > + if (window.event && window.event.altKey) > + completionText += " "; > > Do you have a specific use case for this in mind? (This won't work as-is on > Windows because Alt-Tab is a system keyboard shortcut for the app/window > switcher). I removed this there was no good reason to have it. > + var expression = this._backwardsRange(" =:({;", > wordRange.startContainer, wordRange.startOffset, this.promptElement); > > There are two calls to _backwardsRange in this patch, but they each pass a > slightly different first parameter. Is that intentional? Should we just build > in the first parameter to _backwardsRange, or make a wrapper function that > builds it in? They are different for a reason. The first scan needs to stop sooner, so it will still when it encounters "." or "[". The next scan needs to read the whole expression, stopping only for " ", ":", "=", "{", "(". This is not a great set, and this logic should be improved. Since space could be encountered in an expression like "test["tim hatcher"].found". I will file a bug about this. > Is this patch missing changes to inspector.css? I forgot to attach it.
(In reply to comment #5) > > + var expression = this._backwardsRange(" =:({;", > > wordRange.startContainer, wordRange.startOffset, this.promptElement); > > > > There are two calls to _backwardsRange in this patch, but they each pass a > > slightly different first parameter. Is that intentional? Should we just build > > in the first parameter to _backwardsRange, or make a wrapper function that > > builds it in? > > They are different for a reason. The first scan needs to stop sooner, so it > will still when it encounters "." or "[". The next scan needs to read the whole > expression, stopping only for " ", ":", "=", "{", "(". This is not a great set, > and this logic should be improved. Since space could be encountered in an > expression like "test["tim hatcher"].found". I will file a bug about this. Maybe we could put these two strings in named variables that explained their function better. I don't know what the names should be since I still don't completely understand the differences. A guess at the names would be something like "wordDelimiters" and "objectDelimiters", but really these are just guesses.