[webkit-reviews] review granted: [Bug 17870] Web Inspector console should feel more like a terminal : [Attachment 19787] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Timothy Hatcher
<timothy at hatcher.name>'s request for review:
Bug 17870: Web Inspector console should feel more like a terminal
http://bugs.webkit.org/show_bug.cgi?id=17870

Attachment 19787: Patch
http://bugs.webkit.org/attachment.cgi?id=19787&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
+	 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