Bug 59662 – showLineTree/showLineTreeForThis would make working with the line box tree easier

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

I'm volunteering to get this done since it's bothered me enough. Please add any feature requests.

[Eric Seidel (no email)]

SGTM. :)  We should also fix the existing ones so they always work!  So often they get confused by anonymous nodes, etc.

[Levi Weintraub]

Here's some sample output where "this" is the RenderText that corresponds to the 3 marked InlineTextBoxes.

(gdb) p this->showLineTreeForThis()
RenderBlock 0x106eb4788                	DIV	0x106eb4680
  RootInlineBox	0x106ebecb8            	RenderBlock 0x106eb4788
    InlineTextBox	0x106ec0068          	RenderText 0x106eb4918 (0,30) "A line with just text and a br"
    InlineTextBox	0x106ebed78          	RenderBR 0x106eb4a88   (0,1) "\n"
  RootInlineBox	0x106ebfee8            	RenderBlock 0x106eb4788
*   InlineTextBox	0x106ebfe78          	RenderText 0x106eb4d78 (0,27) "A line with some collapsed "
*   InlineTextBox	0x106ebffa8          	RenderText 0x106eb4d78 (32,48) "spaces and line "
*   InlineTextBox	0x106ebf6b8          	RenderText 0x106eb4d78 (50,56) "breaks"
    InlineTextBox	0x106ebf728          	RenderBR 0x106eb4ee8   (0,1) "\n"
  RootInlineBox	0x106ebf858            	RenderBlock 0x106eb4788
    InlineTextBox	0x106ebf7e8          	RenderText 0x106eb4fd8 (1,13) "Here's some "
    InlineTextBox	0x106ebf918          	RenderText 0x106eb6738 (7,9) "! "
    InlineTextBox	0x106ebf988          	RenderText 0x106eb5e48 (1,5) "RTL "
    InlineTextBox	0x106ebf9f8          	RenderText 0x106eb6358 (0,16) "with nested ltr "
    InlineFlowBox	0x106ebfad8          	RenderInline 0x106eb6528
      InlineTextBox	0x106ebfa68        	RenderText 0x106eb6658 (0,19) "and a bordered span"
    InlineTextBox	0x106ebfb68          	RenderText 0x106eb6738 (0,7) " SHALOM"
    InlineTextBox	0x106ebfbd8          	RenderText 0x106eb6818 (0,1) "."

[Levi Weintraub]

Created attachment 91753 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 91753 [details]
Patch

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

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

Why do we pass non-null pointers to markedBox1 and markedBox2?

[Eric Seidel (no email)]

So fantastic.  I'm so excited about this debugging feature.

[Eric Seidel (no email)]

Comment on attachment 91753 [details]
Patch

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.

[Levi Weintraub]

Comment on attachment 91753 [details]
Patch

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

>> Source/WebCore/rendering/InlineTextBox.cpp:1294
>> +    for (; printedCharacters < 39; printedCharacters++)
> 
> What's with the constants?  Can they have names?

39 is the arbitrary value showRenderTree uses to align corresponding DOM nodes to the right of the displayed RenderObjects. I re-used this for consistency and because I call showRenderObject on the containing RenderBlock. I suppose I could throw it in RenderObject.h or something so it can be re-used all the places it exists.

>> 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.

I'd honestly hate to add a box/label class just for this debugging feature. This mirrors showTreeAndMark from Node.

>> Source/WebCore/rendering/RenderObject.cpp:2668
>> +    if (ro)
> 
> object would be better than ro.

Just mirroring the showTree above. I can rename both if it's worthwhile, but I don't foresee anyone being confused by these functions :p

[Levi Weintraub]

Created attachment 91958 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 91958 [details]
Patch

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

> Source/WebCore/rendering/InlineBox.h:154
> +    virtual const char* boxName() const;

I would have just defined boxName in the header, even though it isn't actually "inline". :)

[Levi Weintraub]

Comment on attachment 91958 [details]
Patch

Clearing flags on attachment: 91958

Committed r85512: <http://trac.webkit.org/changeset/85512>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.

[Levi Weintraub]

(In reply to comment #11)
> All reviewed patches have been landed.  Closing bug.

Thanks for the review!