Bug 122957 - [CSSRegions] Move region styling code into RenderNamedFlowFragment
Summary: [CSSRegions] Move region styling code into RenderNamedFlowFragment
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrei Bucur
Keywords: AdobeTracked
Depends on:
Blocks: 57312
  Show dependency treegraph
Reported: 2013-10-17 05:25 PDT by Mihnea Ovidenie
Modified: 2013-11-20 00:45 PST (History)
7 users (show)

See Also:

Patch (39.20 KB, patch)
2013-11-19 06:50 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (39.92 KB, patch)
2013-11-19 09:42 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2013-10-17 05:25:54 PDT
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.
Comment 1 Andrei Bucur 2013-11-19 06:50:21 PST
Created attachment 217295 [details]
Comment 2 Mihnea Ovidenie 2013-11-19 08:17:58 PST
Comment on attachment 217295 [details]

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.
Comment 3 Andrei Bucur 2013-11-19 09:26:17 PST
Thanks for the review, patch incorporating feedback on its way.
Comment 4 Andrei Bucur 2013-11-19 09:42:58 PST
Created attachment 217306 [details]
Comment 5 Mihnea Ovidenie 2013-11-19 10:07:53 PST
Comment on attachment 217306 [details]

Comment 6 WebKit Commit Bot 2013-11-20 00:45:12 PST
Comment on attachment 217306 [details]

Clearing flags on attachment: 217306

Committed r159553: <http://trac.webkit.org/changeset/159553>
Comment 7 WebKit Commit Bot 2013-11-20 00:45:14 PST
All reviewed patches have been landed.  Closing bug.