Bug 68638 - [CSSRegions] Make RenderFlowThread track if it has variable width regions
Summary: [CSSRegions] Make RenderFlowThread track if it has variable width regions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-22 10:52 PDT by Dave Hyatt
Modified: 2011-09-22 11:11 PDT (History)
1 user (show)

See Also:


Attachments
Patch (3.86 KB, patch)
2011-09-22 10:54 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2011-09-22 10:55 PDT, Dave Hyatt
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.