Bug 209610 – Swipe down gestures cause the video layer to stick for a moment before bouncing back into place

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

[Peng Liu]

Swipe down gestures cause video layer to stick for a moment before bouncing back into place

[Peng Liu]

<rdar://problem/55814988>

[Peng Liu]

Created attachment 394629 [details]
Patch

[Darin Adler]

Comment on attachment 394629 [details]
Patch

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

We *must* find a way to test these animations rather than just making code changes and manually trying it out each time. This has turned out to be a difficult area to code properly and seems likely we will continue to break it over and over again in the future. It’s worth lots of extra effort to find a way to test this. Even if we can’t do it right at this moment and need to land this fix first.

> Source/WebCore/html/HTMLMediaElement.cpp:6132
> +        scheduleEvent(eventNames().webkitendfullscreenEvent);

Is this correct in the case where we are about to call *enter*VideoFullscreenForVideoElement because m_videoFullscreenStandby is true?

> Source/WebCore/html/HTMLMediaElement.cpp:6138
>          else
> -            document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this));
> -        scheduleEvent(eventNames().webkitendfullscreenEvent);
> +            m_fullscreenTaskQueue.enqueueTask([this] {
> +                document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this));
> +            });

Change log does not make it clear why enqueing this on the task queue will properly sequence with the scheduled event. Is there something that makes sure that the events and the full screen task queue work in sequence?

Multiple-line else body gets braces in WebKit coding style.

[Peng Liu]

Comment on attachment 394629 [details]
Patch

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

Completely agree with you that we need tests for the animation. But I need to complete a patch for webkit.org/b/203723 before we can add more tests for video fullscreen and PiP.

It is difficult to write reliable tests to test the animation itself, but we definitely can test the configurations related to animations in the WebCore side, e.g., the source/destination rect of animations. For this bug, such kind of tests can prevent regression in the future.

>> Source/WebCore/html/HTMLMediaElement.cpp:6132
>> +        scheduleEvent(eventNames().webkitendfullscreenEvent);
> 
> Is this correct in the case where we are about to call *enter*VideoFullscreenForVideoElement because m_videoFullscreenStandby is true?

When m_videoFullscreenStandby is true, this patch only changes the order to call scheduleEvent() and *enter*VideoFullscreenForVideoElement(). We will enter PiP when m_videoFullscreenStandby is true. It is a kind of exit fullscreen.

>> Source/WebCore/html/HTMLMediaElement.cpp:6138
>> +            });
> 
> Change log does not make it clear why enqueing this on the task queue will properly sequence with the scheduled event. Is there something that makes sure that the events and the full screen task queue work in sequence?
> 
> Multiple-line else body gets braces in WebKit coding style.

Unfortunately, this patch does not guarantee the things will happen in sequence. Eric proposed a solution: call exitVideoFullscreenForVideoElement() after HTMLElement::dispatchEvent(event) in HTMLMediaElement::dispatchEvent(). I will upload a new patch based on his solution.

Sorry, will fix the style issue.

[Peng Liu]

Created attachment 394647 [details]
Revised the patch based on comments from Darin and Eric

[EWS]

Committed r259095: <https://trac.webkit.org/changeset/259095>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394647 [details].