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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 10 08:21:45 PDT 2008

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

Attachment 22125: Bug fixed

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

+    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 =
+	 if (groupTitleElement) {
+	     var groupElement =
+	     if (groupElement)

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

+		 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";
+	     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!

More information about the webkit-reviews mailing list