[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
bugzilla-daemon at webkit.org
Thu Aug 5 14:12:45 PDT 2010
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.
More information about the webkit-reviews
mailing list