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

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

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

> WebCore/dom/Document.cpp:4741
> +    setNeedsStyleRecalc(SyntheticStyleChange);
> +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);
> +    m_fullScreenElement->setNeedsStyleRecalc(SyntheticStyleChange);

You don't need these if you're doing a recalcStyle(Force).

> WebCore/dom/Document.cpp:4746
> +    frame()->view()->setShouldUpdateWhileOffscreen(TRUE);

true, not TRUE

> WebCore/dom/Document.cpp:4772
> +    element->detach();
>      // m_fullScreenElement has already been cleared; recalc the style of 
>      // the passed in element instead.
>      element->setNeedsStyleRecalc(FullStyleChange);
>      if (element != documentElement())
>	   documentElement()->setNeedsStyleRecalc(FullStyleChange);
> +    element->attach();
> +
>      updateStyleIfNeeded();

Why not just do a recalcStyle(Force) here too?

> WebCore/dom/Document.cpp:4778
> +    // Save off the m_fullScreenRenderer to destroy only after the client
has
> +    // been notified that the renderer has changed:
> +    RenderBox* renderer = m_fullScreenRenderer;
> +    webkitSetFullScreenRenderer(0);
> +    renderer->destroy();

Renderers are not refcounted, and are only normally created/destroyed in
attach/detach or style changes. It worries me that you're destroying the
renderer here.

> WebCore/dom/Document.cpp:4780
> +    frame()->view()->setShouldUpdateWhileOffscreen(FALSE);

false, not FALSE

> WebCore/dom/Document.cpp:4788
> +void Document::webkitSetFullScreenRenderer(RenderBox* renderer)
> +{
> +    m_fullScreenRenderer = renderer;
> +   
page()->chrome()->client()->fullScreenRendererChanged(m_fullScreenRenderer);
> +}

Where is the corresponding Document.h change?

This method does not need the webkit prefix.

> WebCore/dom/Node.cpp:1338
> +    if (document()->webkitFullScreen() &&
document()->webkitCurrentFullScreenElement() == this) {

document()->webkitFullScreen() is a bad method name. It should be
isFullscreen(). Only methods exposed via IDL need the webkit prefix.

> WebCore/dom/Node.cpp:1345
> +	   RenderFullScreen* fullscreenRenderer = new
(document()->renderArena()) RenderFullScreen(document());
> +	  
fullscreenRenderer->setStyle(RenderFullScreen::createFullScreenStyle());
> +	   parentRenderer->addChild(fullscreenRenderer, 0);
> +	   parentRenderer = fullscreenRenderer;
> +	   nextRenderer = 0;
> +	   document()->webkitSetFullScreenRenderer(fullscreenRenderer);
> +    }

What prevents this renderer from being destroyed if JS runs in the document,
and the parent's child renderers are re-computed?