Jordan Santell [:jsantell] [@jsantell] Reporter
	Description • 10 years ago

We have documentation for it[1], and while it just falls back to `console.group`, we should atleast have a bug tracking this.

[1]https://developer.mozilla.org/en-US/docs/Web/API/Console.groupCollapsed

	Arthur Stolyar [:nekr]
	Comment 1 • 10 years ago

Hi all,

just finished my work on first patch to Firefox. It should handle this bug, and this: https://bugzilla.mozilla.org/show_bug.cgi?id=1088355

What was done:

* In general added ability to create nested groups and make them collapsable
* Create new type of message inside conole-output.js which is ConsoleGroup
* In addition to .message className, now messages have [message] attribute. Difference is that className is for visual and attribute for service, like JS manipulations.
* All selector queries from JS which picks up messages now uses [message] attribute
* Used .theme-twisty class as a expand icon
* ConsoleGroup message has container for new message and title. Title is ConsoleGeneric, but without [message] attibute
* Prev. list item also resolves bug #1088355, because used same message type for title which is used for regular logs. But still depends on it: #1088900

Design metrics and styles did by myself, they are not best, but visualy it looks ok, I belive. If it look good, it also might resolve this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=755534

One note: there is not test for work that did. I am not clearly understand how to write those test and how it'll be tested in xpshell.

Thanks.
Arthur

	Arthur Stolyar [:nekr]
	Comment 2 • 10 years ago

Oh, one more thing:

Limit on message is handled in this way:

Group does not represent one message, it represet all inner messages. When messages inside group reaches the limit, they are just removed from the group, not a whole group. If group has not messages or just removed message was last in the group, that group is removed.

	Arthur Stolyar [:nekr]
	Comment 3 • 10 years ago

Found bug that group is not removed when it's empty and reached message limit.

There are also thing under question:
If group is |private|, should it accept only private messages? If no, how it should be handled?

	Jordan Santell [:jsantell] [@jsantell] Reporter
	Comment 5 • 10 years ago

Thanks nekr for the patch -- we'll need some tests to go with it before we merge though, atleast -- I'll find someone more familiar with the webconsole to review this then as well

	Arthur Stolyar [:nekr]
	Comment 6 • 10 years ago

> Thanks nekr for the patch -- we'll need some tests to go with it before we merge though

Yes, I understood that tests are required. I just need little help to figure out how to write them correctly. Will ask about it in IRC when I have more free time.

	Arthur Stolyar [:nekr]
	Comment 7 • 10 years ago

Oops. Did something wrong :( Made second patch, put it's diff of first patch, not all changes. hg qref does not for me on Windows, sorry.. How can I add patch with whole differences?

	Jordan Santell [:jsantell] [@jsantell] Reporter
	Comment 8 • 10 years ago

I just stick to the git checkout of it since I always mess up mercurial patches :\

	Arthur Stolyar [:nekr]
	Comment 9 • 10 years ago

Finally figured out how to merge two patches, sorry that mess.

	Arthur Stolyar [:nekr]
	Comment 10 • 10 years ago

Also, in IRC :past was suggested to reviewers. I knew you are busy right now. Added him just to track this bug.
If I am wrong, feel free to remove :past from reviewers.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 11 • 10 years ago

Comment on attachment 8516149 [details] [diff] [review]
bug1088360.patch

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

Sorry for the delay in reviewing this, I played with it briefly and it looks good! I'll try to do a proper review early next week. In the meantime, the patch needs rebasing. There is also a bunch of trailing whitespace in the patch, please remove it.

	Arthur Stolyar [:nekr]
	Comment 12 • 10 years ago

> Sorry for the delay in reviewing this, I played with it briefly and it looks good! I'll try to do a
> proper review early next week.

That is good, thanks!

> I'll try to do a proper review early next week. In the meantime, the patch needs rebasing.

Is it something that I can fix in the code or it is just a fact and there are not problems for it?

> There is also a bunch of trailing whitespace in the patch, please remove it.

Ok, I will look closely for this problem on weekend and fix it.

	Arthur Stolyar [:nekr]
	Comment 13 • 10 years ago

Updated as requested.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 14 • 10 years ago

Comment on attachment 8523606 [details] [diff] [review]
bug1088360.patch

Don't forget to set the r? flag if you want me to see it any time soon ;-)
I'll try to get to it tomorrow.

	Arthur Stolyar [:nekr]
	Comment 15 • 10 years ago

Yes, I am sorry. I missed that r? attached not to entire bug, but to specific patch :(

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 16 • 10 years ago

Comment on attachment 8523606 [details] [diff] [review]
bug1088360.patch

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

After playing with this for a while, I sure like how it looks. The patch is not ready however, as it causes a lot of devtools mochitests to fail. Try running "mach mochitest-devtools browser/devtools/webconsole" to see the failures.

One thing that I'm concerned about is performance. At one point, when I was playing with jsbin.com, I got the console to beachball while opening. I didn't try to investigate this yet, so it could be a fluke, but things like getAllMessages() relying on querySelectorAll() can be an issue here. Console performance is something that we should be very careful not to regress.

Still, this is very nice work, kudos!

::: browser/devtools/webconsole/console-output.js
@@ +502,3 @@
>      container.className = "message";
> +
> +    if (isFinite(id)) {

Number.isFinite() please, for extra safety.

@@ +845,5 @@
>  
>      let body = this._renderBody();
> +
> +    // do not want this for groups since they are not repeatable
> +    // and might contain lot of textContent

I don't think this is right, we would still want to avoid repeating identical messages in the same group and increment the counter instead.

@@ +861,5 @@
>  
>      this.element.appendChild(timestamp.element);
>      this.element.appendChild(indentNode);
> +
> +    if (icon) {

What's the if here for? We are unconditionally creating the icon element, aren't we?

@@ +1261,5 @@
>   * @extends Messages.Extended
>   * @param object packet
>   *        The Console API call packet received from the server.
>   */
> +Messages.ConsoleGeneric = function(packet, extOptions = {})

All arguments must have comments. In this particular case you should also describe the behavior of the quoteStrings option with respect to null and undefined.

@@ +1271,5 @@
>      severity: CONSOLE_API_LEVELS_TO_SEVERITIES[packet.level],
>      private: packet.private,
>      filterDuplicates: true,
> +    quoteStrings: extOptions.quoteStrings == null ? true :
> +      extOptions.quoteStrings,

Why not simply:

quoteStrings: extOptions.quoteStrings ? true : false;

If the reason is backwards compatibility, then it's better to change all call sites instead of having a counterintuitive behavior.

@@ +2024,5 @@
> +    timestamp: packet.timeStamp,
> +    category: "webdev",
> +    severity: CONSOLE_API_LEVELS_TO_SEVERITIES[packet.level],
> +    private: packet.private,
> +    filterDuplicates: false,

I think we want to coalesce duplicate entries.

@@ +2095,5 @@
> +   *
> +   * @private
> +   * @type boolean
> +   */
> +  _gotChildren: false,

Nit: _hasChildren is a little better.

::: browser/devtools/webconsole/webconsole.js
@@ +376,5 @@
>    // Width of the monospace characters in Web Console's input box.
>    _inputCharWidth: 0,
>  
> +  getLastMessage: function WCF_getLastMessage() {
> +    var lastChild = this.outputNode.lastElementChild;

Nit: lastChild can be inlined as it doesn't affect the groupDepth>0 case.

@@ +386,5 @@
> +    }
> +
> +    return lastChild;
> +  },
> +  insertMessage: function WCF_insertMessage(messageNode, afterMessage) {

Nit: empty lines between methods please.

@@ +2474,5 @@
>     *
>     * @param nsIDOMNode aNode
>     *        The message node you want to remove.
>     */
> +  removeOutputMessage: function WCF_removeOutputMessage(aNode, ignoreParent)

Add a comment for the new parameter please.

@@ +3932,5 @@
>     */
>    clearPrivateMessages: function JST_clearPrivateMessages()
>    {
> +    let nodes = this.hud.outputNode.querySelectorAll(
> +      "[message][private]:not([group=true])");

Why should we not respect user privacy for grouped messages?

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +166,5 @@
>    flex-direction: column;
>    align-items: flex-start;
>  }
>  
> +#output-container.hideTimestamps .message {

Can we find a more performant way to do this?

There is some useful advice in the wiki and MDN about CC performance:
https://wiki.mozilla.org/DevTools/CSSTips#Performance

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 17 • 10 years ago

(In reply to Arthur Stolyar [:nekr] from comment #12)
> > I'll try to do a proper review early next week. In the meantime, the patch needs rebasing.
> 
> Is it something that I can fix in the code or it is just a fact and there
> are not problems for it?

By rebasing, I meant that you should update your workspace to the latest tip and rebase the patch on top of that. That way I can just import your patch and go, instead of having to deal with patch conflicts.

	Arthur Stolyar [:nekr]
	Comment 18 • 10 years ago

> After playing with this for a while, I sure like how it looks. The patch is not ready however, as it
> causes a lot of devtools mochitests to fail. Try running "mach mochitest-devtools 
> browser/devtools/webconsole" to see the failures.

I currently need fully rebuild project because some bug in build system.. I will try as soon as possible.

> but things like getAllMessages() relying on querySelectorAll() can be an issue here.
> Console performance is something that we should be very careful not to regress.

Do you think getElementsByClassName is faster? I understand what complex selectors is slower, but move from .message to [message] is absolutely required.
I see here one solution -- permanently have in memory Array/Set with all messages.

> I don't think this is right, we would still want to avoid repeating identical messages in the same 
> group and increment the counter instead.

That should not prevent identical messages in group to be combined in one. That should prevent combining identical groups. Correct me if I am wrong. In fact, I think I did not test this part and there might be bug.

> What's the if here for? We are unconditionally creating the icon element, aren't we?

This is artifact of my first implementation version. I added additional method for icon creation like in body case, so classes which inherits from that class might override icon creation (or remove it). That method exist in patch, but I can remove it if that is bad decision from me.

> Why should we not respect user privacy for grouped messages?
I mentioned this in #3 comment:
> There are also thing under question:
> If group is |private|, should it accept only private messages? If no, how it should be handled?

Absolutely patch is not complete without fixing this problem, but no one answered to me. As I understand, when chrome console (Ctrl+Shift+J) is opened it uses same data what is in tab console. There should be special case for it, but I currently did not found it.

> Can we find a more performant way to do this?

I removed there only |>| sign, so it was already not really fast. But yes, I can make it better.

_____

All other will be fixed.

> By rebasing, ...

Understood, thank you.

P.S.
I am not clearly understand workflow with flags, what means |feedback+| flag?

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 19 • 10 years ago

> Do you think getElementsByClassName is faster? I understand what complex
> selectors is slower, but move from .message to [message] is absolutely
> required.

It should be, as it would avoid the query selector parsing step.

> That should not prevent identical messages in group to be combined in one.
> That should prevent combining identical groups. Correct me if I am wrong. In
> fact, I think I did not test this part and there might be bug.

I agree about not combining identical groups. I didn't try that, so if that's the case then a new test for this would be warranted.

> > What's the if here for? We are unconditionally creating the icon element, aren't we?
> 
> This is artifact of my first implementation version. I added additional
> method for icon creation like in body case, so classes which inherits from
> that class might override icon creation (or remove it). That method exist in
> patch, but I can remove it if that is bad decision from me.

I'm not sure I follow: are you expecting add-ons to override _renderIcon() and not create an icon or something else? Why would that be useful?

> > Why should we not respect user privacy for grouped messages?
> I mentioned this in #3 comment:
> > There are also thing under question:
> > If group is |private|, should it accept only private messages? If no, how it should be handled?
> 
> Absolutely patch is not complete without fixing this problem, but no one
> answered to me. As I understand, when chrome console (Ctrl+Shift+J) is
> opened it uses same data what is in tab console. There should be special
> case for it, but I currently did not found it.

Private messages come from windows in private browsing mode. Therefore, when that window is closed, we should clear every message originating from that window. Does that make sense?

> I am not clearly understand workflow with flags, what means |feedback+| flag?

f+ is a way of saying "I like the direction this patch is going", but it's not ready to land, otherwise I would have marked it with r+.

	Arthur Stolyar [:nekr]
	Comment 20 • 10 years ago

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #19)
> 
> I'm not sure I follow: are you expecting add-ons to override _renderIcon()
> and not create an icon or something else? Why would that be useful?

Some sub-classes inherits from that class and overrides _renderBody() method for example. But there was no way to override icon creation. Existence of this method provides that ability.

> 
> Private messages come from windows in private browsing mode. Therefore, when
> that window is closed, we should clear every message originating from that
> window. Does that make sense?

Yes and no. I think I need investigate code in more depth. I will try find time on this week to fix issues with this patch.

> 
> f+ is a way of saying "I like the direction this patch is going", but it's
> not ready to land, otherwise I would have marked it with r+.

Thank you for clarification. I understood now.

	michael.heuberger
	Comment 22 • 9 years ago

any news on this one? looks like it's an old ticket but it's still bugging me. chrome does it correctly ...

	Arthur Stolyar [:nekr]
	Comment 23 • 9 years ago

I have not much free time to complete this bug. Building Firefox on my PC takes almost a day to complete, so work was stopped once I was needed to build whole project again (incremental build was broken for me).

I am not sure if I will be able to complete it at all, so any one are welcome to update the patch.

	michael.heuberger
	Comment 24 • 9 years ago

totally understand, but can't you delegate this to someone else then?

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 25 • 9 years ago

Delegate it to whom? Arthur was just volunteering his time to fix this bug, for which we are absolutely grateful! I'm resetting the assignee, to indicate anyone is free to jump in on this one, but that shouldn't stop him from getting back to it if time permits.

CCing Jeff to assess the priority of this bug among the many other things we have to work on.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 26 • 9 years ago

(In reply to Panos Astithas [:past] from comment #25)
> Delegate it to whom? Arthur was just volunteering his time to fix this bug,
> for which we are absolutely grateful! I'm resetting the assignee, to
> indicate anyone is free to jump in on this one, but that shouldn't stop him
> from getting back to it if time permits.
> 
> CCing Jeff to assess the priority of this bug among the many other things we
> have to work on.

Thanks Panos. This bug blocks bug 1088793, which is on the devedition-40 list - I've also added the chrome-parity whiteboard

Generally speaking we want to improve console look & feel over the next quarter or so and this is one of the key issues, people have created some fancy utilities that rely on groupCollapsed. For the current cycle there seem to be higher priority issues.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 27 • 8 years ago

Hi, 
I'm going to finish this one in a bit.
Is it safe if I refer to the existing patch, or should I restart from scratch ?

	Brian Grinstead [:bgrins]
	Comment 28 • 8 years ago

(In reply to Nicolas Chevobbe from comment #27)
> Hi, 
> I'm going to finish this one in a bit.
> Is it safe if I refer to the existing patch, or should I restart from
> scratch ?

I think it's alright to refer to the existing patch with a couple of caveats.  First, I'm sure it won't apply cleanly so it'll need a rebase.  Next, the whole concept of 'afterMessage' has been removed as of Bug 1044365 which should hopefully lead to some simplification across the patch.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 29 • 8 years ago

As discussed on IRC, here's a patch of my WIP on this bug. This patch is functional but I did not ran test with it.

	Brian Grinstead [:bgrins]
	Comment 30 • 8 years ago

Comment on attachment 8740129 [details] [diff] [review]
collapsible console.group

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

OK so the implementation looks good.  As we discussed on IRC, I'm afraid we'd need to immediately redo new tests / implementation added here in https://github.com/bgrins/gecko-dev/tree/console-frontend.  As such, let's give it a go for implementing there (reusing as much as possible from this), and come back to this patch depending on the timeline for that project

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 31 • 8 years ago

This will be fixed in the new console in Bug 1307873

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 32 • 8 years ago

Fixed with https://hg.mozilla.org/mozilla-central/rev/4b037d091109

	Sebastian Zartner [:sebo]
	Comment 33 • 8 years ago

I can confirm that it works in Nightly 2016-10-10 as expected while it didn't work before.

I've also updated the compatibility note at https://developer.mozilla.org/en-US/docs/Web/API/Console/groupCollapsed and mentioned it in the developer release notes at https://developer.mozilla.org/en-US/Firefox/Releases/52.

Sebastian

	mh
	Comment 34 • 7 years ago

collapsed groups still fall back to regular, uncollapsible groups as of Firefox for Developers 53 and Firefox 52 beta.

	console.groupCollapsed('group');
	console.log('test');
	
	console.groupCollapsed('group nested');
	console.log('test 2');
	console.groupEnd();
	
	console.groupEnd();

Screenshot:
http://prntscr.com/e32q4d

	mh
	Comment 35 • 7 years ago

Collapsed groups are collapsible only in Firefox Nightly (54) as of 2017-02-01 (although there are other issues in the dev console e.g. cannot turn Server logging on - all buttons to enable/disable contexts disappeared).

I reckon some of the changes have not been backported to v52 after all.

	mh
	Comment 36 • 7 years ago

I know now that "groupCollapsed" is being handled only in "new-frontend" (i have not seen that it has been mentioned anywhere here), but devtools.webconsole.new-frontend-enabled is still false in 52 and 53. It's set to true by default in nigthly 54, so that's why I could see collapsed groups.

Nevertheless, there is a regression in "new-frontend" which does not allow to show Server logs, nor there is a filter option available for it, so I am going to fill in a new bug report for that.

So after all, I would really like to see groupCollapsed implemented even in "old frontend" for devtools... which is still the default frontend for v52 and v53 from what I see.

	peter
	Comment 37 • 7 years ago

Current situation May 28 2017: 
- Regular Firefox 53.0.3: not supported
- Regular Firefox 53.0.3 with new-debugger-frontend enabled: not supported
- Firefox Developer Edition 54.0a2 (2017-05-11) (new-debugger-frontend enabled by default): not supported
- Firefox Nightly 55.0a1 (2017-05-28) (new-debugger-frontend enabled by default): supported

Any indication of when this will land in the regalar and developer edition? Or with what version?

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 38 • 7 years ago

(In reply to peter from comment #37)
...
> - Firefox Nightly 55.0a1 (2017-05-28) (new-debugger-frontend enabled by
> default): supported
> 
> Any indication of when this will land in the regalar and developer edition?
> Or with what version?

Firefox 55 will move to Beta / Developer Edition on June 13, and will be released August 9th.

We are in the middle of eliminating the "Aurora" channel of releases that Developer Edition was formerly based on, so starting with Beta 55 Developer Edition users will be updated to a version of Beta built specifically for them.

	peter
	Comment 39 • 7 years ago

Current situation in Juli 15 2017:
- Firefox Developer Edition 55.0b9: not supported
- Firefox Developer Edition 55.0b9 with new-debugger-frontend enabled : not supported
- Firefox Nightly 56.0a1 (2017-07-14) (new-debugger-frontend enabled by default): supported

I was hoping it would be included in Developer Edition 55, since it was supported in Firefox Nightly 55.0a1.

	Sebastian Zartner [:sebo]
	Comment 40 • 7 years ago

You toggled the wrong preference. devtools.debugger.new-debugger-frontend is for the Debugger panel UI. The right one for this feature is devtools.webconsole.new-frontend-enabled, which toggles the new Console UI. The preference is enabled by default in Nightly since Firefox 52[1][2], though it didn't get shipped yet. The bug to enable the pref in other versions is bug 1308219.

Sebastian

[1] Bug 1304178
[2] https://developer.mozilla.org/en-US/Firefox/Experimental_features#new-console-frontend

	peter
	Comment 41 • 7 years ago

Thanks for the correction, that totally works.