[webkit-reviews] review denied: [Bug 19158] Inspector should support console.group/console.groupEnd : [Attachment 22125] Bug fixed

[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 22125: Bug fixed
https://bugs.webkit.org/attachment.cgi?id=22125&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
+    const KURL& url = m_frame->loader()->url();
+    
+    if (arguments.isEmpty())
+	 page->inspectorController()->addMessageToConsole(JSMessageSource,
GroupTitleMessageLevel, String(), 0, url.string());
+    else
+	 page->inspectorController()->addMessageToConsole(JSMessageSource,
GroupTitleMessageLevel, exec, arguments, 0, url.string());

I don't think we want to pass in the Frame's URL here. Just passing String()
would be better.

+	 callFunction(m_scriptContext, m_scriptObject, "startGroupInConsole",
0, NULL, exception);

We use 0 instead of NULL in C++ code. (Ditto for endGroup.)

+	 if (typeof msg.groupLevel === "number") {

When will this be false?

+	     while (msg.groupLevel > this.groupLevel)
+		 this.startGroup();
+	     while (msg.groupLevel < this.groupLevel)
+		 this.endGroup();

It seems like if the difference between msg.groupLevel and this.groupLevel were
more than 1, we'd end up with groups without titles. Do we really want to allow
that?

+    startGroup: function() {

The opening brace should be on the next line, aligned with the "s" in
"startGroup". Ditto for the other functions you added.

+	 this.currentGroup.appendChild(groupElement);groupElement.className +=
" " + this.groupLevel;

You're missing a newline after the semicolon here. You should use addStyleClass
instead of manually appending to .className.

Why do we need the groupLevel as part of the class name?

     _messagesClicked: function(event)
     {
+	 var groupTitleElement =
event.target.enclosingNodeOrSelfWithClass("console-group-title-level");
+	 if (groupTitleElement) {
+	     var groupElement =
groupTitleElement.enclosingNodeOrSelfWithClass("console-group");
+	     if (groupElement)

There should be braces around the body of this if since it's more than one
line.

+		 if (groupElement.hasStyleClass("collapsed"))
+		     groupElement.removeStyleClass("collapsed");
+		 else
+		     groupElement.addStyleClass("collapsed");
+		 groupTitleElement.scrollIntoViewIfNeeded(true);
+	 }

Should we return at the end of the if block?

@@ -605,6 +656,9 @@ WebInspector.ConsoleMessage.prototype = 
	     case WebInspector.ConsoleMessage.MessageLevel.Error:
		 levelString = "Error";
		 break;
+	     case WebInspector.ConsoleMessage.MessageLevel.GroupTitle:
+			     levelString = "GroupTitle";
+			     break;
	 }

Indentation got a little funny here.

There are tabs in inspector.css. Our pre-commit hook won't let us land the
patch that way. Please replace the tabs with 4 spaces.

I think this is really close!