Bug 139137 – Enable activating optimized fullscreen mode from standard fullscreen mode.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Enable activating optimized fullscreen mode from standard fulscreen mode.

[Jeremy Jones]

rdar://problem/18518346

[Jeremy Jones]

Created attachment 242326 [details]
Patch

[WebKit Commit Bot]

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.

[Jer Noble]

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.

[Simon Fraser (smfr)]

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.

[Jeremy Jones]

(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

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 242880 [details]
Patch for landing.

[Jeremy Jones]

Created attachment 242884 [details]
Patch for landing.

[Jeremy Jones]

rdar://problem/18914569

[Jeremy Jones]

Created attachment 242885 [details]
Patch for landing.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 242885 [details]
Patch for landing.

Clearing flags on attachment: 242885

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