[webkit-reviews] review denied: [Bug 49481] Implement WebKit Full Screen support : [Attachment 73795] WebCore-Rendering

[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 73795: WebCore-Rendering
https://bugs.webkit.org/attachment.cgi?id=73795&action=review

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

There's too much special magic here, and it's not clear to me how much is just
for the fullscreen animation.

> WebCore/page/FrameView.cpp:495
> +#if ENABLE(FULLSCREEN_API)
> +    Document* document = m_frame->document();
> +    if (document->webkitFullScreen() &&
document->webkitFullScreenRenderer())
> +	  
view->compositor()->updateCompositingLayers(CompositingUpdateAfterLayoutOrStyle
Change, document->webkitFullScreenRenderer()->layer());
> +#endif

I don't think the fullscreen layers should be disconnected at all, so they
shoudn't need updating separately.

> WebCore/page/FrameView.cpp:563
> +    // The fullScreenRenderer's graphicsLayer  has been re-parented, and the
above recursive syncCompositingState
> +    // call will not cause the subtree under it to repaint.	Explicitly call
the syncCompositingState on 
> +    // the fullScreenRenderer's graphicsLayer here:
> +    Document* document = m_frame->document();
> +    if (document->webkitFullScreen() &&
document->webkitFullScreenRenderer()) {
> +	   RenderObject* fullScreenRenderer =
document->webkitFullScreenRenderer();
> +	   if (GraphicsLayer* fullScreenLayer =
toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())
> +	       fullScreenLayer->syncCompositingState();
> +    }

Ditto.

> WebCore/page/FrameView.cpp:2005
> +	   // The fullScreenRenderer's graphicsLayer  has been re-parented, and
the above recursive syncCompositingState
> +	   // call will not cause the subtree under it to repaint.  Explicitly
call the syncCompositingState on 
> +	   // the fullScreenRenderer's graphicsLayer here:
> +	   if (document->webkitFullScreen() &&
document->webkitFullScreenRenderer()) {
> +	       RenderObject* fullScreenRenderer =
document->webkitFullScreenRenderer();
> +	       if (GraphicsLayer* fullScreenLayer =
toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())
> +		   fullScreenLayer->syncCompositingState();
> +	   }

Ditto.

> WebCore/rendering/RenderLayerBacking.cpp:185
> +    // We'd need RenderObject::convertContainerToLocalQuad(), which doesn't
yet exist.  If this
> +    // is a fullscreen renderer, don't clip to the viewport, as the renderer
will be asked to
> +    // display outside of the viewport bounds.
> +    if (compositor()->compositingConsultsOverlap() &&
!layerOrAncestorIsTransformed(m_owningLayer) 
> +#if ENABLE(FULLSCREEN_API)
> +	   && !layerOrAncestorIsFullScreen(m_owningLayer)
> +#endif

Is this only true during animation to fullscreen, or when in fullscreen? Why
isn't the viewport fullscreen when in fullscreen?