Bug 17870 – Web Inspector console should feel more like a terminal

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 19787 [details]
Patch

[Adam Roben (:aroben)]

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

[Timothy Hatcher]

Landed in r31079.

[Adam Roben (:aroben)]

(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?

[Timothy Hatcher]

(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.

[Adam Roben (:aroben)]

(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.