[webkit-reviews] review granted: [Bug 49481] Implement WebKit Full Screen support : [Attachment 77137] NewFullScreen-Part-5-WebFullscreenController

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 49481: Implement WebKit Full Screen support
https://bugs.webkit.org/show_bug.cgi?id=49481

Attachment 77137: NewFullScreen-Part-5-WebFullscreenController
https://bugs.webkit.org/attachment.cgi?id=77137&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77137&action=review

> WebKit/mac/ChangeLog:44
> +	   * WebView/WebFullscreenController.h: Added.
> +	   * WebView/WebFullscreenController.mm: Added.
> +	   (-[WebFullscreenController windowDidExitFullscreen:]):  Close the
fullscreen window.
> +	   (-[WebFullscreenController windowDidEnterFullscreen:]): Swap the
webView back into the fullscreen window. 
> +	   (-[WebFullscreenController animationDidStop:finished:]): Call
windowDid{Exit|Enter}FullScreen as appropriate.
> +	   (-[WebFullscreenController applicationDidResignActive:]):
> +	   (-[WebFullscreenController applicationDidChangeScreenParameters:]):
Resize the fullscreen window to match
> +	       the new screen parameters.
> +	   (-[WebFullscreenController enterFullscreen:]):  Set up the animation
that will take the fullscreen element
> +	       from its original screen rect into fullscreen.
> +	   (-[WebFullscreenController exitFullscreen]): Swap the webView back
into its original window.
> +	       Set up the animation that will take the fullscreen element back
into its original screen
> +	       rect.
> +	   (-[WebFullscreenController _updatePowerAssertions]): Now checks
_isAnyMoviePlaying to determine 
> +	       whether to disable screensaver and sleep.
> +	   (-[WebFullscreenController _isAnyMoviePlaying]): Walks through the
sub-tree starting at the fullscreen element
> +	       looking for HTMLVideoElements; returns whether any are found to
be playing.
> +	   (-[WebFullscreenController _animationDuration]): Returns the current
animation duration, affected by control
> +	       and shift keys.
> +	   (-[WebFullscreenWindow canBecomeKeyWindow]): Allow the window to
become key.
> +	   (-[WebFullscreenWindow keyDown:]): Handle the 'Esc' key.
> +	   (-[WebFullscreenWindow cancelOperation:]): Request to exit
fullscreen.
> +	   (-[WebFullscreenWindow rendererLayer]): Convenience accessor.
> +	   (-[WebFullscreenWindow setRendererLayer:]): Ditto.
> +	   (-[WebFullscreenWindow backgroundLayer]): Ditto.
> +	   (-[WebFullscreenWindow animationView]): Ditto.
> +	   (MediaEventListener::MediaEventListener): Implements the
EventListener protocol.
> +	   (MediaEventListener::handleEvent): Tells its delegate to
_updatePowerAssertions.

You should use FullScreen, not Fullscreen, consistently, for the classes,
methods and filenames.

> WebKit/mac/WebView/WebFullscreenController.mm:33
> +#import <IOKit/pwr_mgt/IOPMLib.h>

Why not IOKit/IOKit.h?

> WebKit/mac/WebView/WebFullscreenController.mm:57
> +static const NSString* isFullscreenKey = @"isFullscreen";

Should be NSString * const IsFullScreenKey, or, given its usage,
IsEnteringFullScreenKey

> WebKit/mac/WebView/WebFullscreenController.mm:88
> + at interface WebFullscreenController()

We usually name such a category Private

> WebKit/mac/WebView/WebFullscreenController.mm:105
> +#pragma mark -
> +#pragma mark Initialization

We don't use #pragma mark

> WebKit/mac/WebView/WebFullscreenController.mm:189
> +    if (_element) {
> +	   DOMWindow* window = _element->document()->domWindow();
> +	   if (window) {
> +	       window->removeEventListener(eventNames.playEvent, 
_mediaEventListener.get(), true);
> +	       window->removeEventListener(eventNames.pauseEvent,
_mediaEventListener.get(), true);
> +	       window->removeEventListener(eventNames.endedEvent,
_mediaEventListener.get(), true);
> +	   }
> +    }
> +	   
> +    _element = element;
> +    
> +    if (_element) {
> +	   DOMWindow* window = _element->document()->domWindow();
> +	   if (window) {
> +	       window->addEventListener(eventNames.playEvent, 
_mediaEventListener, true);
> +	       window->addEventListener(eventNames.pauseEvent,
_mediaEventListener, true);
> +	       window->addEventListener(eventNames.endedEvent,
_mediaEventListener, true);
> +	   }
> +    }

Please add some comments about why you need to register this event handler.

> WebKit/mac/WebView/WebFullscreenController.mm:271
> +	   WebFullscreenWindow* window = [self _fullscreenWindow];

The * go on the right for Obj-C objects, so WebFullscreenWindow *window =

> WebKit/mac/WebView/WebFullscreenController.mm:287
> +    BOOL isFullscreenAnimation = [[anim valueForKey:isFullscreenKey]
boolValue];

Variable name is ambiguous. Should be isEnteringFullscreenAnimation

> WebKit/mac/WebView/WebFullscreenController.mm:407
> +    CGRect destinationFrame = CGRectMake(childRenderer->x(),
childRenderer->y(), childRenderer->width(), childRenderer->height());	 

childRenderer->x(), childRenderer->y() are not the correct way to find out
where an element is. X and Y can be relative to some enclosing element (e.g.
something with a transform). Maybe use getBoundingClientRect?

> WebKit/mac/WebView/WebFullscreenController.mm:498
> +    // webView was moved to the fullscreen window.  Check too see

One space after periods please. Typo at "too see".

> WebKit/mac/WebView/WebFullscreenController.mm:502
> +    if (_placeholderView && [_placeholderView window])
> +    {

Brace should be on previous line.

> WebKit/mac/WebView/WebFullscreenController.mm:663
> +#if !defined(BUILDING_ON_TIGER) // IOPMAssertionCreateWithName not defined
on < 10.5
> +- (void)_disableIdleDisplaySleep
> +{
> +    if (_idleDisplaySleepAssertion == kIOPMNullAssertionID) 
> +#if defined(BUILDING_ON_LEOPARD) // IOPMAssertionCreateWithName is not
defined in the 10.5 SDK
> +	   IOPMAssertionCreate(kIOPMAssertionTypeNoDisplaySleep,
kIOPMAssertionLevelOn, &_idleDisplaySleepAssertion);
> +#else // IOPMAssertionCreate is depreciated in > 10.5
> +	   IOPMAssertionCreateWithName(kIOPMAssertionTypeNoDisplaySleep,
kIOPMAssertionLevelOn, CFSTR("WebKit playing a video fullscreen."),
&_idleDisplaySleepAssertion);
> +#endif
> +}
> +
> +- (void)_enableIdleDisplaySleep
> +{
> +    if (_idleDisplaySleepAssertion != kIOPMNullAssertionID) {
> +	   IOPMAssertionRelease(_idleDisplaySleepAssertion);
> +	   _idleDisplaySleepAssertion = kIOPMNullAssertionID;
> +    }
> +}
> +
> +- (void)_disableIdleSystemSleep
> +{
> +    if (_idleSystemSleepAssertion == kIOPMNullAssertionID) 
> +#if defined(BUILDING_ON_LEOPARD) // IOPMAssertionCreateWithName is not
defined in the 10.5 SDK
> +	   IOPMAssertionCreate(kIOPMAssertionTypeNoIdleSleep,
kIOPMAssertionLevelOn, &_idleSystemSleepAssertion);
> +#else // IOPMAssertionCreate is depreciated in > 10.5
> +    IOPMAssertionCreateWithName(kIOPMAssertionTypeNoIdleSleep,
kIOPMAssertionLevelOn, CFSTR("WebKit playing a video fullscreen."),
&_idleSystemSleepAssertion);
> +#endif
> +}
> +
> +- (void)_enableIdleSystemSleep
> +{
> +    if (_idleSystemSleepAssertion != kIOPMNullAssertionID) {
> +	   IOPMAssertionRelease(_idleSystemSleepAssertion);
> +	   _idleSystemSleepAssertion = kIOPMNullAssertionID;
> +    }
> +}
> +

I think it would have been preferable to add this in a separate patch.

> WebKit/mac/WebView/WebFullscreenController.mm:739
> +    return (WebFullscreenWindow *)[super window];

[self window] should do, right?