Bug 141791 – Move more CoreMedia soft-linked functions to CoreMediaSoftLink.{cpp,h}

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Move more CoreMedia soft-linked functions to CoreMediaSoftLink.{cpp,h}

This does not move every function, but probably gets about half of the ones remaining.

[David Kilzer (:ddkilzer)]

Created attachment 246879 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> Move more CoreMedia soft-linked functions to CoreMediaSoftLink.{cpp,h}
> 
> This does not move every function, but probably gets about half of the ones
> remaining.

And by half I really mean closer to one-third.

[David Kilzer (:ddkilzer)]

Created attachment 246881 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Created attachment 246892 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Created attachment 246892 [details]
> Patch v3

Added CMTimeCompare() soft-link macros.  Not sure how I missed those.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Created attachment 246881 [details]
> Patch v2

The change from Patch v1 was to move these declarations into the CoreMediaSoftLink.{cpp,h} files:

typedef struct opaqueCMNotificationCenter* CMNotificationCenterRef;
43	typedef void (*CMNotificationCallback)(CMNotificationCenterRef inCenter, const void *inListener, CFStringRef inNotificationName, const void *inNotifyingObject, CFTypeRef inNotificationPayload);

However, this is SPI from the CMNotificationCenter.h "private" header, so it should go into its own *SPI.h header along with definitions for these functions:

CMNotificationCenterGetDefaultLocalCenter
CMNotificationCenterAddListener
CMNotificationCenterRemoveListener

Patch v4 forthcoming.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (In reply to comment #3)
> > Created attachment 246881 [details]
> > Patch v2
> 
> The change from Patch v1 was to move these declarations into the
> CoreMediaSoftLink.{cpp,h} files:
> 
> typedef struct opaqueCMNotificationCenter* CMNotificationCenterRef;
> 43	typedef void (*CMNotificationCallback)(CMNotificationCenterRef inCenter,
> const void *inListener, CFStringRef inNotificationName, const void
> *inNotifyingObject, CFTypeRef inNotificationPayload);
> 
> However, this is SPI from the CMNotificationCenter.h "private" header, so it
> should go into its own *SPI.h header along with definitions for these
> functions:
> 
> CMNotificationCenterGetDefaultLocalCenter
> CMNotificationCenterAddListener
> CMNotificationCenterRemoveListener
> 
> Patch v4 forthcoming.

Oops, I was wrong, CMNotificationCenter.h is API.  Patch v4 still forthcoming.

[David Kilzer (:ddkilzer)]

Created attachment 246893 [details]
Patch v4

[Brent Fulgham]

Comment on attachment 246893 [details]
Patch v4

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

> Source/WebCore/ChangeLog:48
> +        - Remove only CoreMedia soft-linked funtion.  It wasn't even

function! :-P

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #3)
> > > Created attachment 246881 [details]
> > > Patch v2
> > 
> > The change from Patch v1 was to move these declarations into the
> > CoreMediaSoftLink.{cpp,h} files:
> > 
> > typedef struct opaqueCMNotificationCenter* CMNotificationCenterRef;
> > 43	typedef void (*CMNotificationCallback)(CMNotificationCenterRef inCenter,
> > const void *inListener, CFStringRef inNotificationName, const void
> > *inNotifyingObject, CFTypeRef inNotificationPayload);
> > 
> > However, this is SPI from the CMNotificationCenter.h "private" header, so it
> > should go into its own *SPI.h header along with definitions for these
> > functions:
> > 
> > CMNotificationCenterGetDefaultLocalCenter
> > CMNotificationCenterAddListener
> > CMNotificationCenterRemoveListener
> > 
> > Patch v4 forthcoming.
> 
> Oops, I was wrong, CMNotificationCenter.h is API.  Patch v4 still
> forthcoming.

I was correct the first time.  Need a CoreMediaSPI.h header.  Good times.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Comment on attachment 246893 [details]
> Patch v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246893&action=review
> 
> > Source/WebCore/ChangeLog:48
> > +        - Remove only CoreMedia soft-linked funtion.  It wasn't even
> 
> function! :-P

check-webkit-style neads spel cheek fer ChangLoogs!

[David Kilzer (:ddkilzer)]

Created attachment 246894 [details]
Patch v5

[WebKit Commit Bot]

Attachment 246894 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:29:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 246894 [details]
Patch v5

r=me, please fix the build.

[Alex Christensen]

Adding platform\spi\cf to AdditionalIncludeDirectories in WebCore.vcxproj/WebCoreCFNetwork.props should fix Windows.

[Brent Fulgham]

(In reply to comment #15)
> Adding platform\spi\cf to AdditionalIncludeDirectories in
> WebCore.vcxproj/WebCoreCFNetwork.props should fix Windows.

Yes -- this is the right thing to correct the build.

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> (In reply to comment #15)
> > Adding platform\spi\cf to AdditionalIncludeDirectories in
> > WebCore.vcxproj/WebCoreCFNetwork.props should fix Windows.
> 
> Yes -- this is the right thing to correct the build.

Instead of adding an entry to Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters?

[Alex Christensen]

Do both.  WebCore.vcxproj.filters tells Visual Studio where to display the header file, but it does not affect compiling.  AdditionalIncludeDirectories adds basically a -I flag to the compiler.

[David Kilzer (:ddkilzer)]

Created attachment 246916 [details]
Patch v6 (for EWS build test)

[David Kilzer (:ddkilzer)]

Committed r180366: <http://trac.webkit.org/changeset/180366>