Bug 110430 - Non-painting fixed elements should not cause repaints on scroll
Summary: Non-painting fixed elements should not cause repaints on scroll
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: Xianzhu Wang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-02-20 22:04 PST by Simon Fraser (smfr)
Modified: 2013-03-28 08:46 PDT (History)
9 users (show)

See Also:


Attachments
Testcase (339 bytes, text/html)
2013-02-20 22:04 PST, Simon Fraser (smfr)
no flags Details
For EWS to generate layout test baseline (4.98 KB, patch)
2013-03-26 17:23 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2013-03-26 17:34 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Add out-of-view case (9.00 KB, patch)
2013-03-27 11:50 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
New style repaint tests (8.01 KB, patch)
2013-03-27 13:20 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
For landing (8.15 KB, patch)
2013-03-28 08:25 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2013-02-20 22:04:01 PST
In bug 108112 I made empty position:fixed elements not composited. However, that now makes us repaint those (empty) layers on every scroll.
Comment 1 Simon Fraser (smfr) 2013-02-20 22:04:18 PST
Created attachment 189460 [details]
Testcase
Comment 2 Radar WebKit Bug Importer 2013-02-20 22:04:50 PST
<rdar://problem/13260967>
Comment 3 Xianzhu Wang 2013-03-26 16:09:43 PDT
In bug 113238, sfmr wrote:
Where is NotCompositedForNoVisibleContent actually consulted? I would expect to see a test for it in FrameView::scrollContentsFastPath(), but do not.

wangxianzhu wrote:
Seems that we need the same test in FrameView::scrollContentsFastPath() (as in ScrollingCoordinator)... Will work on this.
Comment 4 Xianzhu Wang 2013-03-26 17:23:09 PDT
Created attachment 195196 [details]
For EWS to generate layout test baseline
Comment 5 Xianzhu Wang 2013-03-26 17:23:45 PDT
Comment on attachment 195196 [details]
For EWS to generate layout test baseline

Not for formal review yet.
Comment 6 Xianzhu Wang 2013-03-26 17:34:28 PDT
Created attachment 195198 [details]
Patch
Comment 7 Xianzhu Wang 2013-03-27 10:21:14 PDT
Comment on attachment 195198 [details]
Patch

Passed most EWS. The expectations seem ok.
Comment 8 Simon Fraser (smfr) 2013-03-27 10:50:50 PDT
Comment on attachment 195198 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1624
> +        if (layer->viewportConstrainedNotCompositedReason() == RenderLayer::NotCompositedForNoVisibleContent) {
> +            // Don't invalidate for fixed layers that don't have visible contents.
> +            continue;

I think we should also skip if the reason is NotCompositedForBoundsOutOfView.
Comment 9 Xianzhu Wang 2013-03-27 11:50:13 PDT
Created attachment 195371 [details]
Add out-of-view case
Comment 10 Simon Fraser (smfr) 2013-03-27 11:53:14 PDT
Comment on attachment 195371 [details]
Add out-of-view case

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

Code changes look good, but the tests should be proper repaint tests.

> LayoutTests/ChangeLog:13
> +        * compositing/repaint/scroll-fixed-layer-no-content.html: Added.
> +        * compositing/repaint/scroll-fixed-layer-no-content-expected.png: Added.
> +        * compositing/repaint/scroll-fixed-layer-no-content-expected.txt: Added.
> +        * compositing/repaint/scroll-fixed-layer-out-of-view.html: Added.
> +        * compositing/repaint/scroll-fixed-layer-out-of-view-expected.png: Added.
> +        * compositing/repaint/scroll-fixed-layer-out-of-view-expected.txt: Added.

These tests should be text tests that dump repaint rects.
Comment 11 Xianzhu Wang 2013-03-27 13:20:45 PDT
Created attachment 195385 [details]
New style repaint tests
Comment 12 Simon Fraser (smfr) 2013-03-27 13:46:44 PDT
Comment on attachment 195385 [details]
New style repaint tests

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

> LayoutTests/compositing/repaint/scroll-fixed-layer-no-content-expected.txt:1
> +

It would be nice if the output looked like: "There should be no repaints here."
Comment 13 Build Bot 2013-03-27 21:08:42 PDT
Comment on attachment 195385 [details]
New style repaint tests

Attachment 195385 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17230761
Comment 14 Xianzhu Wang 2013-03-28 08:25:48 PDT
Created attachment 195571 [details]
For landing
Comment 15 WebKit Review Bot 2013-03-28 08:46:43 PDT
Comment on attachment 195571 [details]
For landing

Clearing flags on attachment: 195571

Committed r147120: <http://trac.webkit.org/changeset/147120>
Comment 16 WebKit Review Bot 2013-03-28 08:46:47 PDT
All reviewed patches have been landed.  Closing bug.