[webkit-reviews] review granted: [Bug 201024] [Picture-in-Picture Web API] Implement HTMLVideoElement.requestPictureInPicture() / Document.exitPictureInPicture() : [Attachment 380945] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Peng Liu
<peng.liu6 at apple.com>'s request for review:
Bug 201024: [Picture-in-Picture Web API] Implement
HTMLVideoElement.requestPictureInPicture() / Document.exitPictureInPicture()
https://bugs.webkit.org/show_bug.cgi?id=201024

Attachment 380945: Patch

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




--- Comment #30 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 380945
  --> https://bugs.webkit.org/attachment.cgi?id=380945
Patch

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

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:59
> +    return false;

This will prevent any page with a PictureInPictureWindow from entering the
PageCache. Can we do better?

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.h:47
> +    int width() const { return m_width; }
> +    int height() const { return m_height; }

You aren't maintaining PiP window 'state', which is important here as spec
says:

    The [width/height] attribute MUST return the [width/height] in CSS pixels
of 
    the Picture-in-Picture window associated with pictureInPictureElement if
    the state is opened. Otherwise, it MUST return 0.

> Source/WebCore/dom/Document.cpp:109
> +#if ENABLE(PICTURE_IN_PICTURE_API)
> +#include "HTMLVideoElement.h"
> +#endif

Nit: conditionally included headers are typically listed after the
unconditional ones.