Bug 19158

Summary: Inspector should support console.group/console.groupEnd
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Web Inspector (Deprecated)Assignee: Keishi Hattori <keishi>
Severity: Enhancement CC: timothy
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 14354    
Description Flags
aroben: review-
interim report
proposed patch
minor mistake fix
aroben: review-
screenshot of console.group usage in Firebug
comparison screenshot
Bug fixed
aroben: review-
New screenshot
Minor fix
Tabs replaced
timothy: review-
Changed css
Fixed css timothy: review+

Description Adam Roben (:aroben) 2008-05-20 16:16:46 PDT
The Inspector should support console.group/console.groupEnd for Firebug parity.
Comment 1 Mark Rowe (bdash) 2008-05-20 16:23:09 PDT
Comment 2 Keishi Hattori 2008-06-28 05:49:00 PDT
Created attachment 21986 [details]

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 3 Adam Roben (:aroben) 2008-06-28 09:14:08 PDT
Comment on attachment 21986 [details]

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?
Comment 4 Keishi Hattori 2008-06-30 22:20:40 PDT
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.
Comment 5 Keishi Hattori 2008-07-01 05:05:55 PDT
Created attachment 22024 [details]
proposed patch

Prompt and CommandMesssages get properly indented.
Comment 6 Keishi Hattori 2008-07-01 05:33:03 PDT
Oops, doesn't work when you close and reopen the inspector.
Comment 7 Keishi Hattori 2008-07-01 05:56:57 PDT
Created attachment 22026 [details]
minor mistake fix

Comment 8 Adam Roben (:aroben) 2008-07-01 07:53:50 PDT
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.
Comment 9 Adam Roben (:aroben) 2008-07-01 07:54:19 PDT
Tim might have some good ideas for how to indent the messages properly.
Comment 10 Timothy Hatcher 2008-07-01 09:54:11 PDT
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.
Comment 11 Keishi Hattori 2008-07-03 05:54:38 PDT
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
Comment 12 Adam Roben (:aroben) 2008-07-03 06:09:39 PDT
Created attachment 22063 [details]
screenshot of console.group usage in Firebug
Comment 13 Keishi Hattori 2008-07-04 00:30:53 PDT
Created attachment 22082 [details]

- I added GroupTitle Message Level
- Groups are collapsible by clicking on the group title message
- group title disappear when collapsed
- design
Comment 14 Keishi Hattori 2008-07-04 01:04:13 PDT
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?
Comment 15 Keishi Hattori 2008-07-05 04:43:00 PDT
Created attachment 22097 [details]

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.
Comment 16 Timothy Hatcher 2008-07-05 08:08:58 PDT
Can you attach an updated screenshot?
Comment 17 Keishi Hattori 2008-07-07 04:14:23 PDT
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.
Comment 18 Keishi Hattori 2008-07-07 04:15:38 PDT
Created attachment 22126 [details]
New screenshot

here is the screen shot
Comment 19 Adam Roben (:aroben) 2008-07-07 12:44:48 PDT
(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 20 Adam Roben (:aroben) 2008-07-10 08:21:45 PDT
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";
+            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!
Comment 21 Keishi Hattori 2008-07-17 11:41:25 PDT
Created attachment 22344 [details]

Sorry the quality of the previous patch was awful.

I thought group deserves a javascript class so I added WebInspector.ConsoleGroup.
Comment 22 Keishi Hattori 2008-07-17 11:54:11 PDT
Created attachment 22347 [details]
Minor fix

I forgot to put in event.preventDefault
Comment 23 Oliver Hunt 2008-07-20 14:23:42 PDT
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
Comment 24 Keishi Hattori 2008-07-21 08:43:01 PDT
Created attachment 22405 [details]
Tabs replaced

I'll run a script to check for tabs from now on.
Comment 25 Timothy Hatcher 2008-07-21 10:42:27 PDT
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!
Comment 26 Keishi Hattori 2008-07-23 04:06:56 PDT
Created attachment 22446 [details]
Changed css

I changed
   .console-group .console-group {
      border-left: 10px solid rgb(255, 0, 0);
   .console-group .console-group > .console-group-messages {
      margin-left: 10px;

This way group title indentation matches Firebug.
Comment 27 Keishi Hattori 2008-07-23 04:12:49 PDT
Created attachment 22447 [details]

Maybe the groups aren't apparent enough? More margin or some kind of background?
Comment 28 Timothy Hatcher 2008-07-23 06:57:15 PDT
(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.
Comment 29 Keishi Hattori 2008-07-23 07:35:26 PDT
Created attachment 22452 [details]
Fixed css

16px margin looks perfect!!
Comment 30 Timothy Hatcher 2008-07-29 10:02:47 PDT
The new version looks better! I still think the disclosure arrow should be closer to the group title.
Comment 31 Timothy Hatcher 2008-07-29 10:05:32 PDT

.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 32 Timothy Hatcher 2008-07-29 10:06:19 PDT
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 33 Timothy Hatcher 2008-07-29 10:40:39 PDT
Comment on attachment 22452 [details]
Fixed css

I am going to tweak the look locally and land.
Comment 34 Timothy Hatcher 2008-07-29 11:08:02 PDT
Landed in r35421.
Comment 35 Timothy Hatcher 2008-07-29 11:21:02 PDT
Found a major bug after landing. Filed as bug 20210.