Bug 33408

Summary: Add flag IGNORE_FIXED_BACKGROUNDS (disabled by default) to ignore fixed background images and accelerate web page scrolling on low-powered/mobile devices
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, eric, hausmann, laszlo.gombos, manyoso
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch 1 with test cases
simon.fraser: review-
Patch 2
ddkilzer: review-
Patch with test cases none

Daniel Bates
Reported 2010-01-08 16:23:17 PST
Proposed optimization to ignore fixed background images (i.e. background-attachment: fixed) when a page does not contain any fixed position elements so as to allow fast repaints (via bit blit) when scrolling a page. On low-powered/mobile devices slow repaints can cause noticeable delays when scrolling a page with a fixed background image. Currently, if a page has elements that specify either a fixed background or a fixed position then we perform a slow repaint (i.e disable blitting) so as to avoid rendering artifacts. By sacrificing support for fixed background images when there are no fixed elements on the page and with come care, we don't need to force slow repaints and can avoid rendering artifacts. Hence, on such devices we can improve the responsiveness of scrolling a page with a fixed background image.
Attachments
Patch 1 with test cases (66.81 KB, patch)
2010-01-08 16:35 PST, Daniel Bates 		simon.fraser: review-
DetailsFormatted DiffDiff
Patch 2 (17.94 KB, patch)
2010-01-08 16:36 PST, Daniel Bates 		ddkilzer: review-
DetailsFormatted DiffDiff
Patch with test cases (76.81 KB, patch)
2010-01-13 14:27 PST, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2010-01-08 16:35:13 PST
Created attachment 46176 [details] Patch 1 with test cases
Daniel Bates
Comment 2 2010-01-08 16:36:40 PST
Created attachment 46177 [details] Patch 2 Add IGNORE_FIXED_BACKGROUNDS flag to the build files and tools.
Daniel Bates
Comment 3 2010-01-08 16:39:56 PST
Disabled by default. (In reply to comment #2) > Created an attachment (id=46177) [details] > Patch 2 > > Add IGNORE_FIXED_BACKGROUNDS flag to the build files and tools.
Simon Fraser (smfr)
Comment 4 2010-01-13 11:09:23 PST
Comment on attachment 46176 [details] Patch 1 with test cases This seems like the wrong approach. Why not just change CSSStyleSelector to override FixedBackgroundAttachment with ScrollBackgroundAttachment? Also, having a whole new build setting, IGNORE_FIXED_BACKGROUNDS, for this seems like overkill. What if you find some other CSS property that you'd like to ignore to get fast scrolling? It seems like it should be ALLOW_FAST_SCROLLING or something that indicates that this is for a limited-capacity mobile device.
Adam Treat
Comment 5 2010-01-13 11:47:27 PST
(In reply to comment #4) > (From update of attachment 46176 [details]) > This seems like the wrong approach. Why not just change CSSStyleSelector to > override FixedBackgroundAttachment with ScrollBackgroundAttachment? Because if the page has fixed position elements then the scroll will be slow anyways so we might as well honor the fixed background? > Also, having a whole new build setting, IGNORE_FIXED_BACKGROUNDS, for this > seems like overkill. What if you find some other CSS property that you'd like > to ignore to get fast scrolling? It seems like it should be > ALLOW_FAST_SCROLLING or something that indicates that this is for a > limited-capacity mobile device. Hmm, then it isn't as descriptive to what it actually does. We could certainly do this, but I guess it should have 'MOBILE' in the name or so... MOBILE_FAST_SCROLLING ?
David Kilzer (:ddkilzer)
Comment 6 2010-01-13 11:55:06 PST
Comment on attachment 46177 [details] Patch 2 > Index: WebCore/WebCore.IgnoreFixedBackgrounds.exp > =================================================================== > --- WebCore/WebCore.IgnoreFixedBackgrounds.exp (revision 0) > +++ WebCore/WebCore.IgnoreFixedBackgrounds.exp (revision 0) > @@ -0,0 +1 @@ > +_WebCoreIgnoreFixedBackgrounds I don't see this method (or variable) implemented anywhere in the patch. If this is a platform decision, why not just check in platform-specific results? Why is a build setting needed at all since you're not actually changing any code? It seems like an awful lot of work just to add a flag that is used by run-webkit-tests to check results for a given platform.
David Kilzer (:ddkilzer)
Comment 7 2010-01-13 12:04:04 PST
(In reply to comment #6) > (From update of attachment 46177 [details]) > > Index: WebCore/WebCore.IgnoreFixedBackgrounds.exp > > =================================================================== > > --- WebCore/WebCore.IgnoreFixedBackgrounds.exp (revision 0) > > +++ WebCore/WebCore.IgnoreFixedBackgrounds.exp (revision 0) > > @@ -0,0 +1 @@ > > +_WebCoreIgnoreFixedBackgrounds > > I don't see this method (or variable) implemented anywhere in the patch. Sorry, I though Patch 2 was in response to Patch 1 having its review- flag set instead of a series.
David Kilzer (:ddkilzer)
Comment 8 2010-01-13 12:07:50 PST
Comment on attachment 46177 [details] Patch 2 (In reply to comment #6) > If this is a platform decision, why not just check in platform-specific > results? Since this is a platform-specific, compile-time decision, I think it would be best to simply check in "failing" test results as expected results instead of trying to do something fancy in run-webkit-tests. That way, you still get test coverage for the tests, and you'll know if the results ever change, e.g., if someone accidentally turns the flag back on for your port!
Daniel Bates
Comment 9 2010-01-13 14:27:21 PST
Created attachment 46509 [details] Patch with test cases Based on the suggestions from both Simon Fraser and David Kilzer, I removed the build system- and tools- related changes. Also, included in the patch expected failure results for the Mac build. I renamed IGNORE_FIXED_BACKGROUNDS to FAST_MOBILE_SCROLLING to be more descriptive of what this change does. That is, accelerate scrolling for low-powered/mobile devices.
Adam Treat
Comment 10 2010-01-14 10:57:03 PST
Comment on attachment 46509 [details] Patch with test cases r=me based on modifications suggested from ddkilzer and smfr. please watch buildbot for any problems.
Eric Seidel (no email)
Comment 11 2010-01-14 12:54:26 PST
Attachment 46509 [details] was posted by a committer and has review+, assigning to Daniel Bates for commit.
Daniel Bates
Comment 12 2010-01-19 10:48:41 PST
Comment on attachment 46509 [details] Patch with test cases Clearing flags on attachment: 46509 Committed r53476: <http://trac.webkit.org/changeset/53476>
Daniel Bates
Comment 13 2010-01-19 10:48:49 PST
All reviewed patches have been landed. Closing bug.
Simon Hausmann
Comment 14 2010-03-22 04:46:44 PDT
Cherry-picked into qtwebkit-4.6 with commit cd258386aa3564cd4c961933401d0f86a4c872f8
Note You need to log in before you can comment on or make changes to this bug.