Bug 202774

Summary: [Picture-in-Picture Web API] The implementation needs runtime logging
Product: WebKit Reporter: Peng Liu <peng.liu6>
Component: MediaAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, ews-watchlist, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 201024    
Bug Blocks: 182688    
Attachments:
Description Flags
Patch
none
Patch
eric.carlson: review+
A revised patch
none
Patch none

Peng Liu
Reported 2019-10-09 16:35:06 PDT
This bug is created to add runtime logging to the implementation.
Attachments
Patch (8.12 KB, patch)
2019-10-18 14:44 PDT, Peng Liu 		no flags
DetailsFormatted DiffDiff
Patch (8.12 KB, patch)
2019-10-18 14:54 PDT, Peng Liu 		eric.carlson: review+
DetailsFormatted DiffDiff
A revised patch (8.87 KB, patch)
2019-10-18 17:06 PDT, Peng Liu 		no flags
DetailsFormatted DiffDiff
Patch (8.31 KB, patch)
2019-10-22 12:41 PDT, Peng Liu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-10-14 15:35:30 PDT
<rdar://problem/56266937>
Peng Liu
Comment 2 2019-10-18 14:44:49 PDT
Created attachment 381333 [details] Patch
Peng Liu
Comment 3 2019-10-18 14:54:51 PDT
Created attachment 381335 [details] Patch
Eric Carlson
Comment 4 2019-10-18 16:29:57 PDT
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 5 2019-10-18 16:49:46 PDT
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
Comment 6 2019-10-18 17:06:26 PDT
Created attachment 381349 [details] A revised patch
Peng Liu
Comment 7 2019-10-22 12:41:05 PDT
Created attachment 381584 [details] Patch Updated the patch after discussion with Eric offline
WebKit Commit Bot
Comment 8 2019-10-22 14:28:43 PDT
Comment on attachment 381584 [details] Patch Clearing flags on attachment: 381584 Committed r251458: <https://trac.webkit.org/changeset/251458>
WebKit Commit Bot
Comment 9 2019-10-22 14:28:44 PDT
All reviewed patches have been landed. Closing bug.
Eric Carlson
Comment 10 2019-10-25 06:58:22 PDT
(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
Comment 11 2019-10-25 07:17:54 PDT
Right. We cannot use macros like ERROR_LOG because the function is static.
Note You need to log in before you can comment on or make changes to this bug.