Bug 215972

Created attachment 407526 [details]
WIP patch

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

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

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

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

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

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

Created attachment 407615 [details]
WIP - add changelogs

<rdar://problem/68098202>

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.

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

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/

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

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

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!

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.

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

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

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