Bug 59662

Summary: showLineTree/showLineTreeForThis would make working with the line box tree easier
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, hyatt, leviw, mitz, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch none

Description Levi Weintraub 2011-04-27 18:11:56 PDT
I'm volunteering to get this done since it's bothered me enough. Please add any feature requests.
Comment 1 Eric Seidel (no email) 2011-04-27 22:20:41 PDT
SGTM. :)  We should also fix the existing ones so they always work!  So often they get confused by anonymous nodes, etc.
Comment 2 Levi Weintraub 2011-04-29 15:02:04 PDT
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) "."
Comment 3 Levi Weintraub 2011-04-29 15:31:49 PDT
Created attachment 91753 [details]
Patch
Comment 4 Ryosuke Niwa 2011-04-29 15:44:19 PDT
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?
Comment 5 Eric Seidel (no email) 2011-04-29 17:22:07 PDT
So fantastic.  I'm so excited about this debugging feature.
Comment 6 Eric Seidel (no email) 2011-04-29 17:26:48 PDT
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.
Comment 7 Levi Weintraub 2011-05-02 11:59:39 PDT
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
Comment 8 Levi Weintraub 2011-05-02 12:37:27 PDT
Created attachment 91958 [details]
Patch
Comment 9 Eric Seidel (no email) 2011-05-02 13:12:38 PDT
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". :)
Comment 10 Levi Weintraub 2011-05-02 13:23:24 PDT
Comment on attachment 91958 [details]
Patch

Clearing flags on attachment: 91958

Committed r85512: <http://trac.webkit.org/changeset/85512>
Comment 11 Levi Weintraub 2011-05-02 13:23:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Levi Weintraub 2011-05-02 13:24:29 PDT
(In reply to comment #11)
> All reviewed patches have been landed.  Closing bug.

Thanks for the review!