Bug 202774 – [Picture-in-Picture Web API] The implementation needs runtime logging

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

This bug is created to add runtime logging to the implementation.

[Radar WebKit Bug Importer]

<rdar://problem/56266937>

[Peng Liu]

Created attachment 381333 [details]
Patch

[Peng Liu]

Created attachment 381335 [details]
Patch

[Eric Carlson]

Comment on attachment 381335 [details]
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>

[Peng Liu]

Comment on attachment 381335 [details]
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)

Cannot use ALWAYS_LOG() here because this function is static.

>> 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.

Right, will fix it.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.h:84
>> +    RefPtr<Logger> m_logger;
> 
> This should be a Ref<const Logger>

Right, will fix it.

[Peng Liu]

Created attachment 381349 [details]
A revised patch

[Peng Liu]

Created attachment 381584 [details]
Patch

Updated the patch after discussion with Eric offline

[WebKit Commit Bot]

Comment on attachment 381584 [details]
Patch

Clearing flags on attachment: 381584

Committed r251458: <https://trac.webkit.org/changeset/251458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

(In reply to Peng Liu from comment #5)
> Comment on attachment 381335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381335&action=review
> 
> >> 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.
> 
> Right, will fix it.
> 
  This was not changed in the final patch.

[Peng Liu]

Right. We cannot use macros like ERROR_LOG because the function is static.