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
Daniel Bates
2010-01-08 16:23:17 PST
Created attachment 46176 [details]
Patch 1 with test cases
Created attachment 46177 [details]
Patch 2
Add IGNORE_FIXED_BACKGROUNDS flag to the build files and tools.
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. 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.
(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 ? 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. (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. 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! 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.
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.
Attachment 46509 [details] was posted by a committer and has review+, assigning to Daniel Bates for commit.
Comment on attachment 46509 [details] Patch with test cases Clearing flags on attachment: 46509 Committed r53476: <http://trac.webkit.org/changeset/53476> All reviewed patches have been landed. Closing bug. Cherry-picked into qtwebkit-4.6 with commit cd258386aa3564cd4c961933401d0f86a4c872f8 |