Bug 212729 - A YouTube video gets stuck after rapidly tapping on touchbar’s PIP button
Summary: A YouTube video gets stuck after rapidly tapping on touchbar’s PIP button
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari Technology Preview
Hardware: Mac Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
: 212727 212728 (view as bug list)
Depends on: 202425
Blocks:
  Show dependency tree / graph
 
Reported: 2020-06-03 21:55 PDT by Peng Liu
Modified: 2020-06-05 09:57 PDT (History)
8 users (show)

See Also:

Attachments
Patch (2.58 KB, patch)
2020-06-03 22:11 PDT, Peng Liu 		darin: review+
Details | Formatted Diff | Diff
Revise the patch based on Darin's comments (2.58 KB, patch)
2020-06-04 20:01 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
Revise the patch based on Darin's comments - prepare for landing (2.60 KB, patch)
2020-06-04 20:06 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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].