Bug 180942 – Enable picture-in-picture from inline element on suspend.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Jones]

Enable picture-in-picture from inline element on suspend.

[Jeremy Jones]

Created attachment 329667 [details]
Preliminary

[Jeremy Jones]

Created attachment 329672 [details]
Preliminary

[EWS Watchlist]

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.

[Jeremy Jones]

Created attachment 329749 [details]
Preliminary

[EWS Watchlist]

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.

[Jeremy Jones]

Created attachment 329760 [details]
Preliminary

[EWS Watchlist]

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.

[Jer Noble]

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.

[Jer Noble]

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.

[Jeremy Jones]

(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

[Jeremy Jones]

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

[Jeremy Jones]

Created attachment 329835 [details]
Preliminary

[Jeremy Jones]

Created attachment 329852 [details]
Patch

[EWS Watchlist]

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.

[Jeremy Jones]

Created attachment 329875 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jeremy Jones]

Created attachment 329893 [details]
Patch

[EWS Watchlist]

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.

[Jer Noble]

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

[Jeremy Jones]

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

[Jeremy Jones]

Created attachment 329941 [details]
Patch for landing.

[EWS Watchlist]

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.

[Tim Horton]

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.

[Simon Fraser (smfr)]

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.

[Jeremy Jones]

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

[Jeremy Jones]

Created attachment 330015 [details]
Patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 330015 [details]
Patch for landing

Clearing flags on attachment: 330015

Committed r226217: <https://trac.webkit.org/changeset/226217>

[Brent Fulgham]

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.

[Brent Fulgham]

Debug build fix landed in r226223:
Committed r226223: <https://trac.webkit.org/changeset/226223>

[Radar WebKit Bug Importer]

<rdar://problem/36185373>