Bug 110430

Summary: Non-painting fixed elements should not cause repaints on scroll
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Xianzhu Wang <wangxianzhu>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, eric, esprehn+autocc, lforschler, ojan.autocc, simon.fraser, wangxianzhu, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
For EWS to generate layout test baseline
none
Patch
none
Add out-of-view case
none
New style repaint tests
none
For landing none

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.