1357473 - Update Console to the latest spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Andrea Marchesini [:baku]]

Not all the static methods have to be updated, some of them are still undocumented (table for instance).

Comment 1

[Andrea Marchesini [:baku]]

Attachment: console_1_webidl.patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: console_2_clear.patch

Comment 3

[Andrea Marchesini [:baku]]

Attachment: console_3_groupEnd.patch

Comment 4

[Andrea Marchesini [:baku]]

Attachment: console_4_trace.patch

Comment 5

[Andrea Marchesini [:baku]]

Attachment: console_5_time.patch

I'm not a super fan of this patch because WebIDL binding takes the JSValue and it does the conversion to nsAString. Then we go from nsAString to JSValue.
But at least we have the default value correctly implemented.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8859261 [details] [diff] [review]
console_1_webidl.patch

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

Are you planning to fold these together? If not please add a commit message

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Where is the spec?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #7)
> Where is the spec?

https://console.spec.whatwg.org/#console-namespace

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859265 [details] [diff] [review]
console_5_time.patch

>+/* static */ void
>+Console::TimeMethod(const GlobalObject& aGlobal, const nsAString& aLabel,
>+                    MethodName aMethodName, const nsAString& aMethodString)
> {
>   JSContext* cx = aGlobal.Context();
> 
>   Sequence<JS::Value> data;
>   SequenceRooter<JS::Value> rooter(cx, &data);
> 
>-  if (!aTime.isUndefined() && !data.AppendElement(aTime, fallible)) {
>+  JS::Rooted<JSString*> str(cx, JS_NewUCStringCopyN(cx, aLabel.BeginReading(),
>+                                                    aLabel.Length()));
>+
>+if (!data.AppendElement(JS::StringValue(str), fallible)) {

Please use ToJSValue, not explicit JS_NewUCStringCopyN

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8859264 [details] [diff] [review]
console_4_trace.patch

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

I believe we will need a follow up bug to print the arguments to console.trace() in the frontend

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8859263 [details] [diff] [review]
console_3_groupEnd.patch

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

Comments have to do with the way we generate fixture data for unit tests.  We'll also need to run `./mach mochitest devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browserconsole_update_stubs_console_api.js` with all the patches applied and commit the changes.

It's a pain right now, so let me attach a patch that folds my changes into this patch

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js
@@ +55,5 @@
>  console.table(['a', 'b', 'c']);
>  `});
>  
>  consoleApi.set("console.group('bar')", {
> +  keys: ["console.group('bar')", "console.groupEnd()"],

This is sort of weird but these are keys used in fixture data for tests.  So we should keep `console.groupEnd('bar')`, `console.groupEnd('foo')`, `console.groupEnd(%cfoo%cbar)`, `console.groupEnd(%cfoo%cbar)` in the `keys` fields, but include the arg removal in `code` blocks lines 62 and 69

::: devtools/client/webconsole/new-console-output/test/store/messages.test.js
@@ +160,5 @@
>  
>      it("does not display console.groupEnd messages to the store", () => {
>        const { dispatch, getState } = setupStore([]);
>  
> +      const message = stubPackets.get("console.groupEnd()");

Related to the previous comment, we want to keep the argument here since it's digging into fixture data that's keyed on the label

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: console_3_groupEnd-WITH-CHANGES.patch

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8859304 [details] [diff] [review]
console_3_groupEnd-WITH-CHANGES.patch

Nicolas, see Comment 11 - this is the original patch plus some stub generation changes folded in.  Can you take over the review to double check my work?

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8859263&action=interdiff&newid=8859304&headers=1

Comment 15

[Andrea Marchesini [:baku]]

Attachment: console_6_exception.patch

Comment 16

[Andrea Marchesini [:baku]]

Attachment: console_7_count.patch

This patch must be rebased when bug 1346326 lands.

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8859308 [details] [diff] [review]
console_6_exception.patch

Moved to bug 1357503.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8859304 [details] [diff] [review]
console_3_groupEnd-WITH-CHANGES.patch

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

The changes looks good to me, thanks.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: console_7_count.patch

Comment 20

[Andrea Marchesini [:baku]]

Attachment: console_5_time.patch

Comment 21

[Andrea Marchesini [:baku]]

Attachment: console_8_tests.patch

Comment 22

[Andrea Marchesini [:baku]]

Attachment: console_8_tests.patch

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8859516 [details] [diff] [review]
console_7_count.patch

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

::: dom/console/Console.cpp
@@ +1375,4 @@
>    }
>  
>    else if (aMethodName == MethodCount) {
>      ConsoleStackEntry frame;

Can this `frame` variable be removed now?

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8859582 [details] [diff] [review]
console_8_tests.patch

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

::: testing/web-platform/meta/console/console-count-label-conversion.any.js.ini
@@ -1,1 @@
> -[console-count-label-conversion.any.html]

There is a corresponding js file that can be removed as well?  console-count-label-conversion.any.js

::: testing/web-platform/meta/console/console-time-label-conversion.any.js.ini
@@ -1,1 @@
> -[console-time-label-conversion.any.worker.html]

There is a corresponding js file that can be removed as well?  console-time-label-conversion.any.js

Comment 25

[Tom Tromey :tromey]

Comment on attachment 8859516 [details] [diff] [review]
console_7_count.patch

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

::: dom/console/Console.cpp
@@ +2187,2 @@
>  
>    aCountLabel = label;

Not really part of this patch, but it seemed fishy to me that aCountLabel would be set on this return path but not on the earlier one.  It seems better to set it at the spot where "label" is assigned.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e609da5f5e89
Update Console to the latest spec - part 1 - WebIDL update, r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/92b431e3e69b
Update Console to the latest spec - part 2 - Console.clear(), r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/71cabfed1013
Update Console to the latest spec - part 3 - Console.groupEnd(), r=bgrins, r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ca88018958
Update Console to the latest spec - part 4 - Console.trace(), r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7766e5354a3
Update Console to the latest spec - part 5 - Console.time(), r=bgrins r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7bd03363f8
Update Console to the latest spec - part 6 - Console.count(), r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d71c8281a83
Update Console to the latest spec - part 7 - test updated, r=bgrins

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/e609da5f5e89
https://hg.mozilla.org/mozilla-central/rev/92b431e3e69b
https://hg.mozilla.org/mozilla-central/rev/71cabfed1013
https://hg.mozilla.org/mozilla-central/rev/73ca88018958
https://hg.mozilla.org/mozilla-central/rev/e7766e5354a3
https://hg.mozilla.org/mozilla-central/rev/ea7bd03363f8
https://hg.mozilla.org/mozilla-central/rev/8d71c8281a83