[webkit-reviews] review granted: [Bug 202774] [Picture-in-Picture Web API] The implementation needs runtime logging : [Attachment 381335] 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 202774: [Picture-in-Picture Web API] The implementation needs runtime
logging
https://bugs.webkit.org/show_bug.cgi?id=202774

Attachment 381335: Patch

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




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

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

>
Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:73
> +    LOG(Media, "HTMLVideoElementPictureInPicture::requestPictureInPicture");

ALWAYS_LOG(LOGIDENTIFIER)

>
Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:80
> +	   promise->reject(InvalidStateError, "The video element is not ready
to enter the Picture-in-Picture mode.");

You should add logging for each of these rejections, eg:

    ERROR_LOG(LOGIDENTIFIER, "The video element is not ready to enter the
Picture-in-Picture mode")

 The string in the Promise rejection is available to script, but the runtime
logging will show up in the console log so we can diagnose failures later.

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.h:84
> +    RefPtr<Logger> m_logger;

This should be a Ref<const Logger>