[Webkit-unassigned] [Bug 122963] [CSSRegions] Display anonymous regions in DRT

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122963





--- Comment #6 from Mihnea Ovidenie <mihnea at adobe.com>  2013-10-25 04:34:29 PST ---
(In reply to comment #4)
> (From update of attachment 214953 [details])
> 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.
> 

I kept the original name and i added a virtual method - dumpRegion - in RenderRegion and in RenderNamedFlowFragment. The method dumps the region properties in a TextStream.

> > Source/WebCore/rendering/RenderTreeAsText.cpp:667
> > +        RenderNamedFlowFragment* region = toRenderNamedFlowFragment(*itRR);
> 
> There's no need to cast to RenderNamedFlowFragment.
> 

I wanted this cast in order to make sure that these regions are in fact RenderNamedFlowFragments. With the new approach, there is no need for that.

> > Source/WebCore/rendering/RenderTreeAsText.cpp:669
> > +        ts << "RenderNamedFlowFragment";
> 
> We can use the renderer name function instead.
> 

Done.

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

Done, both in RenderRegion::dumpRegion and in RenderNamedFlowFragment::dumpRegion.

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

Right :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.