Bug 122959

Summary: [CSSRegions] Move regions auto-size code into RenderNamedFlowFragment
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Andrei Bucur <abucur>
Status: RESOLVED FIXED    
Severity: Normal CC: abucur, commit-queue, esprehn+autocc, glenn, kondapallykalyan, mibalan, WebkitBugTracker
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Mihnea Ovidenie 2013-10-17 05:28:26 PDT
After https://bugs.webkit.org/show_bug.cgi?id=119135, the region auto-size 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 auto-size.
Comment 1 Andrei Bucur 2014-01-08 07:59:23 PST
Created attachment 220633 [details]
Patch
Comment 2 Mihnea Ovidenie 2014-01-08 08:23:47 PST
(In reply to comment #1)
> Created an attachment (id=220633) [details]
> Patch

Would not have been easier to make *hasAutoLogicalHeight* a virtual which returns false in RenderRegion and do the check in RenderNamedFlowFragment? The static implementation in RenderFlowThread calls isRenderNamedFlowFragment() which is already a virtual method.
Comment 3 Andrei Bucur 2014-01-09 02:04:09 PST
Created attachment 220704 [details]
Patch
Comment 4 Mihnea Ovidenie 2014-01-09 02:15:45 PST
Comment on attachment 220704 [details]
Patch

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

r=me, please take a look at the comments before landing.

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:159
> +    ASSERT(newLogicalHeight < LayoutUnit::max() / 2);

I would use RenderFlowThread::maxLogicalHeight() instead of LayoutUnit::max() / 2.

> Source/WebCore/rendering/RenderTreeAsText.cpp:671
> +            if (isRenderNamedFlowFragment && toRenderNamedFlowFragment(renderRegion)->hasAutoLogicalHeight())

Since hasAutoLogicalHeight always returns false for RenderRegion, you can still use the previous condition.
Comment 5 Andrei Bucur 2014-01-09 03:26:22 PST
Created attachment 220709 [details]
Patch
Comment 6 WebKit Commit Bot 2014-01-09 04:02:22 PST
Comment on attachment 220709 [details]
Patch

Clearing flags on attachment: 220709

Committed r161553: <http://trac.webkit.org/changeset/161553>
Comment 7 WebKit Commit Bot 2014-01-09 04:02:25 PST
All reviewed patches have been landed.  Closing bug.