Bug 146660 – Media Session: propagate metadata changes to UI clients

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

[Matt Rajca]

We need to give UI clients a chance to handle media metadata changes.

[Radar WebKit Bug Importer]

<rdar://problem/21694290>

[Matt Rajca]

Created attachment 256255 [details]
Patch

[Matt Rajca]

Created attachment 256258 [details]
Patch

[Eric Carlson]

Comment on attachment 256258 [details]
Patch

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

> Source/WebKit2/Shared/WebMediaSessionMetadata.cpp:38
> +PassRefPtr<WebMediaSessionMetadata> WebMediaSessionMetadata::create(const MediaSessionMetadata& metadata)
> +{
> +    return adoptRef(new WebMediaSessionMetadata(metadata));
> +}

The return type should be Ref<WebMediaSessionMetadata>, not PassRefPtr<WebMediaSessionMetadata>. See http://www.webkit.org/coding/RefPtr.html.

[Matt Rajca]

Created attachment 256308 [details]
Patch

[Matt Rajca]

(In reply to comment #4)
> Comment on attachment 256258 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256258&action=review
> 
> > Source/WebKit2/Shared/WebMediaSessionMetadata.cpp:38
> > +PassRefPtr<WebMediaSessionMetadata> WebMediaSessionMetadata::create(const MediaSessionMetadata& metadata)
> > +{
> > +    return adoptRef(new WebMediaSessionMetadata(metadata));
> > +}
> 
> The return type should be Ref<WebMediaSessionMetadata>, not
> PassRefPtr<WebMediaSessionMetadata>. See
> http://www.webkit.org/coding/RefPtr.html.

Fixed.

[Tim Horton]

Comment on attachment 256308 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIUIClient.h:61
> +#if ENABLE(MEDIA_SESSION)

no need to guard this forward-declaration (but it's fine either way)

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:112
> +#if ENABLE(MEDIA_SESSION)

ditto

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:171
> +WK_ADD_API_MAPPING(WKMediaSessionMetadataRef, WebMediaSessionMetadata)

Why is this mapping #if'd out if WKMediaSessionMetadata has all the functions but the contents of said functions are if'd out? Which way are you disabling this?

[Matt Rajca]

(In reply to comment #7)
> Comment on attachment 256308 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256308&action=review
> 
> > Source/WebKit2/UIProcess/API/APIUIClient.h:61
> > +#if ENABLE(MEDIA_SESSION)
> 
> no need to guard this forward-declaration (but it's fine either way)

I think I'll leave it in.

> 
> > Source/WebKit2/UIProcess/API/C/WKAPICast.h:112
> > +#if ENABLE(MEDIA_SESSION)
> 
> ditto
> 
> > Source/WebKit2/UIProcess/API/C/WKAPICast.h:171
> > +WK_ADD_API_MAPPING(WKMediaSessionMetadataRef, WebMediaSessionMetadata)
> 
> Why is this mapping #if'd out if WKMediaSessionMetadata has all the
> functions but the contents of said functions are if'd out? Which way are you
> disabling this?

The contents of the functions have to be #if'd out since it doesn't seem like we can use #if ENABLE in those header files. I can remove the #if guards around the mapping.

[Matt Rajca]

Created attachment 256318 [details]
Patch

[Tim Horton]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 256308 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=256308&action=review
> > 
> > > Source/WebKit2/UIProcess/API/APIUIClient.h:61
> > > +#if ENABLE(MEDIA_SESSION)
> > 
> > no need to guard this forward-declaration (but it's fine either way)
> 
> I think I'll leave it in.

That's fine! I only noted this because Darin has previously called this practice "overkill" (see https://bugs.webkit.org/show_bug.cgi?id=134461#c6).

> > Why is this mapping #if'd out if WKMediaSessionMetadata has all the
> > functions but the contents of said functions are if'd out? Which way are you
> > disabling this?
> 
> The contents of the functions have to be #if'd out since it doesn't seem
> like we can use #if ENABLE in those header files. I can remove the #if
> guards around the mapping.

That's what I was hoping you'd do :)

[Tim Horton]

Comment on attachment 256318 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKMediaSessionMetadata.cpp:49
> +    return 0;

These should be nullptr, not 0, I think?

> Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:456
> +    WKPageMediaSessionMetadataDidChangeCallback                         mediaSessionMetadataDidChange;

Please make sure that it's OK to add this without bumping the version number and that you do everything necessary to not break internal builds because of it.

[Matt Rajca]

(In reply to comment #11)
> Comment on attachment 256318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256318&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKMediaSessionMetadata.cpp:49
> > +    return 0;
> 
> These should be nullptr, not 0, I think?

Fixed. Not sure why we were using 0 in other WK* sources.

> 
> > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:456
> > +    WKPageMediaSessionMetadataDidChangeCallback                         mediaSessionMetadataDidChange;
> 
> Please make sure that it's OK to add this without bumping the version number
> and that you do everything necessary to not break internal builds because of
> it.

I'll watch the bots. Looks like Brady added a few callbacks last month and things were okay.

[Matt Rajca]

Committed r186484: <http://trac.webkit.org/changeset/186484>