[webkit-reviews] review denied: [Bug 122963] [CSSRegions] Display anonymous regions in DRT : [Attachment 214953] Patch 2

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 122963: [CSSRegions] Display anonymous regions in DRT
https://bugs.webkit.org/show_bug.cgi?id=122963

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

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214953&action=review


Thanks Mihnea. I have a couple comments below.

> Source/WebCore/rendering/RenderTreeAsText.cpp:664
> +static void writeRenderNamedFlowRegionList(const RenderRegionList&
flowThreadRegionList, TextStream& ts, int indent)

I guess that the previous name was better in case we need to reuse the function
for other types of RenderFlowThreads.

> Source/WebCore/rendering/RenderTreeAsText.cpp:667
> +	   RenderNamedFlowFragment* region = toRenderNamedFlowFragment(*itRR);

There's no need to cast to RenderNamedFlowFragment.

> Source/WebCore/rendering/RenderTreeAsText.cpp:669
> +	   ts << "RenderNamedFlowFragment";

We can use the renderer name function instead.

> Source/WebCore/rendering/RenderTreeAsText.cpp:670
> +	   if (region->generatingElement()) {

region->generatingElement() is used a lot of times. Might be better to put it
on the stack.

> Source/WebCore/rendering/RenderTreeAsText.cpp:717
>	   if (!validRegionsList.isEmpty() || !invalidRegionsList.isEmpty()) {

You can remove this "if" statement. The condition is duplicated in the nested
ifs.