[webkit-reviews] review denied: [Bug 26742] Support fullscreen in MediaPlayer. : [Attachment 31894] patch v1.

[bugzilla-daemon at webkit.org]

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

Attachment 31894: patch v1.
https://bugs.webkit.org/attachment.cgi?id=31894&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()
> +{
> +    // Convert the element rect to screen coordinates
> +    RefPtr<ClientRectList> rectList = getClientRects();

BTW, this will ignore CSS and SVG transforms in the content.

> +    IntRect elementRect(0,0,0,0);

Rects have a ctor that set the values to 0.

> +    if (rectList->length() > 0) {
> +	   ClientRect* rect = rectList->item(0);
> +	   IntRect intrect = IntRect(rect->left(), rect->top(), rect->width(),
rect->height());
> +	   elementRect = document()->view()->contentsToScreen(intrect);
> +    }
> +    
> +    // If the user has more than one monitor, use the one with the bottom
left corner. 
> +    // FIXME: should we have a way for the caller to specify which screen to
use?
> +    IntRect fullscreenRect =
document()->view()->contentsToScreen(IntRect(0,0,0,0));

I don't like this (see below).

> +    void enterFullscreen();
> +    void exitFullscreen();

Do these need to return bool in case the user agent wants to deny the request
for fullscreen?

I really think we need to expose something through WebKit to allow the hosting
application to make decisions about whether fullscreen video is allowed. Maybe
it's just a preference at first?

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

> +    virtual void enterFullscreen(IntRect, IntRect) {}

Params should be const IntRect&, and be named so I know what they represent.
I'm not a big fan of feeding in the rects; what if the video is transformed,
and we want to do a more cinematic animation to fullscreen?

> +    virtual void exitFullscreen(IntRect) {}

What is the rect?

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

> +
> +    // Fullscreen wants to get out.

Clarify the comment. Who's asking: the media player, or the browser?

> diff --git a/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm
b/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm

> +void MediaPlayerPrivate::enterFullscreen(IntRect elementRect, IntRect
fullscreenRect)
> +{
> +    if (m_fullscreenController)
> +	   return;

Should you return something to indicate that you're already in fullscreen?

The asynchronous nature of this bothers me a bit. Should the MediaPlayer API
expose the asynchronicity?

We also need to make sure that destroying the media element while in
fullscreen, and during both transitions works.

> +    m_fullscreenController.adoptNS([[MediaPlayerQTKitFullscreenController
alloc] init]);
> +    [m_fullscreenController.get() setDelegate:m_objcObserver.get()];
> +    [m_fullscreenController.get() setMovie:m_qtMovie.get()];
> +    NSScreen * fullscreen = nil;
> +    for(NSScreen* screen in [NSScreen screens]) {

Space after the for. Enumeration needs to work on Tiger.

Maybe we should determine which screen to show in here, rather than passing in
the rect from outside?
We may be able to make better decisions, e.g. based on bit depth, size,
primary, or even user-agent supplied preferences,
but should be able to choose the screen based on window position too.

> +	   if (NSContainsRect([screen frame], fullscreenRect)) {
> +	       fullscreen = screen;
> +	       break;
> +	   }
> +    }
> +    if (!fullscreen) {
> +	   // The rect given is in a opaque region

Clarify comment to say the window might be offscreen.

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

> + at interface MediaPlayerQTKitFullscreenController : NSWindowController
> +{
> +    QTMovie* _movie;

Add to comment to say whether this is retained.

> +    id<MediaPlayerQTKitFullscreenControllerDelegate> _delegate;

Ditto.

> diff --git
a/WebCore/platform/graphics/mac/MediaPlayerQTKitFullscreenController.mm
b/WebCore/platform/graphics/mac/MediaPlayerQTKitFullscreenController.mm

Maybe wrap the contents of the file with #if ENABLE(VIDEO)? Same for other new
files.

> +#import "config.h"
> +
> +#import <QTKit/QTKit.h>
> +#import <objc/objc-runtime.h>
> +#include <HIToolbox/HIToolbox.h>
> +

> + at interface MediaPlayerQTKitFullscreenController ()

No need for ()

> +- (NSString*)windowNibName
> +{
> +    return @"No nib";
> +}

Need this?

> +- (void)windowDidLoad
> +{
> +    NSWindow * window = [self window];
> +    QTMovieView* view = [[getQTMovieViewClass() alloc] init];
> +    [window setContentView:view];
> +    [view setControllerVisible:NO];
> +    [view setPreservesAspectRatio:YES];
> +    [view setMovie:[self movie]];
> +    [window setHasShadow:YES];
> +    [view release];
> +}
> +
> +- (MediaPlayerQTKitFullscreenWindow*)fullscreenWindow
> +{
> +    return (MediaPlayerQTKitFullscreenWindow*)[super window];

[self window]

> +#pragma mark -
> +#pragma mark Accessors

Should probably remove these.

> +- (void)setMovie:(QTMovie*)movie
> +{
> +    id oldMovie = _movie;
> +    _movie = [movie retain];
> +    [oldMovie release];
> +}

Should this set the movie on the movie view?

> +- (void)requestExitFullscreen
> +{
> +    [[self delegate] fullscreenExitRequested];
> +}

-request implies that refusal is possible, which is not the case here.

> +- (id)initWithContentRect:(NSRect)contentRect styleMask:(NSUInteger)aStyle
backing:(NSBackingStoreType)bufferingType defer:(BOOL)flag
> +{
> +    (void)aStyle;

There's a macro like UNUSED_PARAM for this.

> +    if ((self = [super initWithContentRect:contentRect
styleMask:NSBorderlessWindowMask backing:bufferingType defer:flag])) {
> +	   [self setOpaque:NO];
> +	   [self setBackgroundColor:[NSColor clearColor]];

Use opaque black?

> +- (void)dealloc
> +{
> +    [_fullscreenAnimation release];

Assert that _fullscreenAnimation is nil.

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

This will probably need to change once we have HUD controls that should be
keyboard navigable.

> +- (void)mouseDown:(NSEvent *)theEvent
> +{
> +    (void)theEvent;

Use UNUSED_PARAM

> +- (void)cancelOperation:(id)sender
> +{
> +    (void)sender;

Use UNUSED_PARAM

> +- (void)animatedResizeDidEnd
> +{
> +    // Call our windowController.
> +    if (_controllerActionOnAnimationEnd)
> +	   objc_msgSend([self windowController],
_controllerActionOnAnimationEnd);

Should this be done via a delegate method, or just a method on the specific
class of the window controller.

> +    [self setFrame:startRect display:NO];
> +    [self makeKeyAndOrderFront:self];
> +
> +    NSMutableDictionary * dict = [[NSMutableDictionary alloc]
initWithCapacity:2];

Capacity 3?

> +    _fullscreenAnimation = [[NSViewAnimation alloc]
initWithViewAnimations:[NSArray arrayWithObject:dict]];

Assert that you dont' have a _fullscreenAnimation already.

> +    [_fullscreenAnimation setDuration: .4 * (([NSEvent modifierFlags] &
NSShiftKeyMask) ? 5. : 1.)];

WebCore style is to say 0.4, 5.0 etc.

> +    [_fullscreenAnimation setFrameRate: 60];

No spaces after colons in Obj-C.

> +- (void)animationDidEnd:(NSAnimation*)animation
> +{
> +    if (animation != _fullscreenAnimation) return;

Return on new line.