Display anonymous regions in DRT

Display anonymous regions in DRT

This patch allows displaying the anonymous region inside a block with flow-from style set.

Before the patch:
  RenderRegion {DIV} at (200,200) size 52x52 [border: (1px solid #000000)]
After the patch:
  RenderBlock (positioned) {DIV} at (200,200) size 52x52 [border: (1px solid #000000)]
    RenderNamedFlowFragment at (1,1) size 50x50

Before the patch:
  Flow Threads
    Thread with flow-name 'article'
      Regions for flow 'article'
      RenderRegion {DIV} #region_1
      RenderRegion {DIV} #region_2

After the patch:
  Named flows
    Named flow 'article'
      Regions for named flow 'article'
        RenderNamedFlowFragment (anonymous child of {DIV} #region_1)
        RenderNamedFlowFragment (anonymous child of {DIV} #region_2)

BUG=321217
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162606

[Julien - ping for review, 2013-11-22 01:23:33]

https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
File Source/core/rendering/RenderTreeAsText.cpp (left):

https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTreeAsText.cpp:591: ts << " invalid";
Why are we removing this information? (it seems useful to know if a region is
invalid for debugging)

https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
File Source/core/rendering/RenderTreeAsText.cpp (right):

https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTreeAsText.cpp:479: // FIXME: temporary hack to
avoid rebase of regions tests
Shouldn't this FIXME be removed? If not, it should be amended to be actionable
as I have no idea what should be done to fix it.

https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTreeAsText.cpp:571: RenderRegion* renderRegion =
*itRR;
Any reason |renderRegion| is non-const?

https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTreeAsText.cpp:574: ts << static_cast<const
RenderObject*>(renderRegion)->renderName();
That's just gross and unneeded, RenderRegion is already a RenderObject.

[mihnea, 2013-11-22 06:28:07]

On 2013/11/22 01:23:33, Julien Chaffraix - AEDT wrote:
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> File Source/core/rendering/RenderTreeAsText.cpp (left):
> 
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> Source/core/rendering/RenderTreeAsText.cpp:591: ts << " invalid";
> Why are we removing this information? (it seems useful to know if a region is
> invalid for debugging)
> 

I decided to display the list of valid and invalid regions separately. It may
make sense in a debug session to see whether the regions in those lists are
really valid or invalid.

>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> File Source/core/rendering/RenderTreeAsText.cpp (right):
> 
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> Source/core/rendering/RenderTreeAsText.cpp:479: // FIXME: temporary hack to
> avoid rebase of regions tests
> Shouldn't this FIXME be removed? If not, it should be amended to be actionable
> as I have no idea what should be done to fix it.
> 

Sorry, missed that.

>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> Source/core/rendering/RenderTreeAsText.cpp:571: RenderRegion* renderRegion =
> *itRR;
> Any reason |renderRegion| is non-const?
> 

It should be const.

>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> Source/core/rendering/RenderTreeAsText.cpp:574: ts << static_cast<const
> RenderObject*>(renderRegion)->renderName();
> That's just gross and unneeded, RenderRegion is already a RenderObject.

Yes, but the renderName method in RenderRegion is private. As an alternative, i
can make the method public in RenderRegion and avoid this ugly cast.

[mihnea, 2013-11-22 07:25:48]

On 2013/11/22 06:28:07, mihnea wrote:
> On 2013/11/22 01:23:33, Julien Chaffraix - AEDT wrote:
> >
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> > File Source/core/rendering/RenderTreeAsText.cpp (left):
> > 
> >
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> > Source/core/rendering/RenderTreeAsText.cpp:591: ts << " invalid";
> > Why are we removing this information? (it seems useful to know if a region
is
> > invalid for debugging)
> > 
> 
> I decided to display the list of valid and invalid regions separately. It may
> make sense in a debug session to see whether the regions in those lists are
> really valid or invalid.
> 
> >
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> > File Source/core/rendering/RenderTreeAsText.cpp (right):
> > 
> >
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> > Source/core/rendering/RenderTreeAsText.cpp:479: // FIXME: temporary hack to
> > avoid rebase of regions tests
> > Shouldn't this FIXME be removed? If not, it should be amended to be
actionable
> > as I have no idea what should be done to fix it.
> > 
> 
> Sorry, missed that.
> 
> >
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> > Source/core/rendering/RenderTreeAsText.cpp:571: RenderRegion* renderRegion =
> > *itRR;
> > Any reason |renderRegion| is non-const?
> > 
> 
> It should be const.
> 
> >
>
https://codereview.chromium.org/79433004/diff/1/Source/core/rendering/RenderT...
> > Source/core/rendering/RenderTreeAsText.cpp:574: ts << static_cast<const
> > RenderObject*>(renderRegion)->renderName();
> > That's just gross and unneeded, RenderRegion is already a RenderObject.
> 
> Yes, but the renderName method in RenderRegion is private. As an alternative,
i
> can make the method public in RenderRegion and avoid this ugly cast.

The last patch takes your comments into account expect for dumping the isValid
flag since the list of valid and invalid regions are displayed separately. I do
not favor any approach in particular, if you feel like we should display the
isValid bit i can do that in this setup in which the list are displayed
separately.

[Julien - ping for review, 2013-11-24 22:44:33]

lgtm

https://codereview.chromium.org/79433004/diff/70001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/79433004/diff/70001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:5705: return "RenderBody"; // FIXME:
Temporary hack until we know that the regression tests pass.
This is still a hack (see r4691 that introduced it) as RenderBlock {body} is
giving you the same information. This patch doesn't change that.

https://codereview.chromium.org/79433004/diff/70001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTreeAsText.cpp (right):

https://codereview.chromium.org/79433004/diff/70001/Source/core/rendering/Ren...
Source/core/rendering/RenderTreeAsText.cpp:479: // FIXME: temporary hack to
avoid rebase of regions tests
Again this FIXME should either be removed or amended to be actionable.

https://codereview.chromium.org/79433004/diff/70001/Source/core/rendering/Ren...
Source/core/rendering/RenderTreeAsText.cpp:644: ts << "Invalid regions for named
flow '" << renderFlowThread->flowThreadName() << "'\n";
Ideally the invalid bit should be dumped by the region itself as it's useful for
debugging. Unfortunately most of the logic was put here.

[commit-bot: I haz the power, 2013-11-25 07:16:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihnea@adobe.com/79433004/150001

[commit-bot: I haz the power, 2013-11-25 08:57:07]

Change committed as 162606