Make RenderObject TextStream-loggable
Created attachment 395522 [details] Patch
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?
(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).
(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.
oh, cq+. ok then.
Committed r259557: <https://trac.webkit.org/changeset/259557> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395522 [details].
<rdar://problem/61323098>
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.
Will fix in a followup.
Reopen for cleanup.
Created attachment 395587 [details] Patch
https://trac.webkit.org/changeset/259581/webkit
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());