Bug 64360

Summary: Move useRepaintBounds from RenderBlock::layoutRunsAndFloats to LineLayoutState
Product: WebKit Reporter: Alexandru Chiculita <achicu>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, mihnea, morrita, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
morrita: review-, morrita: commit-queue-
Patch
tony: review+, tony: commit-queue-
Patch none

Description Alexandru Chiculita 2011-07-12 07:32:34 PDT
Fixing the following FIXME from RenderBlockLineLayout.cpp

// FIXME: Should useRepaintBounds be on the LineLayoutState?
// It appears to be used to track the case where we're only repainting a subset of our lines.
Comment 1 Alexandru Chiculita 2011-07-12 07:34:23 PDT
Created attachment 100483 [details]
Patch
Comment 2 Alexandru Chiculita 2011-07-12 12:03:06 PDT
I'm trying to split the RenderBlock::layoutRunsAndFloats into smaller methods. In the end it should make it easier to fix floats bugs that affect CSS Columns. I will add a master bug for it.
Comment 3 Hajime Morrita 2011-07-12 21:37:24 PDT
Comment on attachment 100483 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:853
> +    bool useRepaintBounds() const { return m_useRepaintBounds; }

"uses" or "shouldUse"? (I know original variable name was wrong....)

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1293
> -                    if (!useRepaintBounds)

Could you elaborate why we don't need this?
Comment 4 Alexandru Chiculita 2011-07-12 23:09:09 PDT
Comment on attachment 100483 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:853
>> +    bool useRepaintBounds() const { return m_useRepaintBounds; }
> 
> "uses" or "shouldUse"? (I know original variable name was wrong....)

Ok.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1293
>> -                    if (!useRepaintBounds)
> 
> Could you elaborate why we don't need this?

Every time the updateRepaintRangeFromBox was called there was an if (!useRepaintBounds) useRepaintBounds = true. So I've moved it to updateRepaintRangeFromBox and removed the if clause. It should be faster to just update it every time than to check it and update only if needed.
Comment 5 Alexandru Chiculita 2011-07-13 01:43:14 PDT
Created attachment 100642 [details]
Patch

- Changed useRepaintBounds to usesRepaintBounds.
Comment 6 Tony Chang 2011-07-14 13:47:40 PDT
Comment on attachment 100642 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests needed.

Please specify why no new tests are needed (e.g., because it's just a refactor).
Comment 7 Alexandru Chiculita 2011-07-14 23:15:50 PDT
Created attachment 100935 [details]
Patch

Thanks for review. I've added the reason in the ChangeLog file. I'm not a committer, so I'm uploading the file again for review.
Comment 8 WebKit Review Bot 2011-07-15 00:23:14 PDT
Comment on attachment 100935 [details]
Patch

Clearing flags on attachment: 100935

Committed r91057: <http://trac.webkit.org/changeset/91057>
Comment 9 WebKit Review Bot 2011-07-15 00:23:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Ryosuke Niwa 2011-07-18 14:46:02 PDT
Some Chromium UI tests appear to start failing after this change but it's probably just a flake:
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/5799
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/5800