Bug 141791 - Move more CoreMedia soft-linked functions to CoreMediaSoftLink.{cpp,h}
Summary: Move more CoreMedia soft-linked functions to CoreMediaSoftLink.{cpp,h}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks: 141625
  Show dependency treegraph
 
Reported: 2015-02-18 21:47 PST by David Kilzer (:ddkilzer)
Modified: 2015-02-19 14:43 PST (History)
5 users (show)

See Also:


Attachments
Patch v1 (26.91 KB, patch)
2015-02-18 22:04 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (27.64 KB, patch)
2015-02-18 22:26 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (27.88 KB, patch)
2015-02-19 07:18 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (27.51 KB, patch)
2015-02-19 07:40 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v5 (34.28 KB, patch)
2015-02-19 09:13 PST, David Kilzer (:ddkilzer)
ap: review+
Details | Formatted Diff | Diff
Patch v6 (for EWS build test) (35.86 KB, patch)
2015-02-19 13:49 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2015-02-18 21:47:19 PST
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.
Comment 1 David Kilzer (:ddkilzer) 2015-02-18 22:04:04 PST
Created attachment 246879 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2015-02-18 22:05:14 PST
(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.
Comment 3 David Kilzer (:ddkilzer) 2015-02-18 22:26:26 PST
Created attachment 246881 [details]
Patch v2
Comment 4 David Kilzer (:ddkilzer) 2015-02-19 07:18:16 PST
Created attachment 246892 [details]
Patch v3
Comment 5 David Kilzer (:ddkilzer) 2015-02-19 07:21:33 PST
(In reply to comment #4)
> Created attachment 246892 [details]
> Patch v3

Added CMTimeCompare() soft-link macros.  Not sure how I missed those.
Comment 6 David Kilzer (:ddkilzer) 2015-02-19 07:31:21 PST
(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.
Comment 7 David Kilzer (:ddkilzer) 2015-02-19 07:37:17 PST
(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.
Comment 8 David Kilzer (:ddkilzer) 2015-02-19 07:40:14 PST
Created attachment 246893 [details]
Patch v4
Comment 9 Brent Fulgham 2015-02-19 08:35:39 PST
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
Comment 10 David Kilzer (:ddkilzer) 2015-02-19 08:36:05 PST
(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.
Comment 11 David Kilzer (:ddkilzer) 2015-02-19 09:08:14 PST
(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!
Comment 12 David Kilzer (:ddkilzer) 2015-02-19 09:13:59 PST
Created attachment 246894 [details]
Patch v5
Comment 13 WebKit Commit Bot 2015-02-19 09:15:54 PST
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.
Comment 14 Alexey Proskuryakov 2015-02-19 09:50:55 PST
Comment on attachment 246894 [details]
Patch v5

r=me, please fix the build.
Comment 15 Alex Christensen 2015-02-19 10:28:07 PST
Adding platform\spi\cf to AdditionalIncludeDirectories in WebCore.vcxproj/WebCoreCFNetwork.props should fix Windows.
Comment 16 Brent Fulgham 2015-02-19 10:39:54 PST
(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.
Comment 17 David Kilzer (:ddkilzer) 2015-02-19 11:00:17 PST
(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?
Comment 18 Alex Christensen 2015-02-19 11:07:57 PST
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.
Comment 19 David Kilzer (:ddkilzer) 2015-02-19 13:49:04 PST
Created attachment 246916 [details]
Patch v6 (for EWS build test)
Comment 20 David Kilzer (:ddkilzer) 2015-02-19 14:43:17 PST
Committed r180366: <http://trac.webkit.org/changeset/180366>