664131 - Expand console object with group methods that indent future console messages in order to create separate blocks of visually combined output

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

As described in bug 644596, we have to implement the rest of the de-facto standard methods in the console object. This bug will track the work for the group, groupCollapsed and groupEnd methods.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

A simple implementation for group/groupEnd that does not allow collapsing the grouped messages.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

The simplest implementation possible. Proper group labels are displayed, new messages are indented as expected, but you can't collapse the groups, like you can in Firebug and WebKit Inspector. Group collapsing will be implemented in a followup bug. Here is a screencast showing this in action:

http://vimeo.com/25082943

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 539467 [details] [diff] [review]
Patch v2

Review of attachment 539467 [details] [diff] [review]:
-----------------------------------------------------------------

Good work!

Giving the patch r+, but please address the comments. Thank you!

::: dom/base/ConsoleAPI.js
@@ +186,5 @@
> +  /**
> +   * Begin a new group for logging output together.
> +   **/
> +  beginGroup: function CA_beginGroup() {
> +    this.groupDepth++;

This is unused.

Do we want to track this here? It's also tracked in HUDService.

@@ +187,5 @@
> +   * Begin a new group for logging output together.
> +   **/
> +  beginGroup: function CA_beginGroup() {
> +    this.groupDepth++;
> +    return Array.prototype.slice.call(arguments[0]).join(" ");

Why slice() then join()?

return Array.prototype.join.call(arguments[0], " ");

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +5540,5 @@
>      // long multi-line messages.
>      let iconContainer = aDocument.createElementNS(XUL_NS, "xul:vbox");
>      iconContainer.classList.add("webconsole-msg-icon-container");
> +    // Apply the curent group by indenting appropriately.
> +    iconContainer.style.margin = "0 0 0 " + HUDService.groupDepth * GROUP_INDENT + "px";

You can change only iconContainer.style.marginLeft.

What happens when console.group() is called many times? I think you should limit how much marginLeft can grow, based on the richlistbox width. I was able to overflow the Web Console output with sufficient numerous calls to group().

::: toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_664131_console_group.js
@@ +24,5 @@
> +  content.console.group("a");
> +  findLogEntry("a");
> +  let msg = outputNode.querySelectorAll(".webconsole-msg-icon-container");
> +  is(msg.length, 1, "one message node displayed");
> +  is(msg[0].style.margin, "0pt 0pt 0pt " + GROUP_INDENT + "px", "correct group indent found");

Please only check style.marginLeft.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

(In reply to comment #3)
> Comment on attachment 539467 [details] [diff] [review] [review]
> Patch v2
> 
> Review of attachment 539467 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Good work!
> 
> Giving the patch r+, but please address the comments. Thank you!
> 
> ::: dom/base/ConsoleAPI.js
> @@ +186,5 @@
> > +  /**
> > +   * Begin a new group for logging output together.
> > +   **/
> > +  beginGroup: function CA_beginGroup() {
> > +    this.groupDepth++;
> 
> This is unused.
> 
> Do we want to track this here? It's also tracked in HUDService.

You are absolutely right. After the latest refactoring this is no longer needed. Removed.

> @@ +187,5 @@
> > +   * Begin a new group for logging output together.
> > +   **/
> > +  beginGroup: function CA_beginGroup() {
> > +    this.groupDepth++;
> > +    return Array.prototype.slice.call(arguments[0]).join(" ");
> 
> Why slice() then join()?
> 
> return Array.prototype.join.call(arguments[0], " ");

No reason. Fixed.

> ::: toolkit/components/console/hudservice/HUDService.jsm
> @@ +5540,5 @@
> >      // long multi-line messages.
> >      let iconContainer = aDocument.createElementNS(XUL_NS, "xul:vbox");
> >      iconContainer.classList.add("webconsole-msg-icon-container");
> > +    // Apply the curent group by indenting appropriately.
> > +    iconContainer.style.margin = "0 0 0 " + HUDService.groupDepth * GROUP_INDENT + "px";
> 
> You can change only iconContainer.style.marginLeft.

Ah yes, that simplifies things a bit.

> What happens when console.group() is called many times? I think you should
> limit how much marginLeft can grow, based on the richlistbox width. I was
> able to overflow the Web Console output with sufficient numerous calls to
> group().

The behavior is identical to what webkit and firebug do: when too many nested groups have been started, the console output grows larger and we get a horizontal scrollbar.

> :::
> toolkit/components/console/hudservice/tests/browser/
> browser_webconsole_bug_664131_console_group.js
> @@ +24,5 @@
> > +  content.console.group("a");
> > +  findLogEntry("a");
> > +  let msg = outputNode.querySelectorAll(".webconsole-msg-icon-container");
> > +  is(msg.length, 1, "one message node displayed");
> > +  is(msg[0].style.margin, "0pt 0pt 0pt " + GROUP_INDENT + "px", "correct group indent found");
> 
> Please only check style.marginLeft.

Done.

Thanks for the review!

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v4

Rebased against latest fx-team repo.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v5

Rebased due to the creation of the new devtools module. This patch has already received an r+ from Mihai who is a devtools module peer, so Gavin, I think you only need to look at the dom/ bits, and perhaps more importantly, with your sr hat on.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 548761 [details] [diff] [review]
Patch v5

>diff --git a/browser/devtools/webconsole/HUDService.jsm b/browser/devtools/webconsole/HUDService.jsm

>   /**
>+   * The nesting depth of the currently active console group.
>+   */
>+  groupDepth: 0,

Won't tracking this globally mean that e.g. two tabs that are both calling group()/groupEnd() concurrently will interfere with each other? That doesn't seem desirable.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v6

(In reply to comment #7)
> Comment on attachment 548761 [details] [diff] [review] [diff] [details] [review]
> Patch v5
> 
> >diff --git a/browser/devtools/webconsole/HUDService.jsm b/browser/devtools/webconsole/HUDService.jsm
> 
> >   /**
> >+   * The nesting depth of the currently active console group.
> >+   */
> >+  groupDepth: 0,
> 
> Won't tracking this globally mean that e.g. two tabs that are both calling
> group()/groupEnd() concurrently will interfere with each other? That doesn't
> seem desirable.

You are absolutely right! This version stores the current depth in the HeadsUpDisplay object which is unique per tab. I also rebased the patch on top of the latest fx-team.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Just a note for future reference - it would be ideal to generate patches with at least 8 lines of context. This patch in particular is rather sucky to review with the default 3 lines :) https://developer.mozilla.org/en/Installing_Mercurial#Configuration has some config examples.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 550706 [details] [diff] [review]
Patch v6

I don't think this is suitable either, since a HUD object is still shared amongst a top-level window and all of its child frames, so frames from different contexts can still interfere with each other. I think you'll need a solution similar to the one for bug 658368 (perhaps code can even be shared for storing per-window state like this).

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gavin Sharp from comment #9)
> Just a note for future reference - it would be ideal to generate patches
> with at least 8 lines of context. This patch in particular is rather sucky
> to review with the default 3 lines :)
> https://developer.mozilla.org/en/Installing_Mercurial#Configuration has some
> config examples.

Sorry about that, I'm using 3 lines of context (per jorendorff's suggestion) to reduce the merge conflicts with mq. I was just being lazy and uploaded the mq patch file as-is, instead of using hg diff to get 8 lines of context.

This is the relevant part of my configuration:

[defaults]
commit = -v
diff=-U 8 -p
qdiff=-U 8
qnew = -U

[diff]
git = 1
showfunc = 1

Comment 12

[Rob Campbell [:rc] (:robcee)]

Panos, I suggested in bug 669658 that we do a manual qdiff -U 8 (looks like you've already got that set, goody!) for producing patches for review.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gavin Sharp from comment #10)
> Comment on attachment 550706 [details] [diff] [review]
> Patch v6
> 
> I don't think this is suitable either, since a HUD object is still shared
> amongst a top-level window and all of its child frames, so frames from
> different contexts can still interfere with each other. I think you'll need
> a solution similar to the one for bug 658368 (perhaps code can even be
> shared for storing per-window state like this).

Let me see if I understand the failure scenario. Pointing two different tabs at http://htmlpad.org/group with consoles opened does not seem to cause interference in the grouping, after the fix in v6. That is, if you only click the outer button. On the other hand, clicking on the button in the iframe causes the log message grouping in the iframe to be dependent on the grouping level of the outer frame.

This however is consistent with what the other browsers are doing (tested on Chrome, Safari, Opera and Firebug). In my mind this makes sense, because the web console is visually connected to the tab, so I would expect console operations from anywhere in the tab's contents to be interfering with each other. I can see some benefits in treating iframes differently, but I'm not sure if this is worth the deviation from what the rest of the web is doing?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Hmm, perhaps you're right. Seems odd that different frames can share that same state, but I guess that's never really a problem in practice (and only an annoyance even if it does somehow come up).

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gavin Sharp from comment #14)
> Hmm, perhaps you're right. Seems odd that different frames can share that
> same state, but I guess that's never really a problem in practice (and only
> an annoyance even if it does somehow come up).

Did you get a chance to think about this some more? Can we land this?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 550706 [details] [diff] [review]
Patch v6

Sure - you should feel free to poke me directly when I'm not responding to bugmail (I only just saw your question now).

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gavin Sharp from comment #16)
> Comment on attachment 550706 [details] [diff] [review] [diff] [details] [review]
> Patch v6
> 
> Sure - you should feel free to poke me directly when I'm not responding to
> bugmail (I only just saw your question now).

Will do, thanks!

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: [in-fx-team] Patch v7

Rebased patch on top of latest fx-team.
Try submission results at:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=421c5376c30f

Comment 19

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 562386 [details] [diff] [review]
[in-fx-team] Patch v7

https://hg.mozilla.org/integration/fx-team/rev/e7facdd9040b

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e7facdd9040b

Comment 21

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/Using_the_Web_Console#The_console_object

Also mentioned on Firefox 9 for developers