Bug 64360 – Move useRepaintBounds from RenderBlock::layoutRunsAndFloats to LineLayoutState

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexandru Chiculita]

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.

[Alexandru Chiculita]

Created attachment 100483 [details]
Patch

[Alexandru Chiculita]

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.

[Hajime Morrita]

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?

[Alexandru Chiculita]

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.

[Alexandru Chiculita]

Created attachment 100642 [details]
Patch

- Changed useRepaintBounds to usesRepaintBounds.

[Tony Chang]

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).

[Alexandru Chiculita]

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.

[WebKit Review Bot]

Comment on attachment 100935 [details]
Patch

Clearing flags on attachment: 100935

Committed r91057: <http://trac.webkit.org/changeset/91057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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