[webkit-reviews] review denied: [Bug 43099] Add JavaScript API to allow a page to go fullscreen : [Attachment 62785] FullScreen-WebKit

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 43099: Add JavaScript API to allow a page to go fullscreen
https://bugs.webkit.org/show_bug.cgi?id=43099

Attachment 62785: FullScreen-WebKit
https://bugs.webkit.org/attachment.cgi?id=62785&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebKit/mac/Configurations/FeatureDefines.xcconfig
> ===================================================================

You should probably land the xconfig changes for all projects at the same time.


> Index: WebKit/mac/WebCoreSupport/WebChromeClient.mm
> ===================================================================

> -#if ENABLE(VIDEO)
> +#if ENABLE(FULLSCREEN)

These (and other) changes regress apps that get video fullscreen now, but won't
have generic fullscreen enabled unless they set the preference.

I think we should keep video fullscreen the way it works currently, and only
switch over to generic fullscreen when we're ready to do so with zero
regressions.

> +#if ENABLE(FULLSCREEN)
> + at implementation WebKitFullScreenListener
> +- (id)initWithElement:(Element*)element
> +{
> +    if (!(self = [super init]))
> +	   return nil;
> +    
> +    _element = element;
> +    return self;
> +}
> +
> +- (void)webkitWillEnterFullScreen
> +{
> +    if (_element)
> +	  
_element->document()->webkitWillEnterFullScreenForElement(_element.get());
> +}
> +
> +- (void)webkitDidEnterFullScreen
> +{
> +    if (_element)
> +	  
_element->document()->webkitDidEnterFullScreenForElement(_element.get());
> +}
> +
> +- (void)webkitWillExitFullScreen
> +{
> +    if (_element)
> +	  
_element->document()->webkitWillExitFullScreenForElement(_element.get());
> +}
> +
> +- (void)webkitDidExitFullScreen
> +{
> +    if (_element)
> +	  
_element->document()->webkitDidExitFullScreenForElement(_element.get());
> +}
> +
> + at end

I'm not sure I see the benefit of having this object that just turns around and
calls back into the document. Why not make the WebView the UI delegate?

Also fullscreen/fullScreen inconsistency here.


> +#if ENABLE(FULLSCREEN)
> +- (void)setFullScreenEnabled:(BOOL)flag
> +{
> +    [self _setBoolValue:flag forKey:WebKitFullScreenEnabledPreferenceKey];
> +}
> +
> +- (BOOL)fullScreenEnabled
> +{
> +    return [self _boolValueForKey:WebKitFullScreenEnabledPreferenceKey];
> +}
> +#endif

Fullscreen/fullScreen inconsistency here (and in the pref name).

> Index: WebKit/mac/WebView/WebUIDelegatePrivate.h
> ===================================================================
>  
> +#if ENABLE(FULLSCREEN)
> + at protocol WebKitFullScreenListener<NSObject>
> +- (void)webkitWillEnterFullScreen;
> +- (void)webkitDidEnterFullScreen;
> +- (void)webkitWillExitFullScreen;
> +- (void)webkitDidExitFullScreen;
> + at end

Fullscreen/fullScreen inconsistency.

> +#if ENABLE(FULLSCREEN)
> +- (BOOL)webView:(WebView *)sender supportsFullscreenForElement:(DOMElement
*)element;
> +- (void)webView:(WebView *)sender enterFullscreenForElement:(DOMElement
*)element;
> +- (void)webView:(WebView *)sender exitFullscreenForElement:(DOMElement
*)element;
> +#endif

Fullscreen/fullScreen inconsistency.

>  @end
> Index: WebKit/mac/WebView/WebView.mm
> ===================================================================
> --- WebKit/mac/WebView/WebView.mm	(revision 64185)
> +++ WebKit/mac/WebView/WebView.mm	(working copy)
> @@ -1434,6 +1434,9 @@ - (void)_preferencesChangedNotification:
>      settings->setHTML5ParserEnabled([preferences html5ParserEnabled]);
>      settings->setHTML5TreeBuilderEnabled_DO_NOT_USE([preferences
html5TreeBuilderEnabled]);
>      settings->setPaginateDuringLayoutEnabled([preferences
paginateDuringLayoutEnabled]);
> +#if ENABLE(FULLSCREEN)
> +    settings->setFullScreenEnabled([preferences fullScreenEnabled]);
> +#endif
>      settings->setMemoryInfoEnabled([preferences memoryInfoEnabled]);
>  }

Fullscreen/fullScreen inconsistency.

r- for breaking the existing video fullscreen.