The Inspector should support console.group/console.groupEnd for Firebug parity.
<rdar://problem/5950862>
Created attachment 21986 [details] prototype Prototype implementation of console.group/groupEnd. This patch works by using ConsoleMessage "startGroup" and "endGroup" to find out where the groups begin and end. This is very ugly, so I think maybe a new MessageLevel(something like GroupStartMessageLevel) might be good?
Comment on attachment 21986 [details] prototype I think we need a slightly different approach here. Here's the Firebug documentation for console.group: > console.group(object[, object, ...]) > Writes a message to the console and opens a nested > block to indent all future messages sent to the console. > Call console.groupEnd() to close the block. So it looks like console.group essentially does a console.log, then increases the indent level for all future console messages. console.groupEnd just decreases the indent level for all future console messages. So I think somewhere we're going to have to keep track of the current indent level. console.group will increment it, and console.groupEnd will decrement it. Every time a message is added to the console, we'll indent it according to the current indent level. We should probably store the indent level in the ConsoleMessage struct in InspectorController.cpp. Then we can pass the indent level down to the Inspector's JS with the rest of the message. This will let us get rid of the messy "startGroup"/"endGroup" special messages. Make sense?
Created attachment 22016 [details] interim report I fixed it to match your description. The prompt and command messages doesn't get grouped yet, so I'll have to do that next.
Created attachment 22024 [details] proposed patch Prompt and CommandMesssages get properly indented.
Oops, doesn't work when you close and reopen the inspector.
Created attachment 22026 [details] minor mistake fix fixed.
Comment on attachment 22026 [details] minor mistake fix 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.
Tim might have some good ideas for how to indent the messages properly.
Does Firebug make the groups collapsable? Either way we might want to make our groups collapsable I think. I would not indent the prompt, that seems really odd to me. I would guess the results of the prompt would be indented though.
Some behavior of the firebug implementation: - Arguments for console.group gets bold is text and gets treated like the title of the group. - As Tim mentioned they are collapsible Screenshot: http://gyazo.com/26d0c7eda57989ede5ba116ae3a0de9b.png
Created attachment 22063 [details] screenshot of console.group usage in Firebug
Created attachment 22082 [details] collapsible Features: - I added GroupTitle Message Level - Groups are collapsible by clicking on the group title message Todo: - group title disappear when collapsed - design
Created attachment 22085 [details] comparison screenshot very minor details console.group gets included in the group and console.groupEnd doesn't. This is the opposite of firebug. Also firebug doesn't indent groupEnd but is in the group(it collapses). They don't bug me but should we do the same?
Created attachment 22097 [details] improved I added triangles to show that they are collapsible. Title now shows fine. Bug: Top level group gets nested when you go to another page without ending a group.
Can you attach an updated screenshot?
Created attachment 22125 [details] Bug fixed Sorry the previous patch might have been corrupted. I was able to fix the bug. I left out m_groupLevel=0; in InspectorController::didCommitLoad.
Created attachment 22126 [details] New screenshot here is the screen shot
(In reply to comment #14) > console.group gets included in the group and console.groupEnd doesn't. This is > the opposite of firebug. This will probably be fixed by bug 19931.
Comment on attachment 22125 [details] Bug fixed + 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!
Created attachment 22344 [details] Improved Sorry the quality of the previous patch was awful. I thought group deserves a javascript class so I added WebInspector.ConsoleGroup.
Created attachment 22347 [details] Minor fix I forgot to put in event.preventDefault
Comment on attachment 22347 [details] Minor fix You've introduced a number of tab characters into the changelog, WebCore/page/Console.idl alas i'm not up to the task of doing any real inspector patch reviewing
Created attachment 22405 [details] Tabs replaced I'll run a script to check for tabs from now on.
Comment on attachment 22405 [details] Tabs replaced Please remove the red color. +.console-group .console-group { + border-left: 10px solid rgb(255, 0, 0); +} This would be better as margin-left: 10px; Otherwise this is ready to land!
Created attachment 22446 [details] Changed css I changed .console-group .console-group { border-left: 10px solid rgb(255, 0, 0); } to .console-group .console-group > .console-group-messages { margin-left: 10px; } This way group title indentation matches Firebug.
Created attachment 22447 [details] Screenshot Maybe the groups aren't apparent enough? More margin or some kind of background?
(In reply to comment #27) > Created an attachment (id=22447) [edit] > Screenshot > > Maybe the groups aren't apparent enough? More margin or some kind of > background? > I think the disclosure triangle is too far from the group title. Maybe move it over to the right more. ANd the indent might need to be more, I don't think a background color is suitable. Try 16px and reducing the space between the disclosure triangle and group title.
Created attachment 22452 [details] Fixed css 16px margin looks perfect!!
The new version looks better! I still think the disclosure arrow should be closer to the group title.
Try: .console-group-title-level { background-image: url(Images/disclosureTriangleSmallDown.png); background-repeat: no-repeat; background-position: 6px 0; font-weight: bold; } .console-group.collapsed .console-group-title-level { background-image: url(Images/disclosureTriangleSmallRight.png); }
Comment on attachment 22452 [details] Fixed css Try moving the disclosure arrow over to the right a bit. See my comment about shifting it 6px. Might look good with 8px too.
Comment on attachment 22452 [details] Fixed css I am going to tweak the look locally and land.
Landed in r35421.
Found a major bug after landing. Filed as bug 20210.