Bug 212729

Summary:

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

Product:

WebKit

Reporter:

Peng Liu <peng.liu6>

Component:

Media

Assignee:

Peng Liu <peng.liu6>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Mac

OS:

Unspecified

Bug Depends on:

202425

Bug Blocks:

Attachments:

Description	Flags
Patch	darin: review+
Revise the patch based on Darin's comments	none
Revise the patch based on Darin's comments - prepare for landing	none

Description Peng Liu 2020-06-03 21:55:36 PDT

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

Comment 1 Peng Liu 2020-06-03 22:00:18 PDT

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

Comment 2 Peng Liu 2020-06-03 22:01:13 PDT

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

Comment 3 Peng Liu 2020-06-03 22:02:50 PDT

<rdar://problem/54407450>

Comment 4 Peng Liu 2020-06-03 22:11:54 PDT

Created attachment 400996 [details]
Patch

Comment 5 Darin Adler 2020-06-04 16:56:46 PDT

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.

Comment 6 Peng Liu 2020-06-04 20:01:24 PDT

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

Comment 7 Peng Liu 2020-06-04 20:06:42 PDT

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

Comment 8 Peng Liu 2020-06-04 20:09:02 PDT

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.

Comment 9 EWS 2020-06-04 22:52:27 PDT

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

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