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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 31 16:58:27 PDT 2009


Eric Seidel <eric at webkit.org> 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 33903: Support Fullscreen for video element
https://bugs.webkit.org/attachment.cgi?id=33903&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
 1622	  IntRect elementRect(0,0,0,0);

There are style problems with that line, but the default constructor would do
better anyway.

Except even better would be to early return like this:

if (!renderer())
    return IntRect();
return
renderer()->view()->frameView()->contentsToScreen(renderer()->absoluteBoundingB
oxRect());

Seems I could make this crash by getting "enterFullScreen or exitFullscreen()"
called on a detached document.	page() goes through frame() if I remember
correctly, and the document looses its frame when detached...

We could use a typedef for PlatformMovie instead of just void*:
 77	void* platformMovie() const;

const?
 146	 IntRect screenRect();

Same problem with page():
virtual bool supportsFullscreen() const { return document() &&
document()->page()->chrome()->client()->supportsFullscreenForNode(this); }

If I had a click handler and detach the document during my handler but dont'
prevent default, it should crash from the enterFullScreen() call:
525525 void MediaControlFullscreenButtonElement::defaultEventHandler(Event*
event)

Extra spacE?
     BOOL _forceDisableAnimation;
 49 
 50 }

Should any of these be @private?  and what's // (retain) supposed to mean?  (I
thought Obj-C 2.0 ... which maybe we don't use in WebCore... did auto-retaining
or something for you? ;)

Seems we should pick one style or the other for .mm files:
55 - (void)setMediaElement:(WebCore::HTMLMediaElement*)mediaElement;
 56 - (WebCore::HTMLMediaElement*)mediaElement;
 57 
 58 - (void)enterFullscreen:(NSScreen *)screen;

What makes this weak pointer safe?
 - (void)setMediaElement:(WebCore::HTMLMediaElement*)mediaElement;
 104 {
 105	 _mediaElement = mediaElement;

Why autorelease here?  Seems unnecessary to add it to the containing
autorelease pool (or if it is, isn't that a bug?)  seems -release is what you
want.

 - (void)clearFadeAnimation
 123 {
 124	 [_fadeAnimation stopAnimation];
 125	 [_fadeAnimation setWindow:nil];
 126	 [_fadeAnimation autorelease];

Sad that this has to be 2-steps:
     [_backgroundFullscreenWindow release];
 141	 _backgroundFullscreenWindow = nil;

Why do we need to self-retain?
 143	 [self autorelease];  // From in exitFullscreen

Style:
 181	 if (newRatio > originalRatio)
 182	 {
 183	     double newWidth = originalRatio * endFrame.size.height;

I would have used static inline functions to split up large functions like:
 170 - (void)enterFullscreen:(NSScreen *)screen;

 249	 [self retain]; // Balanced in windowDidExitFullscreen
the comment ideally should explain "why"  :)

Most parts of WebKit prefer an early-return style:
 282	 if ((self = [super initWithContentRect:contentRect
styleMask:NSBorderlessWindowMask backing:bufferingType defer:flag])) {

I don't remember if AppKit has strong style to the contrary, but it seems
checking !self would make more sense here.

Why not just use normal syntaxt?
 323	     objc_msgSend([self windowController],
_controllerActionOnAnimationEnd);

Style again:
44     if ((self = [super initWithContentRect:contentRect
styleMask:NSBorderlessWindowMask backing:bufferingType defer:flag]))
 45	{

Style:
7     if (![[self window] isVisible]) {
 118	     [[self window] setAlphaValue:0];
 119	 }

I'm surprised we don't do more of the  184 - (void)windowDidLoad
 wiring in a nib?  Or maybe i'm over-remembering the power of nibs here.

Wouldn't Marc Piccerelli yell at you for this? ;)
0 - (void)updateVolume
 281 {
 282	 [self willChangeValueForKey:@"volume"];
 283	 [self didChangeValueForKey:@"volume"];
 284 }

Aren't there NSDateFormatters for this sort of thing?
 359 static NSString *timeToString(double time)

Ideally this patch would be smaller for easier review.	 I can't promise that
I've actually read it as closely as I really should have, but the above is
enough for at least one more round.


More information about the webkit-reviews mailing list