Bug 180942 - Enable picture-in-picture from inline element on suspend.
Summary: Enable picture-in-picture from inline element on suspend.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-18 13:25 PST by Jeremy Jones
Modified: 2017-12-21 14:18 PST (History)
9 users (show)

See Also:


Attachments
Preliminary (207.16 KB, patch)
2017-12-18 13:26 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Preliminary (211.57 KB, patch)
2017-12-18 13:33 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Preliminary (184.78 KB, patch)
2017-12-19 01:24 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Preliminary (186.96 KB, patch)
2017-12-19 06:58 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Preliminary (188.61 KB, patch)
2017-12-19 14:56 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (139.46 KB, patch)
2017-12-19 16:32 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (101.21 KB, patch)
2017-12-19 19:04 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.77 MB, application/zip)
2017-12-19 20:32 PST, Build Bot
no flags Details
Patch (99.36 KB, patch)
2017-12-19 21:07 PST, Jeremy Jones
jer.noble: review+
Details | Formatted Diff | Diff
Patch for landing. (122.55 KB, patch)
2017-12-20 11:52 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing (124.08 KB, patch)
2017-12-21 00:06 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2017-12-18 13:25:03 PST
Enable picture-in-picture from inline element on suspend.
Comment 1 Jeremy Jones 2017-12-18 13:26:20 PST
Created attachment 329667 [details]
Preliminary
Comment 2 Jeremy Jones 2017-12-18 13:33:45 PST
Created attachment 329672 [details]
Preliminary
Comment 3 Build Bot 2017-12-18 13:35:43 PST
Attachment 329672 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit2.mm:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit2.h:112:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jeremy Jones 2017-12-19 01:24:59 PST
Created attachment 329749 [details]
Preliminary
Comment 5 Build Bot 2017-12-19 01:26:31 PST
Attachment 329749 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Jeremy Jones 2017-12-19 06:58:52 PST
Created attachment 329760 [details]
Preliminary
Comment 7 Build Bot 2017-12-19 07:00:52 PST
Attachment 329760 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:220:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Jer Noble 2017-12-19 13:49:07 PST
Comment on attachment 329760 [details]
Preliminary

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

> Source/WebCore/page/ChromeClient.h:340
> -    virtual void enterVideoFullscreenForVideoElement(HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode) { }
> +    virtual void enterVideoFullscreenForVideoElement(HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode, bool) { }

Looks like this change is making the Windows bots unhappy (because they've marked as final a method that's no longer virtual). You'll probably have to update their headers to add this new parameter there.
Comment 9 Jer Noble 2017-12-19 14:17:46 PST
Comment on attachment 329760 [details]
Preliminary

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

So much of the VideoFullscreenInterfaceAVKit code is bifurcated into #if ENABLE(FULLSCREEN_API) and #else portions, I wonder if it would be better to have a "FullscreenAPI" and "NonFullscreenAPI" subclasses of the two, and switch which is allocated at runtime.

> Source/WebCore/dom/Document.cpp:6217
> +    m_fullScreenChangeDelayTimer.startOneShot(0_s);

These should really be GenericTaskQueues rather than 0_s Timers.  Do we need to start the timer both on willEnter and didEnter?

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:132
> +#if ENABLE(FULLSCREEN_API)
> +    } m_currentMode, m_targetMode;
> +#else
> +    } m_currentMode;
> +#endif

This is a really weird way of initializing these member variables. Please re-word this to something more like:

Mode m_currentMode;
#if ENABLE(FULLSCREEN_API)
Mode m_targetMode;
#endif

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:626
> +#if ENABLE(FULLSCREEN_API)
> +    m_targetStandby = standby;
> +    m_targetMode = mode;
> +    setInlineRect(initialRect, true);
> +    doSetup();
> +#else
> +    ASSERT_UNUSED(standby, !standby);
> +    bool isInPictureInPictureMode = m_currentMode.hasPictureInPicture();
> +    m_currentMode = mode;
> +    m_inlineRect = initialRect;

m_currentMode is only ever set here (and not in the ENABLE(FULLSCREEN_API) branch). This seems incorrect, since it's used outside that feature check elsewhere.
Comment 10 Jeremy Jones 2017-12-19 14:28:28 PST
(In reply to Jer Noble from comment #8)
> Comment on attachment 329760 [details]
> Preliminary
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329760&action=review
> 
> > Source/WebCore/page/ChromeClient.h:340
> > -    virtual void enterVideoFullscreenForVideoElement(HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode) { }
> > +    virtual void enterVideoFullscreenForVideoElement(HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode, bool) { }
> 
> Looks like this change is making the Windows bots unhappy (because they've
> marked as final a method that's no longer virtual). You'll probably have to
> update their headers to add this new parameter there.

Added the param to win WebChromeClient.h/cpp
Comment 11 Jeremy Jones 2017-12-19 14:55:04 PST
(In reply to Jer Noble from comment #9)
> Comment on attachment 329760 [details]
> Preliminary
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329760&action=review
> 
> So much of the VideoFullscreenInterfaceAVKit code is bifurcated into #if
> ENABLE(FULLSCREEN_API) and #else portions, I wonder if it would be better to
> have a "FullscreenAPI" and "NonFullscreenAPI" subclasses of the two, and
> switch which is allocated at runtime.

I had a previous version that did this. There ended up being a lot of duplicated code.

I expect the ENABLE(FULLSCREEN_API) branch to replace the other branch when the time comes, so I'd like to keep both version as harmonized as possible so we don't lose bug fixes.

If you think it would be clearer, I could duplicate more of the conditionalized functions to make make two non-conditionalized versions.

> 
> > Source/WebCore/dom/Document.cpp:6217
> > +    m_fullScreenChangeDelayTimer.startOneShot(0_s);
> 
> These should really be GenericTaskQueues rather than 0_s Timers.  Do we need
> to start the timer both on willEnter and didEnter?

I'll file a follow up bug to switch m_fullScreenChangeDelayTimer and related timers to GenericTaskQueue.

This change moves the fullscreenChanged timer from didEnter to willEnter. This allows JS to update page state before the animation begins.
I'm not sure if this is compatible with the spec, so I only changed it for this particular build.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:132
> > +#if ENABLE(FULLSCREEN_API)
> > +    } m_currentMode, m_targetMode;
> > +#else
> > +    } m_currentMode;
> > +#endif
> 
> This is a really weird way of initializing these member variables. Please
> re-word this to something more like:
> 
> Mode m_currentMode;
> #if ENABLE(FULLSCREEN_API)
> Mode m_targetMode;
> #endif

Done.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:626
> > +#if ENABLE(FULLSCREEN_API)
> > +    m_targetStandby = standby;
> > +    m_targetMode = mode;
> > +    setInlineRect(initialRect, true);
> > +    doSetup();
> > +#else
> > +    ASSERT_UNUSED(standby, !standby);
> > +    bool isInPictureInPictureMode = m_currentMode.hasPictureInPicture();
> > +    m_currentMode = mode;
> > +    m_inlineRect = initialRect;
> 
> m_currentMode is only ever set here (and not in the ENABLE(FULLSCREEN_API)
> branch). This seems incorrect, since it's used outside that feature check
> elsewhere.

m_currentMode is modified via setMode() clearMode().

ENABLE(FULLSCREEN_API) code sets m_targetMode here, then updates m_currentMode as the current state actually changes.
!ELEMENT(FULLSCREEN_API) code only has m_currentMode.
Comment 12 Jeremy Jones 2017-12-19 14:56:27 PST
Created attachment 329835 [details]
Preliminary
Comment 13 Jeremy Jones 2017-12-19 16:32:42 PST
Created attachment 329852 [details]
Patch
Comment 14 Build Bot 2017-12-19 17:15:52 PST
Attachment 329852 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:877:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Jeremy Jones 2017-12-19 19:04:03 PST
Created attachment 329875 [details]
Patch
Comment 16 Build Bot 2017-12-19 19:05:33 PST
Attachment 329875 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Build Bot 2017-12-19 20:32:28 PST
Comment on attachment 329875 [details]
Patch

Attachment 329875 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5765540

New failing tests:
fast/mediastream/MediaStream-MediaElement-setObject-null.html
Comment 18 Build Bot 2017-12-19 20:32:30 PST
Created attachment 329887 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Jeremy Jones 2017-12-19 21:07:46 PST
Created attachment 329893 [details]
Patch
Comment 20 Build Bot 2017-12-19 21:09:05 PST
Attachment 329893 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Jer Noble 2017-12-20 10:50:28 PST
Comment on attachment 329893 [details]
Patch

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

r=me with nits.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:596
> -        [m_playerViewController exitFullScreenAnimated:NO completionHandler:[protectedThis, this] (BOOL, NSError*) {
> +        [m_playerViewController exitFullScreenAnimated:NO completionHandler:[protectedThis, this] (BOOL success, NSError *error) {
> +            UNUSED_PARAM(success);
> +            UNUSED_PARAM(error);

Nit: this seems unnecessary.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:885
> -            [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL, NSError*) {
> +            [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL success, NSError *error) {
> +                UNUSED_PARAM(success);
> +                UNUSED_PARAM(error);

Ditto.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:988
> +

Extraneous whitespace.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1264
> +    RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> +    RetainPtr<id> strongCompletionHandler = adoptNS([completionHandler copy]);
> +    fullscreenMayReturnToInline([protectedThis, strongCompletionHandler](bool restored)  {

nit: fullscreenMayReturnToInline([protectedThis = makeRefPtr(this), strongCompletionHandler = adoptNS([completionHandler copy])](bool restored) {

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1267
> +        void (^completionHandler)(BOOL restored) = strongCompletionHandler.get();
> +        completionHandler(restored);

nit: "auto" or just "strongCompletionHandler.get()(restored)"

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1279
> +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> +        dispatch_async(dispatch_get_main_queue(), [protectedThis, this] {

nit: "protectedThis = makeRefPtr(this)"

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1420
> +    RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> +    dispatch_async(dispatch_get_main_queue(), [protectedThis, this] {

Ditto.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1446
> +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> +        [m_playerViewController enterFullScreenAnimated:YES completionHandler:[this, protectedThis] (BOOL success, NSError *error) {

Ditto.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1466
> +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> +        [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL success, NSError *error) {

Ditto.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1498
> +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> +        [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL success, NSError *error) {

Ditto.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1524
> +    RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> +    dispatch_async(dispatch_get_main_queue(), [protectedThis, this] {

Ditto.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1616
> +
> +

Whitespace

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:218
> +    RefPtr<VideoFullscreenControllerContext> protectedThis(this);
> +    WebThreadRun([protectedThis, this] {

Ditto protectedThis.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:235
> +    RetainPtr<CALayer> videoFullscreenLayer = [m_videoFullscreenView layer];
> +    RefPtr<VideoFullscreenControllerContext> protectedThis(this);
> +    WebThreadRun([protectedThis, this, videoFullscreenLayer] {

Ditto protectedThis and videoFullscreenLayer (with retainPtr() ).

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:254
> +    RetainPtr<CALayer> videoFullscreenLayer = [m_videoFullscreenView layer];
> +    RefPtr<VideoFullscreenControllerContext> protectedThis(this);
> +    WebThreadRun([protectedThis, this, videoFullscreenLayer] {

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:256
> +        m_fullscreenModel->setVideoFullscreenLayer(nil, [protectedThis, this] {

WTFMove().
Comment 22 Jeremy Jones 2017-12-20 11:42:41 PST
(In reply to Jer Noble from comment #21)
> Comment on attachment 329893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329893&action=review
> 
> r=me with nits.
> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:596
> > -        [m_playerViewController exitFullScreenAnimated:NO completionHandler:[protectedThis, this] (BOOL, NSError*) {
> > +        [m_playerViewController exitFullScreenAnimated:NO completionHandler:[protectedThis, this] (BOOL success, NSError *error) {
> > +            UNUSED_PARAM(success);
> > +            UNUSED_PARAM(error);
> 
> Nit: this seems unnecessary.

Removed param names and unused macro in 4 places.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:885
> > -            [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL, NSError*) {
> > +            [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL success, NSError *error) {
> > +                UNUSED_PARAM(success);
> > +                UNUSED_PARAM(error);
> 
> Ditto.

Done.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:988
> > +
> 
> Extraneous whitespace.

Gone.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1264
> > +    RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> > +    RetainPtr<id> strongCompletionHandler = adoptNS([completionHandler copy]);
> > +    fullscreenMayReturnToInline([protectedThis, strongCompletionHandler](bool restored)  {
> 
> nit: fullscreenMayReturnToInline([protectedThis = makeRefPtr(this),
> strongCompletionHandler = adoptNS([completionHandler copy])](bool restored) {

Done 2x.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1267
> > +        void (^completionHandler)(BOOL restored) = strongCompletionHandler.get();
> > +        completionHandler(restored);
> 
> nit: "auto" or just "strongCompletionHandler.get()(restored)"

Done 2x.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1279
> > +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> > +        dispatch_async(dispatch_get_main_queue(), [protectedThis, this] {
> 
> nit: "protectedThis = makeRefPtr(this)"

Changed to this protectThis = makeRefPtr/WTFMove pattern everywhere across modified files.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1420
> > +    RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> > +    dispatch_async(dispatch_get_main_queue(), [protectedThis, this] {
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1446
> > +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> > +        [m_playerViewController enterFullScreenAnimated:YES completionHandler:[this, protectedThis] (BOOL success, NSError *error) {
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1466
> > +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> > +        [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL success, NSError *error) {
> 
> Ditto.
Ditto.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1498
> > +        RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> > +        [m_playerViewController exitFullScreenAnimated:YES completionHandler:[protectedThis, this] (BOOL success, NSError *error) {
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1524
> > +    RefPtr<VideoFullscreenInterfaceAVKit> protectedThis(this);
> > +    dispatch_async(dispatch_get_main_queue(), [protectedThis, this] {
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1616
> > +
> > +
> 
> Whitespace

Removed

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:218
> > +    RefPtr<VideoFullscreenControllerContext> protectedThis(this);
> > +    WebThreadRun([protectedThis, this] {
> 
> Ditto protectedThis.

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:235
> > +    RetainPtr<CALayer> videoFullscreenLayer = [m_videoFullscreenView layer];
> > +    RefPtr<VideoFullscreenControllerContext> protectedThis(this);
> > +    WebThreadRun([protectedThis, this, videoFullscreenLayer] {
> 
> Ditto protectedThis and videoFullscreenLayer (with retainPtr() ).

Fixed 5x.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:254
> > +    RetainPtr<CALayer> videoFullscreenLayer = [m_videoFullscreenView layer];
> > +    RefPtr<VideoFullscreenControllerContext> protectedThis(this);
> > +    WebThreadRun([protectedThis, this, videoFullscreenLayer] {
> 
> Ditto.

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:256
> > +        m_fullscreenModel->setVideoFullscreenLayer(nil, [protectedThis, this] {
> 
> WTFMove().

Fixed this in many places.
Comment 23 Jeremy Jones 2017-12-20 11:52:43 PST
Created attachment 329941 [details]
Patch for landing.
Comment 24 Build Bot 2017-12-20 11:55:02 PST
Attachment 329941 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Tim Horton 2017-12-20 12:21:42 PST
Comment on attachment 329941 [details]
Patch for landing.

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

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h:140
> +    void setupFullscreenWithID(uint64_t contextId, uint32_t videoLayerID, const WebCore::IntRect& initialRect, float hostingScaleFactor, WebCore::HTMLMediaElementEnums::VideoFullscreenMode, bool allowsPictureInPicture, bool standby);

At some point this might deserve a struct? Also maybe enum (class) instead of bools?

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:291
> +        [CATransaction commit];

Do we need this explicit commit? Everything will happen together in the implicit commit, no? If the reason you’re doing it is for setDisableActions, you should use WebActionDisablingCALayerDelegate instead.
Comment 26 Simon Fraser (smfr) 2017-12-20 12:57:40 PST
Comment on attachment 329941 [details]
Patch for landing.

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

> Source/WebCore/html/HTMLMediaElement.cpp:5898
>      else
>  #endif
>      if (document().page()->chrome().client().supportsVideoFullscreen(oldVideoFullscreenMode)) {
> -        document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this));
> +        if (m_videoFullscreenStandby)
> +            document().page()->chrome().client().enterVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this), m_videoFullscreenMode, m_videoFullscreenStandby);
> +        else
> +            document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this));

The fact that there are all these downcast<HTMLVideoElement> here suggests that this code should move to HTMLVideoElement.

> Source/WebCore/html/HTMLMediaElement.cpp:5905
> +    ASSERT(is<HTMLVideoElement>(*this));

So why is it on HTMLMediaElement?

> Source/WebCore/html/HTMLMediaElement.cpp:7472
> -        if (m_videoFullscreenMode == VideoFullscreenModeStandard && supportsPictureInPicture() && isPlaying())
> +        if (((m_videoFullscreenMode == VideoFullscreenModeStandard) || m_videoFullscreenStandby) && supportsPictureInPicture() && isPlaying())

These also sound very video-related.

> Source/WebCore/page/ChromeClient.h:340
> +    virtual void enterVideoFullscreenForVideoElement(HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode, bool) { }

Mysterious bool parameter. At least name it here in the API.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:110
> +        void operator=(HTMLMediaElementEnums::VideoFullscreenMode mode) {m_mode = mode;}

Space after the {. Why do you need this operator?

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:116
> +        void setModeValue(HTMLMediaElementEnums::VideoFullscreenMode mode, bool value) { value ? setMode(mode) : clearMode(mode); }
> +        void setMode(HTMLMediaElementEnums::VideoFullscreenMode mode) { m_mode |= mode; }
> +        void clearMode(HTMLMediaElementEnums::VideoFullscreenMode mode) { m_mode &= ~mode; }
> +        bool hasMode(HTMLMediaElementEnums::VideoFullscreenMode mode) const { return m_mode & mode; }

Aren't you really just re-implementing OptionSet<> here?

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:213
> +    bool m_exitFullscreenNeedsExitFullscreen { false };
> +    bool m_exitFullscreenNeedsExitPictureInPicture { false };
> +    bool m_exitFullscreenNeedsReturnContentLayer { false };
> +
> +    bool m_enterFullscreenNeedsEnterFullscreen { false };
> +    bool m_enterFullscreenNeedsExitFullscreen { false };
> +    bool m_enterFullscreenNeedsEnterPictureInPicture { false };
> +    bool m_enterFullscreenNeedsExitPictureInPicture { false };

It's hard to tell from the names what these are actually for. And there are so many. Can you group them into structs or something?

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1038
> +void VideoFullscreenInterfaceAVKit::exitFullscreen(const IntRect& finalRect)

Blank line above please.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1204
> +        // ASSUMPTION: we are exiting pip because we are entering fullscreen

Can we assert that?

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h:157
> +    void requestUpdateInlineRect(uint64_t contextId);
> +    void requestVideoContentLayer(uint64_t contextId);
> +    void returnVideoContentLayer(uint64_t contextId);

What kind of context is this referring to? Can we use something more strongly typed than uint64_t? See wtf/Identified.h.

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:277
> +#ifndef NDEBUG
> +        [videoLayer setName:@"Web video fullscreen manager layer"];
> +#endif

We want these names to show in non-debug builds too.
Comment 27 Jeremy Jones 2017-12-20 22:50:59 PST
(In reply to Simon Fraser (smfr) from comment #26)
> Comment on attachment 329941 [details]
> Patch for landing.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329941&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5898
> >      else
> >  #endif
> >      if (document().page()->chrome().client().supportsVideoFullscreen(oldVideoFullscreenMode)) {
> > -        document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this));
> > +        if (m_videoFullscreenStandby)
> > +            document().page()->chrome().client().enterVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this), m_videoFullscreenMode, m_videoFullscreenStandby);
> > +        else
> > +            document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this));
> 
> The fact that there are all these downcast<HTMLVideoElement> here suggests
> that this code should move to HTMLVideoElement.

I'll file a follow up bug to explore moving all fullscreen code out of media element and into video element.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5905
> > +    ASSERT(is<HTMLVideoElement>(*this));
> 
> So why is it on HTMLMediaElement?

It interacts with fullscreen code which is in HTMLMediaElement.
We'll look into moving all this code into video element.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:7472
> > -        if (m_videoFullscreenMode == VideoFullscreenModeStandard && supportsPictureInPicture() && isPlaying())
> > +        if (((m_videoFullscreenMode == VideoFullscreenModeStandard) || m_videoFullscreenStandby) && supportsPictureInPicture() && isPlaying())
> 
> These also sound very video-related.

Ditto.

> 
> > Source/WebCore/page/ChromeClient.h:340
> > +    virtual void enterVideoFullscreenForVideoElement(HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode, bool) { }
> 
> Mysterious bool parameter. At least name it here in the API.

"standby"

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:110
> > +        void operator=(HTMLMediaElementEnums::VideoFullscreenMode mode) {m_mode = mode;}
> 
> Space after the {. Why do you need this operator?

Space added.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:116
> > +        void setModeValue(HTMLMediaElementEnums::VideoFullscreenMode mode, bool value) { value ? setMode(mode) : clearMode(mode); }
> > +        void setMode(HTMLMediaElementEnums::VideoFullscreenMode mode) { m_mode |= mode; }
> > +        void clearMode(HTMLMediaElementEnums::VideoFullscreenMode mode) { m_mode &= ~mode; }
> > +        bool hasMode(HTMLMediaElementEnums::VideoFullscreenMode mode) const { return m_mode & mode; }
> 
> Aren't you really just re-implementing OptionSet<> here?

That appears to be the case.
I'll file a follow up to adopt OptionSet<>.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:213
> > +    bool m_exitFullscreenNeedsExitFullscreen { false };
> > +    bool m_exitFullscreenNeedsExitPictureInPicture { false };
> > +    bool m_exitFullscreenNeedsReturnContentLayer { false };
> > +
> > +    bool m_enterFullscreenNeedsEnterFullscreen { false };
> > +    bool m_enterFullscreenNeedsExitFullscreen { false };
> > +    bool m_enterFullscreenNeedsEnterPictureInPicture { false };
> > +    bool m_enterFullscreenNeedsExitPictureInPicture { false };
> 
> It's hard to tell from the names what these are actually for. And there are
> so many. Can you group them into structs or something?

I have a longer term plan that will remove all these.
I'll file a follow up bug to address this in the nearer term.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1038
> > +void VideoFullscreenInterfaceAVKit::exitFullscreen(const IntRect& finalRect)
> 
> Blank line above please.

Done.

> 
> > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1204
> > +        // ASSUMPTION: we are exiting pip because we are entering fullscreen
> 
> Can we assert that?

Deleted the comment. It is unnecessary, and just describes the conditions of the "if".

> 
> > Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h:157
> > +    void requestUpdateInlineRect(uint64_t contextId);
> > +    void requestVideoContentLayer(uint64_t contextId);
> > +    void returnVideoContentLayer(uint64_t contextId);
> 
> What kind of context is this referring to? Can we use something more
> strongly typed than uint64_t? See wtf/Identified.h.

I'll file a follow up to clean this up.

> 
> > Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:277
> > +#ifndef NDEBUG
> > +        [videoLayer setName:@"Web video fullscreen manager layer"];
> > +#endif
> 
> We want these names to show in non-debug builds too.

Done.
Comment 28 Jeremy Jones 2017-12-21 00:06:49 PST
Created attachment 330015 [details]
Patch for landing
Comment 29 Build Bot 2017-12-21 00:08:18 PST
Attachment 330015 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 WebKit Commit Bot 2017-12-21 02:08:18 PST
Comment on attachment 330015 [details]
Patch for landing

Clearing flags on attachment: 330015

Committed r226217: <https://trac.webkit.org/changeset/226217>
Comment 31 Brent Fulgham 2017-12-21 09:05:09 PST
This introduced a build failure in Debug:

/Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:440:1: error: function 'setInlineRect'
      could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
{
^
/Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:449:1: error: function
      'setHasVideoContentLayer' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
{
^
2 errors generated.
Comment 32 Brent Fulgham 2017-12-21 09:18:00 PST
Debug build fix landed in r226223:
Committed r226223: <https://trac.webkit.org/changeset/226223>
Comment 33 Radar WebKit Bug Importer 2017-12-21 14:18:37 PST
<rdar://problem/36185373>