Bug 172328 – Prevent javascript interface from activating picture-in-picture for video elements that are showing capture camera on ios.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Prevent javascript interface from activating picture-in-picture for video elements that are showing capture camera on ios.

[Jeremy Jones]

rdar://problem/32286221

[Jeremy Jones]

Created attachment 310585 [details]
Patch

[Eric Carlson]

Comment on attachment 310585 [details]
Patch

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

> Source/WebCore/html/HTMLVideoElement.cpp:160
> +        if (player() && !player()->supportsPictureInPicture())
> +            return false;

Nit: I see that there is another "if (player() && player()->xxx)" in this method, followed by an unguarded use of "player()", so apparently this method can't succeed if player() is NULL. Can you remove this check and add an "ASSERT(player())" at the beginning of the method to verify this in debug builds, plus an early return to avoid a release build crash?

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.h:83
> +    bool supportsPictureInPicture() const override { return true; };
> +    bool supportsFullscreen() const override { return true; };

Nit: I doubt PiP works in QTKit, I would skip this.

[Jeremy Jones]

Comment on attachment 310585 [details]
Patch

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

>> Source/WebCore/html/HTMLVideoElement.cpp:160
>> +            return false;
> 
> Nit: I see that there is another "if (player() && player()->xxx)" in this method, followed by an unguarded use of "player()", so apparently this method can't succeed if player() is NULL. Can you remove this check and add an "ASSERT(player())" at the beginning of the method to verify this in debug builds, plus an early return to avoid a release build crash?

it is actually a if (!player() || !player()->supportsFullscreen()) return false.  That is the line that allows the later code to just use dereference player() without checking.

I can just add a if (!player()) return false; to the top of the function and remove that check throughout.

>> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.h:83
>> +    bool supportsFullscreen() const override { return true; };
> 
> Nit: I doubt PiP works in QTKit, I would skip this.

Skipped QTKit changes.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:-761
> -

Skipped this change too.

[Jeremy Jones]

Created attachment 310705 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 310705 [details]
Patch for landing.

Clearing flags on attachment: 310705

Committed r217175: <http://trac.webkit.org/changeset/217175>