[webkit-reviews] review denied: [Bug 19158] Inspector should support console.group/console.groupEnd : [Attachment 22026] minor mistake fix

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Keishi Hattori
<casey.hattori at gmail.com>'s request for review:
Bug 19158: Inspector should support console.group/console.groupEnd
https://bugs.webkit.org/show_bug.cgi?id=19158

Attachment 22026: minor mistake fix
https://bugs.webkit.org/attachment.cgi?id=22026&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
This is looking really good! Pretty minor comments below. I guess I'll r- until
we are actually showing the indentation in the messages.

+    page->inspectorController()->increaseGroupLevel();
+    
+    return log(exec, arguments);

Does Firebug increase the indent level before or after printing the message
passed to console.group? We should match either way.

You don't need the "return" keyword here since this function doesn't have a
return value.

@@ -179,6 +181,7 @@ struct ConsoleMessage {
     Vector<ProtectedPtr<JSValue> > wrappedArguments;
     unsigned line;
     String url;
+    unsigned group;
 };

Why don't we call this new member "groupLevel" to match the terminology in the
rest of the patch?

+    void increaseGroupLevel();
+    void decreaseGroupLevel();
+    void setScriptGroupLevel();

I think setScriptGroupLevel can be a private function.

+    setGroupLevel: function(group)
+    {
+	 this.groupLevel = group;
+	 this.promptElement.style.marginLeft = (this.groupLevel * 10) + "px";
+    },

I'm a little confused by this function. Is the point of it to indent the prompt
itself? What's the purpose of that?

+    this.group = group;

Let's call this "this.groupLevel" as well.

What is Firebug's behavior when you evaluate something in the command line
after console.group has been called? Is the expression you evaluated indented
just like a call to console.log would be? If so, we need to modify
_enterKeyPressed to pass the current group level to the ConsoleCommand
constructor.