Bug 212729 – A YouTube video gets stuck after rapidly tapping on touchbar’s PIP button

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

[Peng Liu]

Picture-in-picture for a YouTube video gets stuck after rapidly tapping on touchbar’s PIP button.

[Peng Liu]

*** Bug 212727 has been marked as a duplicate of this bug. ***

[Peng Liu]

*** Bug 212728 has been marked as a duplicate of this bug. ***

[Peng Liu]

<rdar://problem/54407450>

[Peng Liu]

Created attachment 400996 [details]
Patch

[Darin Adler]

Comment on attachment 400996 [details]
Patch

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

> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:37
>  #import "HTMLElement.h"
>  #import "HTMLMediaElement.h"
> +#import "HTMLVideoElement.h"

Don’t need HTMLElement.h or HTMLMediaElement.h if we are including HTMLVideoElement.h.

> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:319
> +    ASSERT(is<HTMLVideoElement>(*m_mediaElement));
> +    if (is<HTMLVideoElement>(*m_mediaElement)) {

I’d do early return rather than nesting everything in an if statement.

> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:320
> +        HTMLVideoElement& asVideo = downcast<HTMLVideoElement>(*m_mediaElement);

I’d write this:

    auto& element = downcast<HTMLVideoElement>(*m_mediaElement);

And then I would use element exclusively in the rest of the function, and not use m_mediaElement at all.

[Peng Liu]

Created attachment 401111 [details]
Revise the patch based on Darin's comments

[Peng Liu]

Created attachment 401112 [details]
Revise the patch based on Darin's comments - prepare for landing

[Peng Liu]

Comment on attachment 400996 [details]
Patch

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

>> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:37
>> +#import "HTMLVideoElement.h"
> 
> Don’t need HTMLElement.h or HTMLMediaElement.h if we are including HTMLVideoElement.h.

Right. Fixed.

>> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:319
>> +    if (is<HTMLVideoElement>(*m_mediaElement)) {
> 
> I’d do early return rather than nesting everything in an if statement.

Good point. Updated the patch.

>> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:320
>> +        HTMLVideoElement& asVideo = downcast<HTMLVideoElement>(*m_mediaElement);
> 
> I’d write this:
> 
>     auto& element = downcast<HTMLVideoElement>(*m_mediaElement);
> 
> And then I would use element exclusively in the rest of the function, and not use m_mediaElement at all.

Make sense! Updated the patch.

[EWS]

Committed r262599: <https://trac.webkit.org/changeset/262599>

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