Bug 190359

Summary: Enable external playback for video in element fullscreen.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: New BugsAssignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, jer.noble, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch For Landing none

Description Jeremy Jones 2018-10-08 10:42:01 PDT 
Enable external playback for video in element fullscreen.
Comment 1 Jeremy Jones 2018-10-08 10:47:46 PDT 
Created attachment 351791 [details]
Patch
Comment 2 Jeremy Jones 2018-10-08 10:48:26 PDT 
rdar://problem/42560085
Comment 3 Jeremy Jones 2018-10-08 12:38:18 PDT 
Created attachment 351802 [details]
Patch
Comment 4 Jeremy Jones 2018-10-08 12:46:35 PDT 
Created attachment 351804 [details]
Patch
Comment 5 Jeremy Jones 2018-10-08 13:00:21 PDT 
Created attachment 351808 [details]
Patch
Comment 6 Jeremy Jones 2018-11-02 11:13:45 PDT 
Created attachment 353716 [details]
Patch
Comment 7 Jer Noble 2018-11-02 11:17:33 PDT 
Comment on attachment 353716 [details]
Patch

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

r=me with nits

> Source/WebCore/html/HTMLMediaElement.h:737
> +    bool mediaPlayerVideoFullscreenStandby() const final { return m_videoFullscreenStandby; }

This is a verb that wants to be a noun. Could we do mediaPlayerIsVideoFullscreenStandby()? (and in all the related declarations)

> Source/WebCore/platform/graphics/MediaPlayer.cpp:724
> +void MediaPlayer::setVideoFullscreenStandby(bool isStandby)
> +{
> +    m_private->setVideoFullscreenStandby(isStandby);
> +}

We pass isStandby in here but never use it. Could we rename this "videoFullscreenStandbyChanged()"?
Comment 8 Jeremy Jones 2018-11-02 13:32:36 PDT 
Comment on attachment 353716 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.h:737
>> +    bool mediaPlayerVideoFullscreenStandby() const final { return m_videoFullscreenStandby; }
> 
> This is a verb that wants to be a noun. Could we do mediaPlayerIsVideoFullscreenStandby()? (and in all the related declarations)

Done.

>> Source/WebCore/platform/graphics/MediaPlayer.cpp:724
>> +}
> 
> We pass isStandby in here but never use it. Could we rename this "videoFullscreenStandbyChanged()"?

Done.
Comment 9 Jeremy Jones 2018-11-05 09:50:29 PST 
Created attachment 353882 [details]
Patch For Landing
Comment 10 WebKit Commit Bot 2018-11-16 16:49:58 PST 
Comment on attachment 353882 [details]
Patch For Landing

Clearing flags on attachment: 353882

Committed r238327: <https://trac.webkit.org/changeset/238327>
Comment 11 WebKit Commit Bot 2018-11-16 16:49:59 PST 
All reviewed patches have been landed.  Closing bug.