Bug 211585 - [macOS] Playhead in Touch Bar continues when loading stalls
Summary: [macOS] Playhead in Touch Bar continues when loading stalls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-07 11:46 PDT by Eric Carlson
Modified: 2020-05-07 16:08 PDT (History)
7 users (show)

See Also:

Attachments
Patch (3.15 KB, patch)
2020-05-07 11:50 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Patch for landing (3.13 KB, patch)
2020-05-07 14:01 PDT, Eric Carlson 		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 Eric Carlson 2020-05-07 11:46:42 PDT 
Playhead in Touch Bar should stop when loading stalls
Comment 1 Eric Carlson 2020-05-07 11:47:11 PDT 
<rdar://problem/33893306>
Comment 2 Eric Carlson 2020-05-07 11:50:52 PDT 
Created attachment 398771 [details]
Patch
Comment 3 Darin Adler 2020-05-07 12:56:26 PDT 
Comment on attachment 398771 [details]
Patch

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

> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:146
> +        bool isPlaying = this->isPlaying() && !this->isStalled();

No need for the second "this->". The first one is needed because of a conflict with the name of the local variable we are setting.

> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:436
> +    return m_mediaElement ? m_mediaElement->readyState() <= HTMLMediaElement::HAVE_CURRENT_DATA : false;

I like using && for expressions like this one.
Comment 4 Eric Carlson 2020-05-07 13:57:31 PDT 
Comment on attachment 398771 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:146
>> +        bool isPlaying = this->isPlaying() && !this->isStalled();
> 
> No need for the second "this->". The first one is needed because of a conflict with the name of the local variable we are setting.

Fixed.

>> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:436
>> +    return m_mediaElement ? m_mediaElement->readyState() <= HTMLMediaElement::HAVE_CURRENT_DATA : false;
> 
> I like using && for expressions like this one.

That is nicer, changed.
Comment 5 Eric Carlson 2020-05-07 14:01:25 PDT 
Created attachment 398795 [details]
Patch for landing
Comment 6 EWS 2020-05-07 16:08:49 PDT 
Committed r261342: <https://trac.webkit.org/changeset/261342>

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