Update Console to the latest spec

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(URL)

Attachments

(7 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

a month ago
Created attachment 8859261 [details] [diff] [review]
console_1_webidl.patch
Assignee: nobody → amarchesini
Attachment #8859261 - Flags: review?(bgrinstead)
(Assignee)

Comment 2

a month ago
Created attachment 8859262 [details] [diff] [review]
console_2_clear.patch
Attachment #8859262 - Flags: review?(bgrinstead)
(Assignee)

Comment 3

a month ago
Created attachment 8859263 [details] [diff] [review]
console_3_groupEnd.patch
Attachment #8859263 - Flags: review?(bgrinstead)
(Assignee)

Comment 4

a month ago
Created attachment 8859264 [details] [diff] [review]
console_4_trace.patch
Attachment #8859264 - Flags: review?(bgrinstead)
(Assignee)

Comment 5

a month ago
Created attachment 8859265 [details] [diff] [review]
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.
Attachment #8859265 - Flags: review?(bugs)
Attachment #8859265 - Flags: review?(bgrinstead)
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
Attachment #8859261 - Flags: review?(bgrinstead) → review+
Attachment #8859262 - Flags: review?(bgrinstead) → review+

Comment 7

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

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

Comment 9

a month ago
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
Attachment #8859265 - Flags: review?(bugs) → review+
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
Attachment #8859264 - Flags: review?(bgrinstead) → review+
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
Created attachment 8859304 [details] [diff] [review]
console_3_groupEnd-WITH-CHANGES.patch
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
Attachment #8859304 - Flags: review?(nchevobbe)
Attachment #8859263 - Attachment is obsolete: true
Attachment #8859263 - Flags: review?(bgrinstead)
Duplicate of this bug: 1270636
Attachment #8859265 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 15

a month ago
Created attachment 8859308 [details] [diff] [review]
console_6_exception.patch
Attachment #8859308 - Flags: review?(bugs)
(Assignee)

Comment 16

a month ago
Created attachment 8859309 [details] [diff] [review]
console_7_count.patch

This patch must be rebased when bug 1346326 lands.
Attachment #8859309 - Flags: review?(bgrinstead)
(Assignee)

Comment 17

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

Moved to bug 1357503.
Attachment #8859308 - Flags: review?(bugs)
(Assignee)

Updated

a month ago
Attachment #8859308 - Attachment is obsolete: true
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.
Attachment #8859304 - Flags: review?(nchevobbe) → review+
(Assignee)

Comment 19

a month ago
Created attachment 8859516 [details] [diff] [review]
console_7_count.patch
Attachment #8859309 - Attachment is obsolete: true
Attachment #8859309 - Flags: review?(bgrinstead)
Attachment #8859516 - Flags: review?(bgrinstead)
(Assignee)

Comment 20

a month ago
Created attachment 8859539 [details] [diff] [review]
console_5_time.patch
Attachment #8859265 - Attachment is obsolete: true
(Assignee)

Comment 21

a month ago
Created attachment 8859571 [details] [diff] [review]
console_8_tests.patch
Attachment #8859571 - Flags: review?(bgrinstead)
(Assignee)

Comment 22

a month ago
Created attachment 8859582 [details] [diff] [review]
console_8_tests.patch
Attachment #8859571 - Attachment is obsolete: true
Attachment #8859571 - Flags: review?(bgrinstead)
Attachment #8859582 - Flags: review?(bgrinstead)
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?
Attachment #8859516 - Flags: review?(bgrinstead) → review+
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
Attachment #8859582 - Flags: review?(bgrinstead) → review+

Comment 25

a month ago
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.

Updated

a month ago
Duplicate of this bug: 1350419
(Assignee)

Updated

a month ago
Depends on: 1346326

Comment 27

a month ago
Pushed by amarchesini@mozilla.com:
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
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
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.