Bug 122957

Summary:

[CSSRegions] Move region styling code into RenderNamedFlowFragment

Product:

WebKit

Reporter:

Mihnea Ovidenie <mihnea>

Component:

CSS

Assignee:

Andrei Bucur <abucur>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abucur, commit-queue, esprehn+autocc, glenn, kondapallykalyan, mibalan, WebkitBugTracker

Priority:

Keywords:

AdobeTracked

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

57312

Attachments:

Description	Flags
Patch	none
Patch	none

Mihnea Ovidenie

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

Attachments
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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Andrei Bucur

Comment 1 2013-11-19 06:50:21 PST

Created attachment 217295 [details] Patch

Mihnea Ovidenie

Comment 2 2013-11-19 08:17:58 PST

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

Comment 3 2013-11-19 09:26:17 PST

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

Andrei Bucur

Comment 4 2013-11-19 09:42:58 PST

Created attachment 217306 [details] Patch

Mihnea Ovidenie

Comment 5 2013-11-19 10:07:53 PST

Comment on attachment 217306 [details] Patch r=me

WebKit Commit Bot

Comment 6 2013-11-20 00:45:12 PST

Comment on attachment 217306 [details] Patch Clearing flags on attachment: 217306 Committed r159553: <http://trac.webkit.org/changeset/159553>

WebKit Commit Bot

Comment 7 2013-11-20 00:45:14 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.