[webkit-reviews] review granted: [Bug 135075] Put position:fixed elements into layers when a WK1 view is layer-backed : [Attachment 235153] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 135075: Put position:fixed elements into layers when a WK1 view is
layer-backed
https://bugs.webkit.org/show_bug.cgi?id=135075

Attachment 235153: Patch
https://bugs.webkit.org/attachment.cgi?id=235153&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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().requiresAcceleratedCompositingForViewpo
rtConstrainedPosition())

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().requiresAcceleratedCompositingForViewportConstrainedPos
ition();

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().requiresAcceleratedCompositingForViewportConstrainedPos
ition();
> +    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;