Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Update Console to the latest spec

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
3 months ago
3 months 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

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

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

3 months 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

3 months 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

3 months 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

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

Comment 16

3 months 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

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

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

Updated

3 months 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

3 months 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

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

Comment 21

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

Comment 22

3 months 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

3 months 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

3 months ago
Duplicate of this bug: 1350419
(Assignee)

Updated

3 months ago
Depends on: 1346326

Comment 27

3 months 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

Comment 28

3 months ago
bugherder
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: 3 months 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.