Bug 215972 – Clean up functions and state variables related to the picture-in-picture implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Cleanup state variables related to the picture-in-picture implementation

[Peng Liu]

Created attachment 407526 [details]
WIP patch

[Peng Liu]

Created attachment 407527 [details]
WIP patch - fix build errors

[Peng Liu]

Created attachment 407529 [details]
WIP patch - fix build errors v2

[Peng Liu]

Created attachment 407541 [details]
WIP - revert a change leading to test failures

[Peng Liu]

Created attachment 407542 [details]
WIP - fix an API test failure

[Peng Liu]

Created attachment 407550 [details]
WIP - fix some layout test failures

[Peng Liu]

Created attachment 407552 [details]
WIP - fix an issue of picture-in-picture support on Mac

[Peng Liu]

Created attachment 407615 [details]
WIP - add changelogs

[Radar WebKit Bug Importer]

<rdar://problem/68098202>

[Darin Adler]

Comment on attachment 407615 [details]
WIP - add changelogs

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

I didn’t get to really review, but I did look over the patch and have a few style comments.

> Source/WebCore/html/HTMLMediaElement.cpp:6085
> +        downcast<HTMLVideoElement>(this)->exitToFullscreenModeWithoutAnimationIfPossible(oldVideoFullscreenMode, VideoFullscreenModeStandard);

It would be better like this:

    downcast<HTMLVideoElement>(*this).

Matches the line above.

> Source/WebCore/html/HTMLMediaElement.cpp:7914
> +    ASSERT(m_videoFullscreenMode != mode);
>      if (m_videoFullscreenMode == mode)
>          return;

Why is this assertion a good idea?

> Source/WebCore/html/HTMLVideoElement.cpp:480
> +    VideoFullscreenMode videoFullscreenMode = toFullscreenMode(mode);

auto

Also, it seems like in the context of this function we should use a shorter name than videoFullscreenMode. Maybe fullscreenMode.

> Source/WebCore/html/HTMLVideoElement.cpp:517
> +            m_pictureInPictureObserver->didEnterPictureInPicture(IntSize(size));

How did we select "casting to IntSize" over expandedIntSize, flooredIntSize (I think this does the same thing casting with better handling for edge cases), and roundedIntSize?

> Source/WebCore/html/HTMLVideoElement.h:85
>      enum class VideoPresentationMode { Inline, Fullscreen, PictureInPicture};

Missing space before the "}".

> Source/WebCore/html/HTMLVideoElement.h:99
> +    // HTMLMediaElement

I don’t think this comment is valuable.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1125
> +        WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView];

auto

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1126
> +        WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer];

auto

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1455
> +            WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView];

auto

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1456
> +            WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer];

auto

> Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:258
> +        // FIXME(rdar://problem/42250952)

This is not a great comment for people who can’t follow a rdar: link. Can we write words here?

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:601
> +        didEnterFullscreen(contextId, { m_mockPictureInPictureWindowSize.width(), m_mockPictureInPictureWindowSize.height() });

Why do we have to write out separate width and height in braces instead of just m_mockPictureInPictureWindowSize?

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:781
> -    m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId));
> +    if (size.isEmpty())
> +        m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, WTF::nullopt));
> +    else
> +        m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, size));

I’d rather use a local variable rather than repeating everything twice:

    Optional<FloatSize> optionalSize;
    if (!size.isEmpty())
        optionalSize = size;
    m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, optionalSize));

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:449
> -    videoElement->didBecomeFullscreenElement();
> +    if (size)
> +        videoElement->didEnterFullscreenOrPictureInPicture(size.value());
> +    else
> +        videoElement->didEnterFullscreenOrPictureInPicture({ });

There’s a function called Optional::valueOr that we should use in case like this so this can be one line of code instead of four.

[Peng Liu]

Created attachment 407745 [details]
WIP - revise the patch according to Darin's comments and fix some issues found in stress test

[Eric Carlson]

Comment on attachment 407745 [details]
WIP - revise the patch according to Darin's comments and fix some issues found in stress test

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

> Source/WebCore/ChangeLog:23
> +        the request. The motivation to do that is to guarantee correct events to be fired.

s/guarantee correct events to be fired/guarantee the correct events are fired/

> Source/WebCore/ChangeLog:25
> +        have a good way to create regression tests for the scenarios that presentation mode change

s/scenarios that presentation mode change/scenarios where presentation mode change/

[Peng Liu]

Created attachment 407785 [details]
Revise the patch according to Eric's comments and fix some issues found in stress tests

[Peng Liu]

Created attachment 407786 [details]
Revise the patch according to Eric's comments and fix some issues found in stress tests

[Peng Liu]

Comment on attachment 407615 [details]
WIP - add changelogs

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

Thanks for the review! I have revised the patch accordingly.

>> Source/WebCore/html/HTMLMediaElement.cpp:6085
>> +        downcast<HTMLVideoElement>(this)->exitToFullscreenModeWithoutAnimationIfPossible(oldVideoFullscreenMode, VideoFullscreenModeStandard);
> 
> It would be better like this:
> 
>     downcast<HTMLVideoElement>(*this).
> 
> Matches the line above.

Fixed.

>> Source/WebCore/html/HTMLMediaElement.cpp:7914
>>          return;
> 
> Why is this assertion a good idea?

Actually, both the assertion and checking need to be removed. HTMLMediaElement::setFullscreenMode is a private function and it expects callers to do the checking.

>> Source/WebCore/html/HTMLVideoElement.cpp:480
>> +    VideoFullscreenMode videoFullscreenMode = toFullscreenMode(mode);
> 
> auto
> 
> Also, it seems like in the context of this function we should use a shorter name than videoFullscreenMode. Maybe fullscreenMode.

Make sense. But "fullscreenMode" is a function name of HTMLMediaElement.

>> Source/WebCore/html/HTMLVideoElement.cpp:517
>> +            m_pictureInPictureObserver->didEnterPictureInPicture(IntSize(size));
> 
> How did we select "casting to IntSize" over expandedIntSize, flooredIntSize (I think this does the same thing casting with better handling for edge cases), and roundedIntSize?

The size will be used by JS code, and an error/offset of 1 is not a big issue. However, I think using flooredIntSize here is better.

>> Source/WebCore/html/HTMLVideoElement.h:85
>>      enum class VideoPresentationMode { Inline, Fullscreen, PictureInPicture};
> 
> Missing space before the "}".

Good catch! Fixed.

>> Source/WebCore/html/HTMLVideoElement.h:99
>> +    // HTMLMediaElement
> 
> I don’t think this comment is valuable.

Agree. Removed.

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1125
>> +        WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView];
> 
> auto

Fixed.

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1126
>> +        WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer];
> 
> auto

Fixed.

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1455
>> +            WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView];
> 
> auto

Fixed.

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1456
>> +            WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer];
> 
> auto

Fixed.

>> Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:258
>> +        // FIXME(rdar://problem/42250952)
> 
> This is not a great comment for people who can’t follow a rdar: link. Can we write words here?

Agree. Updated the comment.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:601
>> +        didEnterFullscreen(contextId, { m_mockPictureInPictureWindowSize.width(), m_mockPictureInPictureWindowSize.height() });
> 
> Why do we have to write out separate width and height in braces instead of just m_mockPictureInPictureWindowSize?

It is a mistake. Fixed.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:781
>> +        m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, size));
> 
> I’d rather use a local variable rather than repeating everything twice:
> 
>     Optional<FloatSize> optionalSize;
>     if (!size.isEmpty())
>         optionalSize = size;
>     m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, optionalSize));

Good idea! Fixed.

>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:449
>> +        videoElement->didEnterFullscreenOrPictureInPicture({ });
> 
> There’s a function called Optional::valueOr that we should use in case like this so this can be one line of code instead of four.

Great idea! Fixed.
Thanks!

[Peng Liu]

Comment on attachment 407745 [details]
WIP - revise the patch according to Darin's comments and fix some issues found in stress test

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

>> Source/WebCore/ChangeLog:23
>> +        the request. The motivation to do that is to guarantee correct events to be fired.
> 
> s/guarantee correct events to be fired/guarantee the correct events are fired/

Fixed.

>> Source/WebCore/ChangeLog:25
>> +        have a good way to create regression tests for the scenarios that presentation mode change
> 
> s/scenarios that presentation mode change/scenarios where presentation mode change/

Fixed.

[Peng Liu]

*** Bug 191629 has been marked as a duplicate of this bug. ***

[EWS]

Committed r266728: <https://trac.webkit.org/changeset/266728>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407786 [details].