1823932 - Improve stderr/stdout output for TextNode

Status: ASSIGNED

(DevTools :: Console, enhancement, P3)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1820804 +++

As noted in https://phabricator.services.mozilla.com/D171473#inline-946057 it would be nice if the stdout/stderr output from console.log and friends was more useful for text nodes.

To fix this we'd need to add a case here

JS::Rooted<JS::Value> v(aCx, aData[i]);
if (v.isObject()) {
  Element* element = nullptr;
  if (NS_SUCCEEDED(UNWRAP_OBJECT(Element, &v, element))) {
    if (i != 0) {
      message.AppendLiteral(" ");
    }
    StringifyElement(element, message);
    continue;
  }
}

That does what we do for Element for Text (ie mozilla::dom::Text). We'd want a separate small helper method that produces a useful string based on the Text reference.

It looks like the devtools console's UI prints #text "<content of text node>", with the string elided if it exceeds a certain limit (I haven't tried to find out what that limit is, and it's possible we'd want a different one on stderr/stdout).

Comment 1

[❌ PTO, back 8/18 ❌ Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

My first question here is, what's the simplest way to test the stderr / stdout behavior of console.log for these types? I've been trying this in the browser toolbox console:

foo = new Text("hello world");
console.log(foo);
console.error(foo);

but based on the description, seems like that's not quite the same thing.

Second question - Looking at the phabricator comment that inspired this set of bugs:

Maybe we could file a bug to have a better architecture to handle different kinds and not have a bug if/elseif block here

I'm curious if we want to attempt to switch from conditionals to a polymorphism-based solution instead? The guidance in this bug's description suggests not, but I figured I'd ask. Right now, looks like Console is where we define StringifyElement, and I imagine it would take some work to switch from conditionals to polymorphism, adding some kind of stringify or toString method onto all these classes.

Assuming we don't want to do some polymorphism-based refactoring, I gather the idea here would be to add a StringifyTextNode helper to Console.cpp that gets the TextNode's data and appends it to the output?

Comment 2

[:Gijs (he/him)]

(In reply to Jared Hirsch [:jhirsch] (he/him) (Needinfo please) from comment #1)

My first question here is, what's the simplest way to test the stderr / stdout behavior of console.log for these types? I've been trying this in the browser toolbox console:

foo = new Text("hello world");
console.log(foo);
console.error(foo);

but based on the description, seems like that's not quite the same thing.

Right, so, that is what I did to test as well - but make sure you check for the result of that in stdout/stderr, not in the browser console - it already looks decent there. :-)

Second question - Looking at the phabricator comment that inspired this set of bugs:

Maybe we could file a bug to have a better architecture to handle different kinds and not have a bug if/elseif block here

I'm curious if we want to attempt to switch from conditionals to a polymorphism-based solution instead? The guidance in this bug's description suggests not, but I figured I'd ask. Right now, looks like Console is where we define StringifyElement, and I imagine it would take some work to switch from conditionals to polymorphism, adding some kind of stringify or toString method onto all these classes.

So to be clear, I'm not married to the approach in comment 0. It seemed the Simplest Thing that Could Possibly Work.

In terms of adding a method to the various classes, I don't know if we can do that without adding weight to all these objects - note that stdout/stderr printing is turned off by default on non-local builds. So it'd be sort of a shame to add memory size to every Node or whatever - though I don't know if adding a method and/or interface for this would do so - I tried a quick web search but my C++-fu is too weak to accurately determine this - I think it depends on whether DOM/webidl methods are virtual and/or whether the inheritance bits would add weight. Folks in #DOM on matrix might know without having to look it up...

Note that a separate interface/mixin/class method would potentially work for TextNode/DocumentFragment/Blob/File (all DOM objects), but not Map/Set (not DOM objects). (That's not a problem, just flagging it won't completely address the ugliness on its own.)

A tweak to comment 0 that wouldn't require a separate interface would be to replace the check for Element with one for just Node, create a generic function that takes a Node, and then check for the NodeType inside the helper function, calling separate helpers for Element/TextNode/DocumentFragment. That still uses polymorphism but doesn't require you to add methods to every subclass immediately, which is probably a good thing in terms of not forcing you to frontload a bunch of work to adjust every single DOM Node subclass (as there's quite a few). So perhaps let's start there and potentially file a follow-up if folks in DOM confirm that moving implementations to the individual subclasses would be a decent longer-term idea?

Assuming we don't want to do some polymorphism-based refactoring, I gather the idea here would be to add a StringifyTextNode helper to Console.cpp that gets the TextNode's data and appends it to the output?

Right, probably with some practical limit on how much text to dump into the output. :-)

Comment 3

[❌ PTO, back 8/18 ❌ Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to :Gijs (he/him) from comment #2)

(In reply to Jared Hirsch [:jhirsch] (he/him) (Needinfo please) from comment #1)

Right, so, that is what I did to test as well - but make sure you check for the result of that in stdout/stderr, not in the browser console - it already looks decent there. :-)

Aha, that's what I was missing--looking in the terminal for the raw stdout / stderr output.

Second question - Looking at the phabricator comment that inspired this set of bugs:

Maybe we could file a bug to have a better architecture to handle different kinds and not have a bug if/elseif block here

I'm curious if we want to attempt to switch from conditionals to a polymorphism-based solution instead? The guidance in this bug's description suggests not, but I figured I'd ask. Right now, looks like Console is where we define StringifyElement, and I imagine it would take some work to switch from conditionals to polymorphism, adding some kind of stringify or toString method onto all these classes.

So to be clear, I'm not married to the approach in comment 0. It seemed the Simplest Thing that Could Possibly Work.

In terms of adding a method to the various classes, I don't know if we can do that without adding weight to all these objects - note that stdout/stderr printing is turned off by default on non-local builds. So it'd be sort of a shame to add memory size to every Node or whatever - though I don't know if adding a method and/or interface for this would do so - I tried a quick web search but my C++-fu is too weak to accurately determine this - I think it depends on whether DOM/webidl methods are virtual and/or whether the inheritance bits would add weight. Folks in #DOM on matrix might know without having to look it up...

haha yes! this is the C++ weirdness I was hoping for...but maybe we can come back to this idea after getting the initial patches landed. I've filed a followup bug for now (bug 1826572).

Note that a separate interface/mixin/class method would potentially work for TextNode/DocumentFragment/Blob/File (all DOM objects), but not Map/Set (not DOM objects). (That's not a problem, just flagging it won't completely address the ugliness on its own.)

A tweak to comment 0 that wouldn't require a separate interface would be to replace the check for Element with one for just Node, create a generic function that takes a Node, and then check for the NodeType inside the helper function, calling separate helpers for Element/TextNode/DocumentFragment. That still uses polymorphism but doesn't require you to add methods to every subclass immediately, which is probably a good thing in terms of not forcing you to frontload a bunch of work to adjust every single DOM Node subclass (as there's quite a few). So perhaps let's start there and potentially file a follow-up if folks in DOM confirm that moving implementations to the individual subclasses would be a decent longer-term idea?

This sounds awesome! I will give it a try.

Assuming we don't want to do some polymorphism-based refactoring, I gather the idea here would be to add a StringifyTextNode helper to Console.cpp that gets the TextNode's data and appends it to the output?

Right, probably with some practical limit on how much text to dump into the output. :-)