Bug 210035 – Make RenderObject TextStream-loggable

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Make RenderObject TextStream-loggable

[Simon Fraser (smfr)]

Created attachment 395522 [details]
Patch

[alan]

Comment on attachment 395522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395522&action=review

> Source/WebCore/rendering/RenderObject.cpp:1918
> +    builder.append(' ');

this will generate trailing whitespace for anonymous renderer (not really an issue though).

> Source/WebCore/rendering/RenderObject.h:786
> +    virtual String debugDescription() const;

why virtual?

[Simon Fraser (smfr)]

(In reply to zalan from comment #2)

> > Source/WebCore/rendering/RenderObject.h:786
> > +    virtual String debugDescription() const;
> 
> why virtual?

Some renderers might want to dump more stuff (e.g. multicol or something).

[alan]

(In reply to Simon Fraser (smfr) from comment #3)
> (In reply to zalan from comment #2)
> 
> > > Source/WebCore/rendering/RenderObject.h:786
> > > +    virtual String debugDescription() const;
> > 
> > why virtual?
> 
> Some renderers might want to dump more stuff (e.g. multicol or something).
sure, let's make it virtual when we've got overrides.

[alan]

oh, cq+. ok then.

[EWS]

Committed r259557: <https://trac.webkit.org/changeset/259557>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395522 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61323098>

[Darin Adler]

Comment on attachment 395522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395522&action=review

> Source/WebCore/dom/Element.cpp:2654
> +    if (hasID()) {
> +        builder.appendLiteral(" id=\'");
> +        builder.append(getIdAttribute());
> +        builder.append('\'');
> +    }

Can be done more efficiently and more attractively in a single line with StringBuilder's variadic append:

    if (hasID())
        append(" id=\'", getIdAttribute(), '\'');

> Source/WebCore/dom/Element.cpp:2659
> +        const size_t maxNumClassNames = 7;

constexpr is better

> Source/WebCore/dom/Element.cpp:2672
> +        if (addEllipsis)
> +            builder.append("...");

More beautiful with a space before the ellipsis?

> Source/WebCore/dom/Node.cpp:1760
> +    if (isTextNode()) {

Since this is already virtual, could put this in Text::debugDescription.

> Source/WebCore/dom/Node.cpp:1774
> +        builder.append(' ');
> +        builder.append('\"');
> +        builder.append(value);
> +        builder.append('\"');

More efficient in a single line:

    builder.append(" \"", value, '"');

> Source/WebCore/rendering/RenderObject.cpp:1917
> +    builder.append(renderName());
> +    builder.append(" 0x"_s);
> +    builder.append(hex(reinterpret_cast<uintptr_t>(this), Lowercase));

Same thing where this could be done in one line and it’s more efficient that way and reads pretty well.

[Simon Fraser (smfr)]

Will fix in a followup.

[Simon Fraser (smfr)]

Reopen for cleanup.

[Simon Fraser (smfr)]

Created attachment 395587 [details]
Patch

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/259581/webkit

[Darin Adler]

Comment on attachment 395587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395587&action=review

One last comment -- not mandatory to do this, but I want you to know about it.

> Source/WebCore/dom/Node.cpp:1759
> +    builder.append(nodeName(), " 0x"_s, hex(reinterpret_cast<uintptr_t>(this), Lowercase));
>      return builder.toString();

I don’t want to keep you going forever, but this can be written:

    return makeString(nodeName(), " 0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase));

The "_s" isn't needed in the variadic context, butI also don’t think it’s harmful. (Sorry that the rule is so inconsistent.) A StringBuilder isn’t needed if you are only doing a single append, and I believe slightly less efficient than just makeString.

> Source/WebCore/dom/Text.cpp:231
> +    builder.append(" length="_s, value.length());

The "_s" here isn’t helpful, but I don’t think it’s harmful either.

> Source/WebCore/rendering/RenderObject.cpp:1919
>      StringBuilder builder;
>  
> -    builder.append(renderName());
> -    builder.append(" 0x"_s);
> -    builder.append(hex(reinterpret_cast<uintptr_t>(this), Lowercase));
> -    builder.append(' ');
> -
> +    builder.append(renderName(), " 0x"_s, hex(reinterpret_cast<uintptr_t>(this), Lowercase));
>      if (node())
> -        builder.append(node()->debugDescription());
> +        builder.append(' ', node()->debugDescription());
>      
>      return builder.toString();

Since this is for debugging, we don’t have to worry about efficiency much, but I have recently been going out of my way to write these simpler functions with makeString instead of StringBuilder. One of these two ways would work:

    if (!node())
        return makeString(renderName(), " 0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase));
    return makeString(renderName(), " 0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase), ' ', node()->debugDescription());

Or:

    auto separator = node() ? " " : "";
    auto description = node() ? String() : node()->debugDescription();
    return makeString(renderName(), " 0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase), separator, node()->debugDescription());