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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>

> diff --git a/WebCore/html/HTMLMediaElement.cpp
b/WebCore/html/HTMLMediaElement.cpp


> +void HTMLMediaElement::enterFullscreen()
> +{
> +    ASSERT(!m_isFullscreen);
> +    if (document() && document()->page())
> +	  
document()->page()->chrome()->client()->enterFullscreenForNode(this);
> +    m_isFullscreen = true;
> +}

I think this should not enter fullscreen if renderer() is null.

> diff --git a/WebCore/platform/graphics/MediaPlayer.h
b/WebCore/platform/graphics/MediaPlayer.h

>  namespace WebCore {
>  
> +// Structure that will hold every native
> +// types supported by the current media player.
> +// We have to do that has multiple media players
> +// backend can live at runtime.
> +typedef struct PlatformMedia {
> +    QTMovie* qtMovie;
> +} PlatformMedia;

I think this would be better as a union with a type flag.

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

> +- (NSString *)windowNibName
> +{
> +    return @"No nib"; // Dummy string to ensure that -loadWindow will be
called.
> +}

I don't like this hack. Why do you go through the window nib-loading path,
instead of just using -initWithWindow: ?

> +#pragma mark -
> +#pragma mark Exposed Interface

We don't normally use #pragma marks.

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

> +#if ENABLE(VIDEO)
> +
> +#import <wtf/RetainPtr.h>
> +#import <QTKit/QTKit.h>
> +#import "WebVideoFullscreenHUDWindowController.h"
> +#import "WebKitSystemInterface.h"

Corresponding header should come first, and the rest should be alphabetical.

> +- (BOOL)canBecomeKeyWindow
> +{
> +    return NO;
> +}

So the HUD controls can't be operated via the keyboard? That sounds bad. What's
accessibility like for those controls?

> + at interface WebVideoFullscreenHUDWindowController () <NSWindowDelegate>
> + at end

This looks weird. I think you should name the category 'Private' or something.

> +- (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];
> +    [[NSRunLoop currentRunLoop] addTimer:_timelineUpdateTimer
forMode:NSRunLoopCommonModes];
> +    [_timelineUpdateTimer fire];

Why are you firing the timer right away?

> +    CGFloat center = (windowWidth - kButtonSize) / 2;
> +    NSControl *playButton =
createControlWithMediaUIControlType(WKMediaUIControlPlayPauseButton,
NSMakeRect(center, top - kButtonSize, kButtonSize, kButtonSize));
> +    [playButton bind:@"value" toObject:self withKeyPath:@"playing"
options:nil];

Do these binding work on all OSes we have to support?

> +- (void)updateVolume
> +{
> +    [self willChangeValueForKey:@"volume"];
> +    [self didChangeValueForKey:@"volume"];
> +}

This violates the will/did change contract for bindings: 'willChange' should be
called before the volume actually changes. If you can't do it correctly, maybe
don't use bindings?

> +
> +- (void)updateTime
> +{
> +    [self updateVolume];
> +
> +    [self willChangeValueForKey:@"currentQTTime"];
> +    [self didChangeValueForKey:@"currentQTTime"];
> +    [self willChangeValueForKey:@"duration"];
> +    [self didChangeValueForKey:@"duration"];
> +    [self willChangeValueForKey:@"remainingTimeText"];
> +    [self didChangeValueForKey:@"remainingTimeText"];
> +    [self willChangeValueForKey:@"elapsedTimeText"];
> +    [self didChangeValueForKey:@"elapsedTimeText"];
> +}

Ditto.

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

> +#if ENABLE(VIDEO)
> +
> +- (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];
> +}

What if this webview isn't in the window?

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

> +- (float)currentValue
> +{
> +    return 0.5 - 0.5 * cos(M_PI * (1 - [self currentProgress]));
> +}

I thought we talked about using a cubic-bezier for this.