[webkit-reviews] review granted: [Bug 172328] Prevent javascript interface from activating picture-in-picture for video elements that are showing capture camera on ios. : [Attachment 310585] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 172328: Prevent javascript interface from activating picture-in-picture for
video elements that are showing capture camera on ios.
https://bugs.webkit.org/show_bug.cgi?id=172328

Attachment 310585: Patch

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




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 310585
  --> https://bugs.webkit.org/attachment.cgi?id=310585
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.