1088900 - Need parse console.group and console.groupCollapsed with styles input line console.log

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Arthur Stolyar [:nekr]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36

Comment 1

[Arthur Stolyar [:nekr]]

Messages |group| and |groupCollapsed| from backend did not have |styles| field, therefore cannot implement styling.

http://mxr.mozilla.org/mozilla-central/source/dom/base/Console.cpp#1099

Comment 2

[Brian Grinstead [:bgrins]]

Yes, I think we'll need to process arguments for console.group and groupCollapsed in the same way that they are processed for the logging methods.

Comment 4

[Koala Yeung]

3 years old issue. Any work in progress?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

yes, it's on my todo list

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1088900 - Parse console.group/groupCollapse for custom styling.

Process the arguments of "MethodGroup" and "MethodGroupCollapsed"
Use the result of ProcessArguments to compute the groupName so we can get
rid of the "%c" parts.

Review commit: https://reviewboard.mozilla.org/r/128842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128842/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1088900 - Add backend tests for console.group custom styling.

Review commit: https://reviewboard.mozilla.org/r/128844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128844/

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1088900 - Use ConsoleMessage's "parameters" property for console.group.

Because we now enable custom styling on console.group, we can't rely solely on groupName
to output the group. We can have multiple pieces on which to apply different styles, like
for console.log, so we use the same properties to have the same behavior for custom styles.

Review commit: https://reviewboard.mozilla.org/r/128846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128846/

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1088900 - Add mocha tests for custom styles on console.group messages.

Tests custom styles on both console.group and console.groupCollapsed messages.
Update the stubs to match what the server now returns.

Review commit: https://reviewboard.mozilla.org/r/128848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128848/

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8856918 [details]
Bug 1088900 - Parse console.group/groupCollapse for custom styling.

https://reviewboard.mozilla.org/r/128842/#review131360

::: dom/console/Console.cpp:1564
(Diff revision 1)
>      case MethodError:
>      case MethodException:
>      case MethodDebug:
>      case MethodAssert:
> +    case MethodGroup:
> +    case MethodGroupCollapsed:

What about MethodGroupEnd?
I guess you should add it here as well.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8856918 [details]
Bug 1088900 - Parse console.group/groupCollapse for custom styling.

https://reviewboard.mozilla.org/r/128842/#review131360

> What about MethodGroupEnd?
> I guess you should add it here as well.

Thanks for the review Andrea !
Since `console.groupEnd` doesn't take any argument (https://console.spec.whatwg.org/#groupend), I don't think we have any reason to parse what may have passed into it. But maybe I'm missing something ?

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8856918 [details]
Bug 1088900 - Parse console.group/groupCollapse for custom styling.

https://reviewboard.mozilla.org/r/128842/#review131360

> Thanks for the review Andrea !
> Since `console.groupEnd` doesn't take any argument (https://console.spec.whatwg.org/#groupend), I don't think we have any reason to parse what may have passed into it. But maybe I'm missing something ?

Our implementation is (currently) different: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Console.webidl#23

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8856918 [details]
Bug 1088900 - Parse console.group/groupCollapse for custom styling.

https://reviewboard.mozilla.org/r/128842/#review131360

> Our implementation is (currently) different: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Console.webidl#23

Interresting, thanks !
I'll add a case for `MethodGroupEnd` as well then.

Comment 14

[Andrea Marchesini [:baku]]

We should check if we want to update our Console API implementation to the latest spec.
Maybe follow up? In case there is not an existing bug.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8856919 [details]
Bug 1088900 - Add backend tests for console.group custom styling.

https://reviewboard.mozilla.org/r/128844/#review131448

::: devtools/server/tests/mochitest/test_webconsole-group-custom-styles.html:1
(Diff revision 1)
> +<!DOCTYPE HTML>

Our backend tests for webconsole are scattered all over.  But most of them are in https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/test so unless if there's a reason to put this one here I'd say move it with the others.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8856920 [details]
Bug 1088900 - Use ConsoleMessage's "parameters" property for console.group.

https://reviewboard.mozilla.org/r/128846/#review131450

Comment 17

[Tom Tromey :tromey]

(In reply to Andrea Marchesini [:baku] from comment #14)
> We should check if we want to update our Console API implementation to the
> latest spec.
> Maybe follow up? In case there is not an existing bug.

There are separate bugs for a change to console.assert and console.count.
Offhand I'm not sure if there are other changes to be done.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8856918 [details]
Bug 1088900 - Parse console.group/groupCollapse for custom styling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128842/diff/1-2/

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8856919 [details]
Bug 1088900 - Add backend tests for console.group custom styling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128844/diff/1-2/

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8856920 [details]
Bug 1088900 - Use ConsoleMessage's "parameters" property for console.group.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128846/diff/1-2/

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8856921 [details]
Bug 1088900 - Add mocha tests for custom styles on console.group messages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128848/diff/1-2/

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8856919 [details]
Bug 1088900 - Add backend tests for console.group custom styling.

https://reviewboard.mozilla.org/r/128844/#review132084

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8856921 [details]
Bug 1088900 - Add mocha tests for custom styles on console.group messages.

https://reviewboard.mozilla.org/r/128848/#review132086

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c060c63c7dc
Parse console.group/groupCollapse for custom styling. r=baku
https://hg.mozilla.org/integration/autoland/rev/31e0c3582035
Add backend tests for console.group custom styling. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/575ccbb27a30
Use ConsoleMessage's "parameters" property for console.group. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/3c6b8512d77f
Add mocha tests for custom styles on console.group messages. r=bgrins

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c060c63c7dc
https://hg.mozilla.org/mozilla-central/rev/31e0c3582035
https://hg.mozilla.org/mozilla-central/rev/575ccbb27a30
https://hg.mozilla.org/mozilla-central/rev/3c6b8512d77f

Comment 26

[Matthew N. [:MattN]]

Please review the screenshots to ensure the styling looks good on all platforms: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=fac2c174087f3ba12ecdd739753bdd93f452ecdb&newProject=mozilla-central&newRev=ff7729561efd16e7c3140d40337a84318730ab9c&filter=_webconsole

Thanks

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #26)
> Please review the screenshots to ensure the styling looks good on all
> platforms:
> https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> central&oldRev=fac2c174087f3ba12ecdd739753bdd93f452ecdb&newProject=mozilla-
> central&newRev=ff7729561efd16e7c3140d40337a84318730ab9c&filter=_webconsole

Thanks for the screenshots - switching from a plain text message to using parameters changed the color on the group title as it is now being highlighted like a string.  I'm not sure if there's a black vs orange, passing onto Nicolas for his opinion.

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

I think we want to keep the same style, sorry for missing that.
I'll file a bug and fix this, it should only be a matter of styling

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

I filed Bug 1356504 for that.