Bug 19158 – Inspector should support console.group/console.groupEnd

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

The Inspector should support console.group/console.groupEnd for Firebug parity.

[Mark Rowe (bdash)]

<rdar://problem/5950862>

[Keishi Hattori]

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?

[Adam Roben (:aroben)]

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?

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 22024 [details]
proposed patch

Prompt and CommandMesssages get properly indented.

[Keishi Hattori]

Oops, doesn't work when you close and reopen the inspector.

[Keishi Hattori]

Created attachment 22026 [details]
minor mistake fix

fixed.

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

Tim might have some good ideas for how to indent the messages properly.

[Timothy Hatcher]

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.

[Keishi Hattori]

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

[Adam Roben (:aroben)]

Created attachment 22063 [details]
screenshot of console.group usage in Firebug

[Keishi Hattori]

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

[Keishi Hattori]

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?

[Keishi Hattori]

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.

[Timothy Hatcher]

Can you attach an updated screenshot?

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 22126 [details]
New screenshot

here is the screen shot

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

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!

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 22347 [details]
Minor fix

I forgot to put in event.preventDefault

[Oliver Hunt]

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

[Keishi Hattori]

Created attachment 22405 [details]
Tabs replaced

I'll run a script to check for tabs from now on.

[Timothy Hatcher]

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!

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 22447 [details]
Screenshot

Maybe the groups aren't apparent enough? More margin or some kind of background?

[Timothy Hatcher]

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

[Keishi Hattori]

Created attachment 22452 [details]
Fixed css

16px margin looks perfect!!

[Timothy Hatcher]

The new version looks better! I still think the disclosure arrow should be closer to the group title.

[Timothy Hatcher]

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);
}

[Timothy Hatcher]

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.

[Timothy Hatcher]

Comment on attachment 22452 [details]
Fixed css

I am going to tweak the look locally and land.

[Timothy Hatcher]

Landed in r35421.

[Timothy Hatcher]

Found a major bug after landing. Filed as bug 20210.