[webkit-reviews] review denied: [Bug 26742] Support fullscreen in MediaPlayer. : [Attachment 33914] Support Fullscreen for video element

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Pierre d'Herbemont
<pdherbemont at free.fr>'s request for review:
Bug 26742: Support fullscreen in MediaPlayer.
https://bugs.webkit.org/show_bug.cgi?id=26742

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
I don't see any reply about my earlier question about a typedef for
PlatformMedia, but maybe I missed it?

Why the extra ()?
 55 @interface WebVideoFullscreenController ()
<WebVideoFullscreenHUDWindowControllerDelegate>
I didn't know empty category names were even allowed?

Which part is required:
 68	return @"No nib"; // Required to get -loadWindow to be called.
returning exactly "No nib" or returning a non-nil string?  The comment is
unclear.

This must be a QT call getQTMovieViewClass(), cause "get" is not part of WebKit
style.

If it's fullscreen, why would it want a shadow?
 88	[window setHasShadow:YES];
(for during the animation to full screen?)  Seems a comment might be needed
here.

I thought there was some sort of fullscreen constant for levels like this:
 89	[window setLevel:NSPopUpMenuWindowLevel-1];
Someone who actually still works @ Apple would know better than I. :)

Why super, not self here?
93 - (WebVideoFullscreenWindow *)fullscreenWindow
 94 {
 95	return (WebVideoFullscreenWindow *)[super window];
 96 }

I'm surprised this isn't a _movieView method on the class:
 107	     QTMovieView *movieView = (QTMovieView *)[[self window]
contentView];

If we had a typedef PlatformMedia you might not need this cast. ;)
 108	     [movieView setMovie:(QTMovie *)_mediaElement->platformMedia()];

No setDelegate call?
 153	 _hudController.delegate = self;

Maybe Obj-C magically turns that into a setDelegate call... I'm kinda surprised
that compiles even, since _hudController is clearly a pointer.

Seems:
constrainFrameToRatioOfFrame
really wants a "centerPoint" call on a rect, so that you don't have to compute
it yourself.  Also sad that this is now at least the 3rd implementation of
preserveAspectRatio logic (in some form) in WebCore.

I would have put this whole block in a separate function (but I tend to be very
static-inline heavy).
 // Create a black window if needed
 198	 if (!_backgroundFullscreenWindow) {
 
Style:
 else {
 206	     [_backgroundFullscreenWindow setFrame:[screen frame] display:NO];
 207	 }

Sad that this block:
242	if (!_forceDisableAnimation && [[self fullscreenWindow] isKeyWindow])
 243	     _fadeAnimation = _backgroundFullscreenWindow ?
[[WebWindowFadeAnimation alloc] initWithDuration:0.2
window:_backgroundFullscreenWindow initialAlpha:initialAlpha finalAlpha:0] :
nil;
 244	 else {
 245	     // This will disable animation
 246	     endFrame = NSZeroRect;
 247	 }
 248 
 249	 [[self fullscreenWindow] animateFromRect:[[self window] frame]
toRect:endFrame withSubAnimation:_fadeAnimation
controllerAction:@selector(windowDidExitFullscreen)];

is basically repeated twice.  Once to animate in, and once to animate out. 
Maybe it could be abstracted into a function instead?

Did you learn if early-return was "sanctioned" here?
if ((self = [super initWithContentRect:contentRect
styleMask:NSBorderlessWindowMask backing:bufferingType defer:flag])) {
It seems like it would be cleaner.

I'm surprised CoreAnimation doesn't take care of more of the "cancel existing
animations an start animating in the other direction" code for you?  I thought
that was part of the CA magic?
 336 - (void)animateFromRect:(NSRect)startRect toRect:(NSRect)endRect
withSubAnimation:(NSAnimation *)subAnimation
controllerAction:(SEL)controllerAction
Seems to be almost entirely about handling the "what if we're already
animating" case.

I'm slightly suprised there no QTHudWindow to reuse here instead of writing one
for WebKit.  I guess the QT ones are either private or not quite the right fit?


I'm surprised this needs to be conditional:
     if ([_timelineUpdateTimer isValid])
 110	     [_timelineUpdateTimer invalidate];

I'm surprised we'd want to do anything if the window was already visible?
118	if (![window isVisible])
 119	     [window setAlphaValue:0];

I would think a few simple accessors would take care of these nil clearings
better than doing it manually at every callsite:
     [_area release];;
 144	 _area = nil;

Different case here:
 174	 return @"No Nib"; // So that -windowDidLoad gets called.
I guess the string contents don't matter then?

I would have probably created each of the UI elements in a separate constructor
function, mostly because windowDidLoad is a huge function at this point.  But
I'm not sure that it maters.  I hate how ugly UI code often is.

Seems strange to ask the delegate twice:
336	if (![_delegate mediaElement])
 337	     return;
 338	 WebCore::ExceptionCode e;
 339	 [_delegate mediaElement]->setVolume(volume / [self maxVolume], e);

NSDateFormatters no right here?
 361 static NSString *timeToString(double time)

Still seems strange to ask the delegate more than once for mediaElement.  Not
that it would change it's mind between calls... but it always scares me when
asking delgates things. :)

Extra space:
 5462	  HTMLMediaElement* videoElement = static_cast<HTMLMediaElement*>
(node);

underlying:
 5466		  // The backend may just warn us that the underlaying
plaftormMovie()

Style:
     }
 5483	  else

Cute:
 30 static const CGFloat slomoFactor = 10.;
But probably not the most readable to all our non-native english speakers. ;)

I'm surprised NSRect doesn't already do this?  CGRect and WebCore's FloatRect
certainly do:
 37 static NSRect scaledRect(NSRect _initialFrame, NSRect _finalFrame, double
factor)

Sigh.  Again FloatRect would do this cleaner:
     dist = squaredDistance(NSMakePoint(NSMaxX(_initialFrame),
NSMinY(_initialFrame)), NSMakePoint(NSMaxX(_finalFrame), NSMinY(_finalFrame)));

Sad that NSRect doesn't have an easy topLeft() accessor...

What does additionalDuration actually do?  Seems like it's the duration needed
to cover some specific distance... just not sure what that distance is named...
(aka, I think the function needs a better name).

In general I think the change looks fine.  You probably want some current Apple
UI Engineer to look at this instead of me though. :)

r-, mostly for unanswered questions from the previous review.