Bug 64360 - Move useRepaintBounds from RenderBlock::layoutRunsAndFloats to LineLayoutState
Summary: Move useRepaintBounds from RenderBlock::layoutRunsAndFloats to LineLayoutState
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-12 07:32 PDT by Alexandru Chiculita
Modified: 2011-07-18 14:46 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.89 KB, patch)
2011-07-12 07:34 PDT, Alexandru Chiculita
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Patch (6.92 KB, patch)
2011-07-13 01:43 PDT, Alexandru Chiculita
tony: review+
tony: commit-queue-
Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2011-07-14 23:15 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

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