Bug 139137

Summary: Enable activating optimized fullscreen mode from standard fullscreen mode.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: MediaAssignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, philipj, sergio, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: iPhone / iPad   
OS: Unspecified   
Attachments:
Description Flags
Patch
jer.noble: review+
Patch for landing.
none
Patch for landing.
none
Patch for landing. none

Description Jeremy Jones 2014-12-01 11:09:43 PST 
Enable activating optimized fullscreen mode from standard fulscreen mode.
Comment 1 Jeremy Jones 2014-12-01 11:10:59 PST 
rdar://problem/18518346
Comment 2 Jeremy Jones 2014-12-01 12:12:51 PST 
Created attachment 242326 [details]
Patch
Comment 3 WebKit Commit Bot 2014-12-01 12:14:43 PST 
Attachment 242326 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:970:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jer Noble 2014-12-04 12:49:49 PST 
Comment on attachment 242326 [details]
Patch

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

r=me, with nits. This will need a 👍 from a WK2 owner before landing.

> Source/WebCore/html/HTMLMediaElement.h:395
> -    enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized };
> +    enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized, VideoFullscreenModeStandardAndOptimized };

Seems like this could be turned into a bitfield:

enum {
    VideoFullscreenModeNone = 0,
    VideoFullscreenModeStandard = 1 << 0,
    VideoFullscreenModeOptimized = 1 << 1,
};

typedef VideoFullscreenMode uint32_t;

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1044
> +    if (m_mode == HTMLMediaElement::VideoFullscreenModeOptimized || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized)
>          [m_playerViewController stopOptimizedFullscreen];
> -    else
> +    if (m_mode == HTMLMediaElement::VideoFullscreenModeStandard || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized)

These if() statements argue for the bitfield approach.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:238
> -    return _page->videoFullscreenManager() && _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized;
> +    return _page->videoFullscreenManager() && (_page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized || _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeStandardAndOptimized);

This return statement has become sufficiently complicated so as to justify breaking it up into multiple statements.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:89
> +    void setFullscreenModeChanged(uint32_t videoFullscreenMode);

Since this file includes HTMLMediaElement.h, you can just use HTMLMediaElement::VideoFullscreenMode here, rather than uint32_t, with the bitfield change above.
Comment 5 Simon Fraser (smfr) 2014-12-08 13:29:27 PST 
Comment on attachment 242326 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:92
> +    void setOptimizedActive(bool);

This is a confusing name. Is it "set the current optimized state to active" or "set optimized and active" or something else?

Maybe setIsOptimized()?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:525
> +    NSString* propertyName = NSStringFromSelector(@selector(optimizedFullscreenActive));

Will the property name ever be different from @"optimizedFullscreenActive"?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:531
> +    if (_avPlayerViewController)
> +        [_avPlayerViewController removeObserver:self forKeyPath:propertyName];
>      _avPlayerViewController = playerViewController;
> +    if (_avPlayerViewController)
> +        [_avPlayerViewController addObserver:self forKeyPath:propertyName options:(NSKeyValueObservingOptionNew | NSKeyValueObservingOptionOld) context:nil];

Messaging nil is safe.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1092
> +    if (!active && m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized)
> +        m_mode = HTMLMediaElement::VideoFullscreenModeStandard;
> +    else if (active && m_mode == HTMLMediaElement::VideoFullscreenModeStandard)
> +        m_mode = HTMLMediaElement::VideoFullscreenModeStandardAndOptimized;

Bitfields would help here too.
Comment 6 Jeremy Jones 2014-12-08 20:44:54 PST 
(In reply to comment #4)
> Comment on attachment 242326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242326&action=review
> 
> r=me, with nits. This will need a 👍 from a WK2 owner before landing.
> 
> > Source/WebCore/html/HTMLMediaElement.h:395
> > -    enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized };
> > +    enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized, VideoFullscreenModeStandardAndOptimized };
> 
> Seems like this could be turned into a bitfield:
> 
> enum {
>     VideoFullscreenModeNone = 0,
>     VideoFullscreenModeStandard = 1 << 0,
>     VideoFullscreenModeOptimized = 1 << 1,
> };
> 
> typedef VideoFullscreenMode uint32_t;

done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1044
> > +    if (m_mode == HTMLMediaElement::VideoFullscreenModeOptimized || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized)
> >          [m_playerViewController stopOptimizedFullscreen];
> > -    else
> > +    if (m_mode == HTMLMediaElement::VideoFullscreenModeStandard || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized)
> 
> These if() statements argue for the bitfield approach.

done.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:238
> > -    return _page->videoFullscreenManager() && _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized;
> > +    return _page->videoFullscreenManager() && (_page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized || _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeStandardAndOptimized);
> 
> This return statement has become sufficiently complicated so as to justify
> breaking it up into multiple statements.

done.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:89
> > +    void setFullscreenModeChanged(uint32_t videoFullscreenMode);
> 
> Since this file includes HTMLMediaElement.h, you can just use
> HTMLMediaElement::VideoFullscreenMode here, rather than uint32_t, with the
> bitfield change above.

done. Also updated WebVideoFullscreenManager.messages.in, and the other use of this enum in WebVideoFullscreenManagerProxy.messages.in
Comment 7 Jeremy Jones 2014-12-08 20:46:36 PST 
(In reply to comment #5)
> Comment on attachment 242326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242326&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:92
> > +    void setOptimizedActive(bool);
> 
> This is a confusing name. Is it "set the current optimized state to active"
> or "set optimized and active" or something else?
> 
> Maybe setIsOptimized()?

Changed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:525
> > +    NSString* propertyName = NSStringFromSelector(@selector(optimizedFullscreenActive));
> 
> Will the property name ever be different from @"optimizedFullscreenActive"?

No.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:531
> > +    if (_avPlayerViewController)
> > +        [_avPlayerViewController removeObserver:self forKeyPath:propertyName];
> >      _avPlayerViewController = playerViewController;
> > +    if (_avPlayerViewController)
> > +        [_avPlayerViewController addObserver:self forKeyPath:propertyName options:(NSKeyValueObservingOptionNew | NSKeyValueObservingOptionOld) context:nil];
> 
> Messaging nil is safe.

Removed conditions.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1092
> > +    if (!active && m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized)
> > +        m_mode = HTMLMediaElement::VideoFullscreenModeStandard;
> > +    else if (active && m_mode == HTMLMediaElement::VideoFullscreenModeStandard)
> > +        m_mode = HTMLMediaElement::VideoFullscreenModeStandardAndOptimized;
> 
> Bitfields would help here too.

Done.
Comment 8 Jeremy Jones 2014-12-08 21:41:50 PST 
Created attachment 242880 [details]
Patch for landing.
Comment 9 Jeremy Jones 2014-12-08 22:19:04 PST 
Created attachment 242884 [details]
Patch for landing.
Comment 10 Jeremy Jones 2014-12-08 22:26:16 PST 
rdar://problem/18914569
Comment 11 Jeremy Jones 2014-12-08 22:51:18 PST 
Created attachment 242885 [details]
Patch for landing.
Comment 12 WebKit Commit Bot 2014-12-08 22:53:08 PST 
Attachment 242885 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:971:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Commit Bot 2014-12-09 00:24:40 PST 
Comment on attachment 242885 [details]
Patch for landing.

Clearing flags on attachment: 242885

Committed r177012: <http://trac.webkit.org/changeset/177012>