[webkit-reviews] review denied: [Bug 49481] Implement WebKit Full Screen support : [Attachment 73797] WebKit-WebFullScreenController

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

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

> WebKit/mac/WebView/WebFullscreenController.h:2
> + * Copyright (C) 2009 Apple Inc. All rights reserved.

2010

> WebKit/mac/WebView/WebFullscreenController.h:42
> +    RefPtr<WebCore::Element> _element; // (retain)
> +    WebCore::RenderBox* _renderer; // (set)
> +    RefPtr<WebCore::Element> _replacementElement; // (retain)

No need for the //retain comments on C++ things; it's obvious.

> WebKit/mac/WebView/WebFullscreenController.mm:83
> +using WebCore::AnimationList;
> +using WebCore::Animation;
> +using WebCore::DOMWindow;
> +using WebCore::Document;
> +using WebCore::Element;
> +using WebCore::Event;
> +using WebCore::EventListener;
> +using WebCore::EventNames;
> +using WebCore::FloatRect;
> +using WebCore::FloatSize;
> +using WebCore::GraphicsLayer;
> +using WebCore::HTMLMediaElement;
> +using WebCore::HTMLNames::videoTag;
> +using WebCore::Length;
> +using WebCore::LengthType;
> +using WebCore::Node;
> +using WebCore::NodeList;
> +using WebCore::RenderBlock;
> +using WebCore::RenderBox;
> +using WebCore::RenderFullScreen;
> +using WebCore::RenderLayer;
> +using WebCore::RenderLayerBacking;
> +using WebCore::RenderObject;
> +using WebCore::RenderStyle;
> +using WebCore::ScriptExecutionContext;
> +

Why not just "using namespace WebCore"?

> WebKit/mac/WebView/WebFullscreenController.mm:144
> +    [_tickleTimer invalidate];
> +    [_tickleTimer release];
> +    _tickleTimer = nil;

There's a retain cycle between the timer and self. You'll never hit dealloc
with a non-nil timer.

> WebKit/mac/WebView/WebFullscreenController.mm:156
> +#ifdef BUILDING_ON_TIGER
> +    // WebFullscreenController is not supported on Tiger:
> +    ASSERT_NOT_REACHED();

This is odd. Why not assert earlier, or #ifdef all the code out on Tiger?

> WebKit/mac/WebView/WebFullscreenController.mm:210
> +    if (_element) {
> +	   DOMWindow* window = _element->document()->domWindow();
> +	   if (window) {
> +	       window->removeEventListener(eventNames.playEvent, 
_mediaEventListener.get(), true);
> +	       window->removeEventListener(eventNames.pauseEvent,
_mediaEventListener.get(), true);
> +	       window->removeEventListener(eventNames.endedEvent,
_mediaEventListener.get(), true);
> +	   }
> +    }
> +	   
> +    _element = element;
> +    
> +    if (_element) {
> +	   DOMWindow* window = _element->document()->domWindow();
> +	   if (window) {
> +	       window->addEventListener(eventNames.playEvent, 
_mediaEventListener, true);
> +	       window->addEventListener(eventNames.pauseEvent,
_mediaEventListener, true);
> +	       window->addEventListener(eventNames.endedEvent,
_mediaEventListener, true);
> +	   }
> +    }

Can you explain what you're doing here? Why are media events special? What if
the content has registered event listeners on the old window?

> WebKit/mac/WebView/WebFullscreenController.mm:274
> +	   [[[self webView] window] orderOut:self];

Is this hiding the browser window? Is that the right thing to do in all cases
(including non-browser clients)?

> WebKit/mac/WebView/WebFullscreenController.mm:312
> +    [[self window] setFrame:[[[self window] screen] frame] display:YES];

Maybe cache [self window]

> WebKit/mac/WebView/WebFullscreenController.mm:378
> +    // Set up the final style of the FullScreen render block.  Set an
absolute
> +    // width and height equal to the size of the screen, and anchor the
layer
> +    // at the top, left at (0,0).  The RenderFullScreen style is already set

> +    // to position:fixed.
> +    PassRefPtr<RenderStyle> newStyle =
RenderStyle::clone(_renderer->style());
> +    newStyle->setWidth(Length((int)screenFrame.size.width, WebCore::Fixed));

> +    newStyle->setHeight(Length((int)screenFrame.size.height,
WebCore::Fixed));
> +    newStyle->setTop(Length(0, WebCore::Fixed));
> +    newStyle->setLeft(Length(0, WebCore::Fixed));
> +    _renderer->setStyle(newStyle);

It's weird to be running this code in WebKit. You should move it to WebCore
somehow.

> WebKit/mac/WebView/WebFullscreenController.mm:389
> +    // Cause the document to layout, thus calculating a new fullscreen
element size:
> +    [self _document]->updateLayout();
> +    
> +    RenderBox* childRenderer = _renderer->firstChildBox();
> +    CGRect destinationFrame = CGRectMake(childRenderer->x(),
childRenderer->y(), childRenderer->width(), childRenderer->height());
> +    
> +    // Some properties haven't propogated from the GraphicsLayer to the
CALayer yet.  So
> +    // tell the renderer's layer to sync it's compositing state:
> +    GraphicsLayer* rendererGraphics =
_renderer->layer()->backing()->graphicsLayer();
> +    rendererGraphics->syncCompositingState();    

Maybe move this code too. Also don't assume that you have any GraphicsLayers;
accelerated composting might be disabled.

> WebKit/mac/WebView/WebFullscreenController.mm:586
> +    _tickleTimer = [[NSTimer
scheduledTimerWithTimeInterval:tickleTimerInterval target:self
selector:@selector(_tickleTimerFired) userInfo:nil repeats:YES] retain];

This creates a retain cycle between the timer and self.

> WebKit/mac/WebView/WebFullscreenController.mm:651
> +- (BOOL)_isAnyMoviePlaying
> +{
> +    if (!_element)
> +	   return NO;
> +    
> +    Node* nextNode = _element.get();
> +    while (nextNode)
> +    {
> +	   if (nextNode->hasTagName(videoTag)) {
> +	       HTMLMediaElement* element =
static_cast<HTMLMediaElement*>(nextNode);
> +	       if (!element->paused() && !element->ended())
> +		   return YES;
> +	   }
> +	   
> +	   nextNode = nextNode->traverseNextNode(_element.get());
> +    }
> +    
> +    return NO;
> +}
> +

What about movies in subframes?