[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 #12 from Mihnea Ovidenie <mihnea at adobe.com>  2013-10-25 10:14:14 PST ---
(In reply to comment #11)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 214953 [details] [details] [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.
> I guess an assert would be better to check for things, but I would not put that in dump render tree. I guess the check must live somewhere in RenderNamedFlowThread if it is not there already.
> 
> I still don't get why you need to duplicate the code. I think that the more complicated function can handle both normal RenderRegions and the new one without casting it. You don't access any of the new functions in the new class.
> 

Ok :) i will post a new patch.

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