Support fullscreen in MediaPlayer.
Created attachment 31894 [details] patch v1. Simple implementation.
Comment on attachment 31894 [details] patch v1. > 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 > +@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> > + > +@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.
http://trac.webkit.org/changeset/45614
Shouldn't have been closed.
Created attachment 33412 [details] Implement video Fullscreen on Mac OS X. ChromeClient gets a new interface enter/exit/supportsFullscreenForNode(). --- 28 files changed, 1110 insertions(+), 13 deletions(-)
Created attachment 33470 [details] Implement video Fullscreen on Mac OS X. ChromeClient gets a new interface enter/exit/supportsFullscreenForNode(). --- 30 files changed, 1588 insertions(+), 13 deletions(-)
Created attachment 33898 [details] Support Fullscreen for video element https://bugs.webkit.org/show_bug.cgi?id=26742 --- 27 files changed, 1464 insertions(+), 13 deletions(-)
Created attachment 33903 [details] Support Fullscreen for video element https://bugs.webkit.org/show_bug.cgi?id=26742 --- 27 files changed, 1464 insertions(+), 13 deletions(-)
Comment on attachment 33903 [details] Support Fullscreen for video element 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()->absoluteBoundingBoxRect()); 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.
(I won't be quoting all the part I agree with, or the obvious one. Thanks for pointing them out.) (In reply to comment #9) > Except even better would be to early return like this: > > if (!renderer()) > return IntRect(); > return > renderer()->view()->frameView()->contentsToScreen(renderer()->absoluteBoundingBoxRect()); > > 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... Well, this shouldn't happen, because the enterFullscreen button has to result from user interaction, which shouldn't happen on detached document. I'll add an assert here. After reading the rest of the review, you seem to highlight a special case where user interaction could happen with no document. I'll address that by adding proper check. > 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? ;) :) I don't think I am allowed to use Obj-C 2.0 here, so instead I am doing as if and marking ivar with // (retain). > 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 do you mean? NSScreen * is an objective C object pointer. The * has to be preceded by a space. WebCore::HTMLMediaElement is a C++ object. I think I am not mistaken here. > 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. Right, after re-thinking release would be fine. Yet some other part of the code has to autorelease, because we may be processing callbacks from that object that may not be protected. (As you have noted below) > - (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 Because the animation that will be running doesn't retain the controller (ie - delegate). So if someone does [controller exitFullscreen]; [controller release]; This is bound to crash. > > 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. I'll recheck what other part of WebKit are doing. > 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. I am not sure about that either. Nibs don't seem to be encouraged in WebKit. Plus, the controls part of this nib are instantiated via WKSI. It can be done, but it felt ok and easier that way. > Wouldn't Marc Piccerelli yell at you for this? ;) > 0 - (void)updateVolume > 281 { > 282 [self willChangeValueForKey:@"volume"]; > 283 [self didChangeValueForKey:@"volume"]; > 284 } This is a bit controversial I know. Yet, having an ivar just to save all ivar, and synchronize them manually feels like a pain, and too much glue. I would suggest to fix the KVO stuff :) Seriously, are you really against such a spread trick? > Aren't there NSDateFormatters for this sort of thing? > 359 static NSString *timeToString(double time) Thanks. > 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. It was originally split in multiple, but was a pain to maintain, and you don't get to see all the stuff that will get used. I was advised not to split it, and it actually make chance are each different file are pretty much doing independent stuff. So there is already a split here IMO. Thanks for the review Eric! Pierre.
Created attachment 33914 [details] Support Fullscreen for video element https://bugs.webkit.org/show_bug.cgi?id=26742 --- 28 files changed, 1484 insertions(+), 14 deletions(-)
Comment on attachment 33914 [details] Support Fullscreen for video element I'm not sure: const IntRect HTMLMediaElement::screenRect() really makes any sense on the dom element. I found the name "screenRect" confusing at first. It's really absoluteRectForRenderer() or something, but that's a more general concept than just HTMLMediaElement, and thus makes it strange that it's on HTMLMediaElement.
Comment on attachment 33914 [details] Support Fullscreen for video element 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.
(In reply to comment #12) > (From update of attachment 33914 [details]) > I'm not sure: > const IntRect HTMLMediaElement::screenRect() > really makes any sense on the dom element. I found the name "screenRect" > confusing at first. It's really absoluteRectForRenderer() or something, but > that's a more general concept than just HTMLMediaElement, and thus makes it > strange that it's on HTMLMediaElement. I am not that happy with this method as well, but I failed to see something sound that would not be more intrusive. I could probably, in the mean time, use absoluteVideoRectForRenderer() on the HTMLVideoElement. Do you prefer me to add absoluteRectForRenderer() on a super class? Pierre.
(In reply to comment #13) > (From update of attachment 33914 [details]) > I don't see any reply about my earlier question about a typedef for > PlatformMedia, but maybe I missed it? I did define a ifdef for it, yet, I missed to propagate it down into the MediaPlayerPrivate. > Why the extra ()? > 55 @interface WebVideoFullscreenController () > <WebVideoFullscreenHUDWindowControllerDelegate> > I didn't know empty category names were even allowed? This is a commonly(?) used way to define local method. > 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. Returning a non nil string. I'll make the comment clearer. > This must be a QT call getQTMovieViewClass(), cause "get" is not part of WebKit > style. Actually, it's how the dynamic linking defines its accessors, there is little I can do here apart using a #define. I don't plan to do it, is this really wanted? > 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. Shadow is nicer when animating the fullscreen window. I'll add a comment. > 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. :) Eh, I don't either :) AFAIK, there is no better way to do this kind of stuff. We really want to be on top of the Menu and Dock here. I could burry that in a constant for now. > Why super, not self here? > 93 - (WebVideoFullscreenWindow *)fullscreenWindow > 94 { > 95 return (WebVideoFullscreenWindow *)[super window]; > 96 } Ok. > I'm surprised this isn't a _movieView method on the class: > 107 QTMovieView *movieView = (QTMovieView *)[[self window] > contentView]; Right. This looks ugly. > > If we had a typedef PlatformMedia you might not need this cast. ;) > 108 [movieView setMovie:(QTMovie *)_mediaElement->platformMedia()]; I'll remove the cast. Note, I now have typedef void * PlatformMedia; This is actually media player and platform specific. For instance if we have a ffmpeg backend on Mac OS this wouldn't be a QTMovie. I should probably have some kind of better abstraction here. But for now, this would seem over-engineered. > 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. This is Obj-C 2.0. I'll have to remove it. > 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. hum. ok thanks :) > 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 } Thanks. > 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? I'll try something. > 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. Oh, I forgot about that. Yes, I checked, and I have to early return. > 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. We are not using Core Animation, but NSAnimation, and AFAIK we really have to do that work ourselves. > 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? Right, there is no QTHUDWindow to reuse. > I'm surprised this needs to be conditional: > if ([_timelineUpdateTimer isValid]) > 110 [_timelineUpdateTimer invalidate]; Right it doesn't. > I'm surprised we'd want to do anything if the window was already visible? > 118 if (![window isVisible]) > 119 [window setAlphaValue:0]; If the window is already visible we want to animate the fadeIn from its current value to 1. If the window is not visible, we want to: 1- set its alpha value to 0 2- show it 3- animate its alpha value to 1 So yes, we don't want to do anything if the window was already visible. > 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; Ok. > Different case here: > 174 return @"No Nib"; // So that -windowDidLoad gets called. > I guess the string contents don't matter then? Right. > > 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. I could do it. But I am not sure if it wouldn't add more useless glue code. At least now, the ugly code is constrained to one function :) > 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) I am by no way an expert of NSFormatters, but I don't think we can do what we want without subclassing. (We want 00:00 but 1:00:00 as well. ) > 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. :) hum :) I could store the ivar on the stack if you prefer. > Extra space: > 5462 HTMLMediaElement* videoElement = static_cast<HTMLMediaElement*> > (node); > > underlying: > 5466 // The backend may just warn us that the underlaying > plaftormMovie() Thanks. > Style: > } > 5483 else Thanks. I'll try the webkit-check-style :) > > Cute: > 30 static const CGFloat slomoFactor = 10.; > But probably not the most readable to all our non-native english speakers. ;) Ok. > 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) I can't find it for CGRect. Do you have the name of the function. Using WebCore's FloatRect here is probably not really appropriate, right? > 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). Right, I'll try to come up with a better name. Thanks for the review! Pierre.
(In reply to comment #15) > > 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. > > hum. ok thanks :) Well, it seems that I can't reuse any of those. I also don't see what I would do with a centerPoint. > > 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); > > 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. :) > > hum :) I could store the ivar on the stack if you prefer. I didn't do it. I don't think it adds anything valuable, as this is common Cocoa practice. It won't also be magically thread safe or anything. I could add it though if style requires it :)
Created attachment 34936 [details] Fullscreen support for the video element. https://bugs.webkit.org/show_bug.cgi?id=26742 --- 28 files changed, 1572 insertions(+), 14 deletions(-)
Comment on attachment 34936 [details] Fullscreen support for the video element. > 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? > +@interface WebVideoFullscreenHUDWindowController () <NSWindowDelegate> > +@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.
(In reply to comment #18) > (From update of attachment 34936 [details]) > > > 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. Ok > > 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. I don't think so because we need to support multiple backend at runtime. Am I mistaken? > > 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: ? Ok. > > +#pragma mark - > > +#pragma mark Exposed Interface > > We don't normally use #pragma marks. Hum. > > > 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. Ok. > > +- (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? This is something I should check as I have no idea. > > > +@interface WebVideoFullscreenHUDWindowController () <NSWindowDelegate> > > +@end > > This looks weird. I think you should name the category 'Private' or something. Ok. > > +- (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? We want to update the time right away. I'll add a comment. > > > + 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? Ok. I'll get rid of bindinds. > > + > > +- (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. ok. > > > 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? There should be no animation. I'll recheck and add a comment. > > > 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. Hum, I thought it would be more understandable with cos. I'll switch to a third order cubic bezier since it's lighter. Thanks Simon! :) Pierre.
Pierre is a committer, so I won't set cq+. He could do it himself, except that the email address I see in http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py is not the one matching this bug. So I expect that if he set commit-queue+ it would be rejected by the commit queue as an unknown committer.
Comment on attachment 34936 [details] Fullscreen support for the video element. Looks like simon asked for mods on landing anyway, so this is cq- regardless.
Created attachment 38990 [details] Fullscreen support for the video element. https://bugs.webkit.org/show_bug.cgi?id=26742 --- 28 files changed, 1620 insertions(+), 14 deletions(-)
Comment on attachment 38990 [details] Fullscreen support for the video element. > diff --git a/WebCore/html/HTMLMediaElement.h b/WebCore/html/HTMLMediaElement.h > + virtual void willRemove(); Would prefer willRemoveFromDocument() if that's what it really means. > + const IntRect screenRect(); Should be IntRect screenRect() const; > diff --git a/WebKit/mac/WebView/WebVideoFullscreenController.mm b/WebKit/mac/WebView/WebVideoFullscreenController.mm > +// We support queuing animation, that means that we'll correctly > +// interrupt the running animation, and queue the next one. "We support queuing animation_s_" > diff --git a/WebKit/mac/WebView/WebVideoFullscreenHUDWindowController.mm b/WebKit/mac/WebView/WebVideoFullscreenHUDWindowController.mm > +@implementation WebVideoFullscreenHUDWindowController > + > +- (id)init > +{ > + NSWindow* window = [[WebVideoFullscreenHUDWindow alloc] initWithContentRect:NSMakeRect(0, 0, windowWidth, windowHeight) styleMask:NSBorderlessWindowMask backing:NSBackingStoreBuffered defer:NO]; > + self = [super initWithWindow:window]; > + [window setDelegate:self]; > + [window release]; > + if (!self) > + return nil; > + [self windowDidLoad]; > + return self; > +} > +- (void)dealloc Missing blank line between the methods. > +- (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]; You've created a retain cycle here (the timer retains its target). That's OK, because you explicitly call -unscheduleTimeUpdate before destroying the window, but I'd like to see a comment that mentions this. > +- (BOOL)playing > +{ > + if (![_delegate mediaElement]) > + return false; Should be 'return NO'. > diff --git a/WebKit/mac/WebView/WebView.mm b/WebKit/mac/WebView/WebView.mm > @@ -2485,7 +2486,7 @@ static bool needsWebViewInitThreadWorkaround() > > if ([self _needsFrameLoadDelegateRetainQuirk]) > [_private->frameLoadDelegate release]; > - > + > [_private release]; > // [super dealloc] can end up dispatching against _private (3466082) > _private = nil; Unwanted whitespace change. > +- (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]; > +} > + > +- (void)_exitFullscreen > +{ > + [_private->fullscreenController exitFullscreen]; > + [_private->fullscreenController release]; > + _private->fullscreenController = nil; > +} What happens if the WebView is removed from the window while in fullscreen mode? Is there something that guarantees that _exitFullscreen will be called in this case? > diff --git a/WebKit/mac/WebView/WebWindowAnimation.h b/WebKit/mac/WebView/WebWindowAnimation.h > +@interface WebWindowFadeAnimation : NSAnimation { > +@private > + CGFloat _initialAlpha, _finalAlpha; We don't normally use comma-separated lists for multiple instance variable declarations. Please put one per line. > diff --git a/WebKit/mac/WebView/WebWindowAnimation.m b/WebKit/mac/WebView/WebWindowAnimation.m > +@implementation WebWindowScaleAnimation > +- (id)init > +{ > + self = [super init]; > + if (!self) > + return nil; > +#ifdef HAVE_WINDOWSETSCALEDFRAME Where does HAVE_WINDOWSETSCALEDFRAME come from? I don't see it defined anywhere. > + [self setAnimationBlockingMode:NSAnimationNonblockingThreaded]; > +#endif > + [self setFrameRate:60.]; 60.0 please. r=me with these issues addressed.
Comment on attachment 38990 [details] Fullscreen support for the video element. Simon has outstanding requested mods for this patch, so marking it cq- to make clear it can't be auto-committed as is.
This patch has been waiting to land for a month. Curious what's the status?
I'm working on landing it. It needs some tweaking.
http://trac.webkit.org/changeset/49136 and the followup build fixes.