Bug 95885 – Add new accelerated compositing for overflow scroll setting.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Glenn Hartmann]

This setting is required for 94743

[Glenn Hartmann]

Created attachment 162311 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[James Robinson]

Comment on attachment 162311 [details]
Patch

What does this do?

[Glenn Hartmann]

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.

[Dana Jansens]

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. :)

[Dana Jansens]

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

[James Robinson]

(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?

[James Robinson]

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.

[James Robinson]

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

[Glenn Hartmann]

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.

[Dana Jansens]

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.

[Glenn Hartmann]

Created attachment 162829 [details]
Patch

[James Robinson]

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

[James Robinson]

This patch does non-chromium code.

[Glenn Hartmann]

Created attachment 162848 [details]
Patch

[James Robinson]

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?

[James Robinson]

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?

[James Robinson]

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

[Glenn Hartmann]

Created attachment 163898 [details]
Patch

[Glenn Hartmann]

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

[James Robinson]

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

[Glenn Hartmann]

Created attachment 163922 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163922 [details]
Patch

Clearing flags on attachment: 163922

Committed r128492: <http://trac.webkit.org/changeset/128492>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.