[webkit-reviews] review denied: [Bug 59662] showLineTree/showLineTreeForThis would make working with the line box tree easier : [Attachment 91753] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Levi Weintraub <leviw at chromium.org>'s
request for review:
Bug 59662: showLineTree/showLineTreeForThis would make working with the line
box tree easier
https://bugs.webkit.org/show_bug.cgi?id=59662

Attachment 91753: Patch
https://bugs.webkit.org/attachment.cgi?id=91753&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91753&action=review

> Source/WebCore/rendering/InlineBox.cpp:118
> +    const char* boxName = 0;
> +    if (isRootInlineBox())
> +	   boxName = "RootInlineBox";
> +    else if (isInlineFlowBox())
> +	   boxName = "InlineFlowBox";
> +    else
> +	   boxName = "InlineBox";

The rendering tree has renderName() or something like that.  Seems we should do
similarly for the linebox tree.

> Source/WebCore/rendering/InlineBox.cpp:120
> +    for (; printedCharacters < 39; printedCharacters++)

Names are useful for constants.

> Source/WebCore/rendering/InlineFlowBox.cpp:1439
> +	   box->showLineTreeAndMark(markedBox1, markedLabel1, markedBox2,
markedLabel2, obj, depth+1);

I believe our style is spaces around operators.

> Source/WebCore/rendering/InlineTextBox.cpp:1292
> +    value = value.substring(start(), len());
> +    value.replace('\\', "\\\\");
> +    value.replace('\n', "\\n");

Can we do string manipulation like this in the debugger?

> Source/WebCore/rendering/InlineTextBox.cpp:1294
> +    for (; printedCharacters < 39; printedCharacters++)

What's with the constants?  Can they have names?

> Source/WebCore/rendering/RenderBlock.cpp:6272
> +void RenderBlock::showLineTreeAndMark(const InlineBox* markedBox1, const
char* markedLabel1, const InlineBox* markedBox2, const char* markedLabel2,
const RenderObject* obj) const

Feels like box and label are a class.

> Source/WebCore/rendering/RenderObject.cpp:2668
> +    if (ro)

object would be better than ro.