Skip to content

WIP for console.group #749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: webreplay-release
Choose a base branch
from
Open

WIP for console.group #749

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions devtools/server/actors/replay/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,7 @@ if (isRecordingOrReplaying) {
}

function consoleAPIMessageLevel({ level }) {
console.log(`level`, level) // group, groupEnd, ...
switch (level) {
case "trace":
return "trace";
Expand All @@ -770,6 +771,10 @@ function consoleAPIMessageLevel({ level }) {
return "error";
case "assert":
return "assert";
case "group":
return "group";
case "groupEnd":
return "groupEnd";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"groupCollapsed" ? 😁

😮 "table" ???

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call. Here are more log levels from firefox
https://searchfox.org/mozilla-central/source/toolkit/modules/Console.jsm#285-304
https://searchfox.org/mozilla-central/source/devtools/client/webconsole/utils/messages.js#563-583

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had mentioned this in chat, but I'll note here that this is insufficient to capture the semantics of grouping, because we discard console messages sometimes, and grouping explicitly relies on opening and closing messages, so this on its own will easily cause the nesting of group/End messages to become mismatched, so this patch passes along grouping info, but then introduces a separate problem.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE: Nesting:

I think it's probably reasonable to pass the potentially unmatched groups to the frontend and rely on it to do a smart thing (like ignoring group-close entries without group-opens, or auto-closing unclosed groups).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little unclear to me if you are describing a case/scenario that I didn't list above?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, should have been clearer. It seems like the randomness of recordings and of recording region division mean that any particular log statement may end up falling into your "Ambiguous" item.

What I'm trying to illustrate with my examples is that I don't think we can fix the ambiguous cases, and I'm realizing a detail that I should have clarified, which is that the linker itself is what tracks overflow, so when there are overflowing messages, we literally know nothing, even in the backend, about those messages, for

We could consider having the backend always append any group/groupCollapsed entries to the start of a log chunk so that we can avoid scenarios 2 and 3 above, but I don't know how messy that might be on the backend. Also happy to talk about alternate approaches!

We do not have enough information in the backend to know how to do this for the ambiguous case, because we would need to know when groups may have been opened or closed in the overflowed messages.

So either the messages themselves need to know their depth, or the linker itself needs to track message depth for overflowing messages, even if it discards the message itself.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Thank you for distilling that.

So either the messages themselves need to know their depth

I'm not sure what this means. Are you saying that the gecko/chromium/node would have to store this information at runtime/record-time?

I have no idea how much effort that would be. If it's easy to do, then– that would be a great option to have.

I think we might still be skipping past what the desired behavior is for the scenarios I raised above. Like maybe the thread moved forward under the assumption that preserving the indentation was definitely what we wanted to do– but I'm not sure it is.

For instance, what Chrome does when you clear the console feels somewhat analogous to how our focus window feature works? In this case, Chrome blows away the indentation:
Kapture 2022-07-16 at 07 33 19

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, what Chrome does when you clear the console feels somewhat analogous to how our focus window feature works? In this case, Chrome blows away the indentation:

That's fair, I guess the core of what I want is to ensure that overflow doesn't cause us to lose information about group nesting, so I want to make sure that information comes through somewhere, so then the question is what should that information look like to get what we want. In your example above I'd think like:

console.group("group"); // depth 0
// Focus window starts here
console.log("foo1"); // depth 1
console.groupEnd();
console.log("foo2"); // depth 0
console.group("group2");
console.log("foo3"); // depth 1
console.groupEnd();
// Focus window ends here

I think I'd expect that either the frontend would indent foo1 to depth 1, or else it could choose to adjust the depths by recognizing that the first groupEnd didn't have a start, and would basically decide to ignore that groupEnd and adjust the depths of all the messages before the first groupEnd.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The frontend could detect and handle indentation for the scenario you've described– since the presence of a groupEnd implies a group (or groupCollapsed– although this is kind of an interesting distinction TBH). What should the frontend do in this case:

console.groupCollapsed("Header text");
// Focus window starts here
console.log("This log should be hidden by default");
console.groupEnd();
// Focus window ends here

We can infer that there was a group but don't know that it was collapsed– and even if we could there would be nothing to show as the toggle header. (We wouldn't have the "Header text" string.)

default:
return "info";
}
Expand Down