Bug 128339

Summary: Should get rid of TileController's CoverageForSlowScrolling mode
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, buildbot, commit-queue, esprehn+autocc, glenn, kondapallykalyan, rniwa, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Patch with layout test results
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Patch
none
Patch simon.fraser: review+

Description Beth Dakin 2014-02-06 15:40:02 PST
We should get rid of TileController's CoverageForSlowScrolling. The mode creates large tiles so that we will paint less for these sites. At one time, that was an important performance improvement because a lot of sites were still in slow scrolling mode. Now that very few sites fall into that mode, we should get rid of it since it just adds complexity.
Comment 1 Beth Dakin 2014-02-06 15:49:16 PST
Created attachment 223396 [details]
Patch
Comment 2 Beth Dakin 2014-02-06 16:04:22 PST
Created attachment 223397 [details]
Patch with layout test results
Comment 3 Tim Horton 2014-02-06 16:08:32 PST
Comment on attachment 223397 [details]
Patch with layout test results

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

> LayoutTests/platform/mac-wk2/tiled-drawing/tile-coverage-slow-scrolling-expected.txt:-12
> -      (tile size 800 x 600)

is there any reason to keep this test around?
Comment 4 Beth Dakin 2014-02-06 16:09:22 PST
(In reply to comment #3)
> (From update of attachment 223397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223397&action=review
> 
> > LayoutTests/platform/mac-wk2/tiled-drawing/tile-coverage-slow-scrolling-expected.txt:-12
> > -      (tile size 800 x 600)
> 
> is there any reason to keep this test around?

Not unless we want a test to document this moment. :-)
Comment 5 Simon Fraser (smfr) 2014-02-06 16:10:32 PST
Comment on attachment 223396 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:1291
> +    if (!shouldClipToLayer && repaintContainer == view)
> +        repaintRect = pixelSnappedIntRect(view->backgroundRect(view));

This confuses me. Do we ever have !shouldClipToLayer when |this| != view?
Comment 6 Beth Dakin 2014-02-06 16:18:05 PST
(In reply to comment #5)
> (From update of attachment 223396 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223396&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:1291
> > +    if (!shouldClipToLayer && repaintContainer == view)
> > +        repaintRect = pixelSnappedIntRect(view->backgroundRect(view));
> 
> This confuses me. Do we ever have !shouldClipToLayer when |this| != view?

Hmm, no I don't think so! I was just being extra careful, but I realize now that is not necessary.
Comment 7 Build Bot 2014-02-06 16:37:58 PST
Comment on attachment 223396 [details]
Patch

Attachment 223396 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6517240081940480

New failing tests:
platform/mac-wk2/tiled-drawing/tile-size-slow-zoomed.html
platform/mac-wk2/tiled-drawing/tile-coverage-slow-scrolling.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-non-propagated-body-background.html
Comment 8 Build Bot 2014-02-06 16:37:59 PST
Created attachment 223404 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 9 Beth Dakin 2014-02-07 14:51:16 PST
Created attachment 223501 [details]
Patch
Comment 10 Build Bot 2014-02-07 16:01:52 PST
Comment on attachment 223501 [details]
Patch

Attachment 223501 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4912089470599168

New failing tests:
compositing/repaint/fixed-background-scroll.html
Comment 11 Build Bot 2014-02-07 16:01:55 PST
Created attachment 223508 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 12 Build Bot 2014-02-07 16:24:22 PST
Comment on attachment 223501 [details]
Patch

Attachment 223501 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6079075974119424

New failing tests:
compositing/repaint/fixed-background-scroll.html
Comment 13 Build Bot 2014-02-07 16:24:26 PST
Created attachment 223512 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Beth Dakin 2014-02-07 16:34:41 PST
Created attachment 223515 [details]
Patch

This will fix the latest test failure.
Comment 15 Beth Dakin 2014-02-07 17:54:28 PST
Created attachment 223530 [details]
Patch
Comment 16 Beth Dakin 2014-02-07 18:08:21 PST
http://trac.webkit.org/changeset/163675