Bug 164514 – REGRESSION (r208149): Media scrubber is not displayed in media controls

• Status: NEW
• Component: New Bugs

[Jer Noble]

REGRESSION (r208149): Media scrubber is not displayed in media controls

[Jer Noble]

rdar://problem/29098431

[Jer Noble]

Created attachment 294162 [details]
Patch

[Darin Adler]

Comment on attachment 294162 [details]
Patch

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

> Source/WebCore/html/MediaElementSession.h:174
> -static bool isType(const WebCore::PlatformMediaSession& session) { return session.mediaType() == WebCore::PlatformMediaSession::Video || session.mediaType() == WebCore::PlatformMediaSession::Audio; }
> +static bool isType(const WebCore::PlatformMediaSession& session)
> +{
> +    return session.mediaType() == WebCore::PlatformMediaSession::Video
> +        || session.mediaType() == WebCore::PlatformMediaSession::Audio
> +        || session.mediaType() == WebCore::PlatformMediaSession::VideoAudio;
> +}

Normally we’d like these isType functions to stay as tiny one-liners. Maybe we can make an inline helper for this to call?

I think it might be nicer to use switch for this rather than a return statement.

[Darin Adler]

Why no regression test?

[Jer Noble]

(In reply to comment #4)
> Why no regression test?

The patch should have mentioned that this fixes a number of broken TestWebKitAPI tests.

[Jer Noble]

(In reply to comment #3)
> Comment on attachment 294162 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294162&action=review
> 
> > Source/WebCore/html/MediaElementSession.h:174
> > -static bool isType(const WebCore::PlatformMediaSession& session) { return session.mediaType() == WebCore::PlatformMediaSession::Video || session.mediaType() == WebCore::PlatformMediaSession::Audio; }
> > +static bool isType(const WebCore::PlatformMediaSession& session)
> > +{
> > +    return session.mediaType() == WebCore::PlatformMediaSession::Video
> > +        || session.mediaType() == WebCore::PlatformMediaSession::Audio
> > +        || session.mediaType() == WebCore::PlatformMediaSession::VideoAudio;
> > +}
> 
> Normally we’d like these isType functions to stay as tiny one-liners. Maybe
> we can make an inline helper for this to call?
> 
> I think it might be nicer to use switch for this rather than a return
> statement.

I'm imagining some template method like:
template<typename T>
bool isIn(T value, initializer_list<T> candidates) {
    for (auto& candidate : candidates) {
        if (value == candidate)
            return true;
    }
    return false;
}

Then, the isType() implementation looks like:

    return isIn(session.mediaType(), { WebCore::PlatformMediaSession::Video, WebCore::PlatformMediaSession::Audio, WebCore::PlatformMediaSession::VideoAudio});

[Jer Noble]

Created attachment 294179 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 294179 [details]
Patch for landing

Clearing flags on attachment: 294179

Committed r208464: <http://trac.webkit.org/changeset/208464>