Bug 146660

<rdar://problem/21694290>

Created attachment 256255 [details] Patch

Created attachment 256258 [details] Patch

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.

Created attachment 256308 [details] Patch

(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.

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?

(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.

Created attachment 256318 [details] Patch

(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 :)

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.

(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.

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