Bug 68638

Summary: [CSSRegions] Make RenderFlowThread track if it has variable width regions
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch mitz: review+

Description Dave Hyatt 2011-09-22 10:52:39 PDT
Make RenderFlowThread track if it has variable width regions. This will be relevant for speed as we begin adding code to shrink lines and do custom painting of blocks, etc.
Comment 1 Dave Hyatt 2011-09-22 10:54:15 PDT
Created attachment 108359 [details]
Patch
Comment 2 Dave Hyatt 2011-09-22 10:55:15 PDT
Created attachment 108361 [details]
Patch
Comment 3 mitz 2011-09-22 10:58:31 PDT
Comment on attachment 108361 [details]
Patch

r=me
I think the naming can be improved, “variable widths” seems like the width can change, maybe you can use “nonUniformLogicalWidth” or flip it and call it “regionsHaveUniformLogicalWidth”.
Comment 4 Adam Roben (:aroben) 2011-09-22 10:59:17 PDT
Comment on attachment 108361 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:308
> +        m_hasValidRegions = false;
> +        LayoutUnit previousRegionLogicalWidth = 0;

Seems like you need to set m_regionsHaveVariableLogicalWidth to false here, too. Otherwise once it goes true it will never become false again!

> Source/WebCore/rendering/RenderFlowThread.h:108
> +    bool regionsHaveVariableLogicalWidth() const { return m_regionsHaveVariableLogicalWidth; }

Seems like this isn't needed yet, but maybe you intend to use it in the future?
Comment 5 Dave Hyatt 2011-09-22 11:02:27 PDT
(In reply to comment #4)

> Seems like you need to set m_regionsHaveVariableLogicalWidth to false here, too. Otherwise once it goes true it will never become false again!
> 

Nice catch! Wouldn't be a correctness issue, but would be a performance one!
Comment 6 Dave Hyatt 2011-09-22 11:11:31 PDT
Fixed in r95740.