Bug 95885

Created attachment 162311 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Attachment 162311 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 162311 [details] Patch What does this do?

Created attachment 162508 [details] Patch (In reply to comment #4) > (From update of attachment 162311 [details]) > What does this do? This is to turn on Ian's feature from https://bugs.webkit.org/show_bug.cgi?id=94743.

Comment on attachment 162508 [details] Patch If you don't want review, you can just upload with --no-review, which won't set the "R?" flag. You don't need to stick anything in the bug title. :)

Oh sorry, and I guess you removed it from the title. If you want review you can set the R? flag again.

(In reply to comment #5) > Created an attachment (id=162508) [details] > Patch > > (In reply to comment #4) > > (From update of attachment 162311 [details] [details]) > > What does this do? > > This is to turn on Ian's feature from https://bugs.webkit.org/show_bug.cgi?id=94743. That's far from an explanation. What *is* the feature?

Comment on attachment 162508 [details] Patch This still makes no sense. What is it supposed to do?

(In reply to comment #9) > (From update of attachment 162508 [details]) > This still makes no sense. What is it supposed to do? Some more context on composited scrolling. wkb.ug/91117 introduces RenderLayer::usesCompositedScrolling(), which currently returns true if we're overflow scrolling and the element has been styled with '-webkit-overflow-scrolling: touch'. If this is true, we get promoted to a composited layer, the associated RenderLayerBacking gets special scrolling layers and in response to RenderLayer::scrollTo() we no longer have to invalidate everything to get the scrolled content in the right spot, thereby avoiding very expensive repaints. It's unfortunate that we require -webkit-overflow-scrolling: touch, though. It would be nice if RenderLayer::usesCompositedScrolling() returned true if the page would continue to behave correctly. This is what wkb.ug/94743 is intended to do. Unfortunately, when we become a composited layer, we switch from subpixel to grayscale anti-aliasing. We only want to auto-opt into composited scrolling if we don't mind the change in anti-aliasing, so we need a setting to enable/disable this feature.

In that case this should not touch any compositor files (WebLayerTre...), etc. It's purely a WebCore setting that maps to compositor triggers.

Created attachment 162819 [details] Patch (In reply to comment #4) > (From update of attachment 162311 [details]) > What does this do? This is to turn on Ian's feature from https://bugs.webkit.org/show_bug.cgi?id=94743.

Comment on attachment 162819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162819&action=review > Source/Platform/ChangeLog:3 > + [chromium] Add new force composited scrolling setting. Can you make the bug title (and changelog) match the setting you're adding again, ie "Accelerated Overflow Scroll"? > Source/WebKit/chromium/ChangeLog:3 > + [chromium] NOT FOR REVIEW Add new force composited scrolling setting. This needs to be fixed also.

Created attachment 162829 [details] Patch

Comment on attachment 162829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162829&action=review > Source/Platform/ChangeLog:13 > + * chromium/public/WebTransformOperations.h: > + (WebTransformOperations): > + * chromium/public/WebTransformationMatrix.h: > + (WebTransformationMatrix): Are you really patching WebTransformOperations and WebTransformationMatrix? I think you need to regenerate changelogs (or at least delete this entry) > Source/WebCore/ChangeLog:8 > + Add new accelerated overflow scroll setting, as required for 94743. This isn't really describing anything. You should say what this setting does and what it's for (and "for 94743" does not explain anything). > Source/WebCore/page/Settings.h:127 > + void setEnableAcceleratedOverflowScroll(bool); This should follow the other compositor trigger settings and be close to them. For example: setAcceleratedCompositingForFixedPositionEnabled setAcceleratedCompositingForScrollableFramesEnabled etc

This patch does non-chromium code.

Created attachment 162848 [details] Patch

Comment on attachment 162848 [details] Patch Why is this name inconsistent with all of the other compositing triggers?

(In reply to comment #18) > (From update of attachment 162848 [details]) > Why is this name inconsistent with all of the other compositing triggers? In 94743, I've added Settings::acceleratedCompositingForOverflowScrollEnabled to match the other settings. Admittedly, it's still hard to tell from the name of the flag what it actually does, but I'm having a hard time coming up with a name that's consistent with the other flags and is still descriptive enough. automaticallyOptIntoCompositedScrollingEnabled is more descriptive, I think, but not consistent. If you have an idea of a name that could work, please let me know. In any case, perhaps we should continue the discussion regarding the name of the flag on 94743, and Glenn can use whatever name we decide in these plumbing patches?

This setting appears to be a compositor trigger, so let's name it like one. Specifically from what I can tell this does NOT change scrolling behavior or overflow behavior at all. All it does is change which elements get promoted into a compositing layer. That's what compositor triggers are.

(In reply to comment #20) > This setting appears to be a compositor trigger, so let's name it like one. Specifically from what I can tell this does NOT change scrolling behavior or overflow behavior at all. All it does is change which elements get promoted into a compositing layer. That's what compositor triggers are. Right. Specifically, the elements that get promoted are overflow-scrollable divs that can safely (i.e., without breaking stacking order) establish a stacking context. So in light of this, and the names of the other compositor triggers, my thoughts are: acceleratedCompositingForOverflowScrollEnabled, or the more verbose but descriptive accelaretedCompositingForOverflowScrollableDivsThatCanSafelyEstablishStackingContextsEnabled Do either of these sound acceptable?

The first. We can only promote layers that are stacking contexts, so that part is redundant.

Created attachment 163898 [details] Patch

Changed the name to acceleratedCompositingForOverflowScrollEnabled to make this more consistent with other compositor triggers. Please take a look.

Comment on attachment 163898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163898&action=review R=me but you have some more consistency issues to fix. > Source/WebCore/page/Settings.cpp:465 > + m_acceleratedCompositingForOverflowScrollEnabled = enable; put this inline like the other setAcceleratedCompositing... functions in Settings.h > Source/WebCore/page/Settings.h:666 > + bool m_acceleratedCompositingForOverflowScrollEnabled : 1; Move this down next to the other m_acceleratedCompositing.. bits > Source/WebKit/chromium/public/WebSettings.h:140 > + virtual void setAcceleratedCompositingForOverflowScrollEnabled(bool) = 0; Put this next to the other setAccelerated... setters up around lines 6067. Notice how the rest of this file is alphabetical? > Source/WebKit/chromium/src/WebSettingsImpl.cpp:390 > +void WebSettingsImpl::setAcceleratedCompositingForOverflowScrollEnabled( Move the implementation of this to match the header order > Source/WebKit/chromium/src/WebSettingsImpl.h:135 > + virtual void setAcceleratedCompositingForOverflowScrollEnabled(bool); Alphabetize

Created attachment 163922 [details] Patch

Comment on attachment 163922 [details] Patch Clearing flags on attachment: 163922 Committed r128492: <http://trac.webkit.org/changeset/128492>

All reviewed patches have been landed. Closing bug.