Bug 122957 – [CSSRegions] Move region styling code into RenderNamedFlowFragment

• Component: CSS
• Status: RESOLVED FIXED

[Mihnea Ovidenie]

After https://bugs.webkit.org/show_bug.cgi?id=119135, the region styling code should be moved from RenderRegion class to the new RenderNamedFlowFragment class. RenderNamedFlowFragment will contain functionality specific to CSSRegions only, as it is the case for region styling.

[Andrei Bucur]

Created attachment 217295 [details]
Patch

[Mihnea Ovidenie]

Comment on attachment 217295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217295&action=review

Looks good, but i have some comments before i r+ it.

> Source/WebCore/ChangeLog:8
> +        The patch moves all the region styling functionality outside of RenderRegion

You are also moving painting from RenderRegion to RenderNamedFlowFragment and you should mention that in changelog.

> Source/WebCore/ChangeLog:11
> +        when everything CSS Regions specific will be located inside RenderNamedFlowThread

It may be nice to specify what other functionality you have in mind here.

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:289
> +    m_flowThread->paintFlowThreadPortionInRegion(paintInfo, this, flowThreadPortionRect(), flowThreadPortionOverflowRect(), LayoutPoint(paintOffset.x() + borderLeft() + paddingLeft(), paintOffset.y() + borderTop() + paddingTop()));

Do we still need to add borderLeft(), paddingLeft, borderTop and paddingTop? Are they not null for the anonymous regions?

> Source/WebCore/rendering/RenderNamedFlowFragment.h:75
> +// FIXME: Temporarily public until we move all the CSSRegions functionality from RenderRegion to here.

I dislike this but i do not have a better solution. I guess it does not make sense to virtualize the method to avoid the change of accessibility.

> Source/WebCore/rendering/RenderRegion.cpp:298
> +    if (m_flowThread && oldStyle && oldStyle->writingMode() != style().writingMode())

You do not need to check m_flowThread here as you tested already above.

> Source/WebCore/rendering/RenderRegion.cpp:431
> +        toRenderNamedFlowFragment(this)->checkRegionStyle();

It's a pity you had to include a file "RenderNamedFlowFragment.h" just for this check.

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

If you cast it here, it means this function cannot be used in the future for dumping regions for other functionalities that use regions and flow threads.

[Andrei Bucur]

Thanks for the review, patch incorporating feedback on its way.

[Andrei Bucur]

Created attachment 217306 [details]
Patch

[Mihnea Ovidenie]

Comment on attachment 217306 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 217306 [details]
Patch

Clearing flags on attachment: 217306

Committed r159553: <http://trac.webkit.org/changeset/159553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.