Bug 139437 - Enable automatic optimized fullscreen mode.
Summary: Enable automatic optimized fullscreen mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-09 02:34 PST by Jeremy Jones
Modified: 2014-12-09 14:39 PST (History)
10 users (show)

See Also:


Attachments
Patch (8.06 KB, patch)
2014-12-09 02:50 PST, Jeremy Jones
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing. (8.26 KB, patch)
2014-12-09 11:43 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2014-12-09 02:34:02 PST
Enable automatic optimized fullscreen mode.
Comment 1 Jeremy Jones 2014-12-09 02:35:11 PST
rdar://problem/18518346
Comment 2 Jeremy Jones 2014-12-09 02:50:35 PST
Created attachment 242900 [details]
Patch
Comment 3 Eric Carlson 2014-12-09 07:10:36 PST
Comment on attachment 242900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242900&action=review

r=me but this needs a thumbs-up from a WK2 reviewer as well.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:244
> +    if (_page->videoFullscreenManager()->mode() & WebCore::HTMLMediaElement::VideoFullscreenModeOptimized)
>          return true;
>      
>      return false;

Nit: this could be simplified to "return _page->videoFullscreenManager()->mode() & WebCore::HTMLMediaElement::VideoFullscreenModeOptimized"

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:255
> +    if (_page->videoFullscreenManager()->willAutomaticallyEnterOptimizedMode())
> +        return true;
> +    
> +    return false;

Nit: this could be simplified to "return _page->videoFullscreenManager()->willAutomaticallyEnterOptimizedMode()"

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:191
> +    if ([m_webView _willAutomaticallyEnterOptimizedVideoMode])
> +        return true;
> +    
> +    return false;

This could be simplified as well, but I think it is clearer as-is.
Comment 4 Jeremy Jones 2014-12-09 10:58:24 PST
(In reply to comment #3)
> Comment on attachment 242900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242900&action=review
> 
> r=me but this needs a thumbs-up from a WK2 reviewer as well.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:244
> > +    if (_page->videoFullscreenManager()->mode() & WebCore::HTMLMediaElement::VideoFullscreenModeOptimized)
> >          return true;
> >      
> >      return false;
> 
> Nit: this could be simplified to "return
> _page->videoFullscreenManager()->mode() &
> WebCore::HTMLMediaElement::VideoFullscreenModeOptimized"

Done.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:255
> > +    if (_page->videoFullscreenManager()->willAutomaticallyEnterOptimizedMode())
> > +        return true;
> > +    
> > +    return false;
> 
> Nit: this could be simplified to "return
> _page->videoFullscreenManager()->willAutomaticallyEnterOptimizedMode()"

Done.

> 
> > Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:191
> > +    if ([m_webView _willAutomaticallyEnterOptimizedVideoMode])
> > +        return true;
> > +    
> > +    return false;
> 
> This could be simplified as well, but I think it is clearer as-is.

Left as-is.
Comment 5 Simon Fraser (smfr) 2014-12-09 11:05:26 PST
Comment on attachment 242900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242900&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:238
> +    if (!_page || !_page->videoFullscreenManager())

Can _page actually be null? When does this happen?

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:189
> +    if ([m_webView _isPlayingFullscreenOptimizedVideo])
> +        return true;
> +    
> +    if ([m_webView _willAutomaticallyEnterOptimizedVideoMode])
> +        return true;

_willAutomaticallyEnterOptimizedVideoMode is confusing.

Isn't this really "playing fullscreen video" && "supports optimized fullscreen"?
Comment 6 Jeremy Jones 2014-12-09 11:43:11 PST
Created attachment 242951 [details]
Patch for landing.
Comment 7 Jeremy Jones 2014-12-09 11:45:29 PST
(In reply to comment #5)
> Comment on attachment 242900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242900&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:238
> > +    if (!_page || !_page->videoFullscreenManager())
> 
> Can _page actually be null? When does this happen?
> 
> > Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:189
> > +    if ([m_webView _isPlayingFullscreenOptimizedVideo])
> > +        return true;
> > +    
> > +    if ([m_webView _willAutomaticallyEnterOptimizedVideoMode])
> > +        return true;
> 
> _willAutomaticallyEnterOptimizedVideoMode is confusing.
> 
> Isn't this really "playing fullscreen video" && "supports optimized
> fullscreen"?

Changed these to _isShowingVideoOptimized and _mayAutomaticallyShowVideoOptimized
Comment 8 WebKit Commit Bot 2014-12-09 13:54:11 PST
Comment on attachment 242951 [details]
Patch for landing.

Clearing flags on attachment: 242951

Committed r177039: <http://trac.webkit.org/changeset/177039>