[webkit-reviews] review denied: [Bug 49481] Implement WebKit Full Screen support : [Attachment 77135] NewFullScreen-Part-3-Renderering

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 49481: Implement WebKit Full Screen support
https://bugs.webkit.org/show_bug.cgi?id=49481

Attachment 77135: NewFullScreen-Part-3-Renderering
https://bugs.webkit.org/attachment.cgi?id=77135&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77135&action=review

I'd like to see one more rev of this patch.

> WebCore/ChangeLog:16
> +	   The RenderFullScreen is intended to be used by clients of that API
to allow a DOM subtree to
> +	   be rendered outside its original Frame. Because of this, there are a
few areas of the
> +	   rendering code which need to be special cased: RenderFullScreen
layers should not be clipped
> +	   to the viewport, as they will almost always be rendering outside the
viewport area;
> +	   RenderFullScreen graphics layers should not be reparented by the
RenderLayerCompositor, as
> +	   the client will likely want to reparent the platformLayer into their
own fullscreen platform
> +	   window; the FrameView must update the RenderFullScreen graphics
layer tree separately from
> +	   the root layer, as the two trees are disconnected.

Rendering outside the viewport is only true now when animating, correct? If so,
the changelog should be updated to say this.

> WebCore/page/FrameView.cpp:542
> +    if (document->webkitIsFullScreen() &&
document->webkitFullScreenRenderer() &&
document->webkitFullScreenRenderer()->isAnimating())

Maybe you could add a Document::isRunningFullscreenAnimation() method that
encapsulates those tests.

> WebCore/page/FrameView.cpp:646
> +	   if (GraphicsLayer* fullScreenLayer =
toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())

I think you should null-check layer()->backing().

> WebCore/page/FrameView.cpp:2153
> +	       if (GraphicsLayer* fullScreenLayer =
toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())

Ditto.

> WebCore/rendering/RenderLayerBacking.cpp:63
> +#if ENABLE(FULLSCREEN_API)
> +#include "RenderFullScreen.h"
> +#endif
> +

Not sure why this include is necessary.

> WebCore/rendering/RenderLayerBacking.cpp:185
> +	   && !layerOrAncestorIsFullScreen(m_owningLayer)

It's unfortunate that the common case of layerOrAncestorIsFullScreen() walks
the whole tree. Maybe test whether the document has a fullscreen renderer
first?

> WebCore/rendering/RenderLayerCompositor.cpp:819
> +#if ENABLE(FULLSCREEN_API)
> +	   // For the sake of clients of the full screen renderer, don't
reparent
> +	   // the full screen layer out from under them if they're in the
middle of
> +	   // animating.
> +	   if (layer->renderer()->isFullScreen() &&
toRenderFullScreen(layer->renderer())->isAnimating())
> +	       return;
> +#endif

What happens to ensure that layers are correctly updated once the animation is
complete? Do we just rely on the style change? Is there a way we can make this
special-casing unnecessary?