[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