[webkit-reviews] review granted: [Bug 26742] Support fullscreen in MediaPlayer. : [Attachment 38990] Fullscreen support for the video element.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 26742: Support fullscreen in MediaPlayer.
https://bugs.webkit.org/show_bug.cgi?id=26742

Attachment 38990: Fullscreen support for the video element.
https://bugs.webkit.org/attachment.cgi?id=38990&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/html/HTMLMediaElement.h
b/WebCore/html/HTMLMediaElement.h

> +    virtual void willRemove();

Would prefer willRemoveFromDocument() if that's what it really means.

> +    const IntRect screenRect();

Should be
       IntRect screenRect() const;

> diff --git a/WebKit/mac/WebView/WebVideoFullscreenController.mm
b/WebKit/mac/WebView/WebVideoFullscreenController.mm

> +// We support queuing animation, that means that we'll correctly
> +// interrupt the running animation, and queue the next one.

"We support queuing animation_s_"

> diff --git a/WebKit/mac/WebView/WebVideoFullscreenHUDWindowController.mm
b/WebKit/mac/WebView/WebVideoFullscreenHUDWindowController.mm

> + at implementation WebVideoFullscreenHUDWindowController
> +
> +- (id)init
> +{
> +    NSWindow* window = [[WebVideoFullscreenHUDWindow alloc]
initWithContentRect:NSMakeRect(0, 0, windowWidth, windowHeight)
styleMask:NSBorderlessWindowMask backing:NSBackingStoreBuffered defer:NO];
> +    self = [super initWithWindow:window];
> +    [window setDelegate:self];
> +    [window release];
> +    if (!self)
> +	   return nil;
> +    [self windowDidLoad];
> +    return self;
> +}
> +- (void)dealloc

Missing blank line between the methods.

> +- (void)scheduleTimeUpdate
> +{
> +    [NSObject cancelPreviousPerformRequestsWithTarget:self
selector:@selector(unscheduleTimeUpdate) object:self];
> +
> +    // First, update right away, then schedule future update
> +    [self updateTime];
> +
> +    _timelineUpdateTimer = [[NSTimer timerWithTimeInterval:0.250 target:self
selector:@selector(updateTime) userInfo:nil repeats:YES] retain];

You've created a retain cycle here (the timer retains its target). That's OK,
because you explicitly call -unscheduleTimeUpdate before destroying the window,
but I'd like to see a comment that mentions this. 

> +- (BOOL)playing
> +{
> +    if (![_delegate mediaElement])
> +	   return false;

Should be 'return NO'.

> diff --git a/WebKit/mac/WebView/WebView.mm b/WebKit/mac/WebView/WebView.mm

> @@ -2485,7 +2486,7 @@ static bool needsWebViewInitThreadWorkaround()
>      
>      if ([self _needsFrameLoadDelegateRetainQuirk])
>	   [_private->frameLoadDelegate release];
> -	   
> +
>      [_private release];
>      // [super dealloc] can end up dispatching against _private (3466082)
>      _private = nil;

Unwanted whitespace change.

> +- (void)_enterFullscreenForNode:(WebCore::Node*)node
> +{
> +    ASSERT(node->hasTagName(WebCore::HTMLNames::videoTag));
> +    HTMLMediaElement* videoElement = static_cast<HTMLMediaElement*>(node);
> +
> +    if (_private->fullscreenController) {
> +	   if ([_private->fullscreenController mediaElement] == videoElement) {

> +	       // The backend may just warn us that the underlaying
plaftormMovie()
> +	       // has changed. Just force an update.
> +	       [_private->fullscreenController setMediaElement:videoElement];
> +	       return; // No more to do.
> +	   }
> +
> +	   // First exit Fullscreen for the old mediaElement.
> +	   [_private->fullscreenController mediaElement]->exitFullscreen();
> +	   // This previous call has to trigger _exitFullscreen,
> +	   // which has to clear _private->fullscreenController.
> +	   ASSERT(!_private->fullscreenController);
> +    }
> +    if (!_private->fullscreenController) {
> +	   _private->fullscreenController = [[WebVideoFullscreenController
alloc] init];
> +	   [_private->fullscreenController setMediaElement:videoElement];
> +	   [_private->fullscreenController enterFullscreen:[[self window]
screen]];	 
> +    }
> +    else
> +	   [_private->fullscreenController setMediaElement:videoElement];
> +}
> +
> +- (void)_exitFullscreen
> +{
> +    [_private->fullscreenController exitFullscreen];
> +    [_private->fullscreenController release];
> +    _private->fullscreenController = nil;
> +}

What happens if the WebView is removed from the window while in fullscreen
mode? Is there something that guarantees that _exitFullscreen will be called in
this case?



> diff --git a/WebKit/mac/WebView/WebWindowAnimation.h
b/WebKit/mac/WebView/WebWindowAnimation.h


> + at interface WebWindowFadeAnimation : NSAnimation {
> + at private
> +    CGFloat _initialAlpha, _finalAlpha;

We don't normally use comma-separated lists for multiple instance variable
declarations. Please put one per line.

> diff --git a/WebKit/mac/WebView/WebWindowAnimation.m
b/WebKit/mac/WebView/WebWindowAnimation.m

> + at implementation WebWindowScaleAnimation
> +- (id)init
> +{
> +    self = [super init];
> +    if (!self)
> +	   return nil;
> +#ifdef HAVE_WINDOWSETSCALEDFRAME

Where does HAVE_WINDOWSETSCALEDFRAME come from? I don't see it defined
anywhere.

> +    [self setAnimationBlockingMode:NSAnimationNonblockingThreaded];
> +#endif
> +    [self setFrameRate:60.];

60.0 please.

r=me with these issues addressed.