[Webkit-unassigned] [Bug 135075] Put position:fixed elements into layers when a WK1 view is layer-backed

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135075


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #235153|review?                     |review+
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2014-07-19 22:47:44 PST ---
(From update of attachment 235153)
View in context: https://bugs.webkit.org/attachment.cgi?id=235153&action=review

Looks like it will work.

> Source/WebCore/css/StyleResolver.cpp:1261
>          || (style.position() == FixedPosition && e && e->document().page() && e->document().page()->settings().fixedPositionCreatesStackingContext())
> +        || (style.position() == FixedPosition && e && e->document().page() && e->document().page()->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition())

Could we add a helper function to make this code easier to read?

    || (style.position() == FixedPosition && shouldSomething(e))

Then the code in the helper function would say:

    if (!element)
        return false;
    Page* page = element->document().page();
    if (!page)
        return false;
    return page->settings().fixedPositionCreatesStackingContext()
        || page->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition();

And we could even put some “why” comment in there. It can be an inline function if needed so the generated code could be the same.

> Source/WebCore/page/ChromeClient.h:430
> +    virtual bool requiresAcceleratedCompositingForViewportConstrainedPosition() const { return false; }

Seems a inconsistent that for Settings we go with ”fixed position” but here we say “viewport constrained position”.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2594
>      const Settings& settings = m_renderView.frameView().frame().settings();
> -    if (!settings.acceleratedCompositingForFixedPositionEnabled())
> +    Page* page = this->page();
> +    bool clientRequiresCompositing = page && page->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition();
> +    if (!(settings.acceleratedCompositingForFixedPositionEnabled() || clientRequiresCompositing))
>          return false;

Sure would be nice if this shared code with StyleResolver.cpp. The code is identical, except for how we get to the Page.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:202
> +    virtual bool requiresAcceleratedCompositingForViewportConstrainedPosition() const override;

I suggest making this private rather than public. Same for most of the other virtual overrides. I don’t think we ever want to call these except polymorphically through the base class, so it’s kind of nice to make them private so we don’t do it by accident. A really minor thing.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1017
> +    if ([documentView isKindOfClass:[WebHTMLView class]])
> +        return documentView.layer;
> +    return false;

I think this would read more logically as an &&:

    return [documentView isKindOfClass:[WebHTMLView class]] && documentView.layer;

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.