Bug 170644 – [Cocoa] WebKit unnecessarily soft-links CorePrediction

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

CorePrediction isn’t dependent on WebKit, so it can be linked at build time. Patch forthcoming.

Created attachment 306580 [details]
Link CorePrediction at build time

[Build Bot]

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


ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:35:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:49:  The parameter name "model" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 5 files


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

(In reply to mitz from comment #1)
> Created attachment 306580 [details]
> Link CorePrediction at build time

This may break the iOS device build.

Looks like the “soft linking” was introduced in bug 168347 but it’s not entirely clear what problem it was meant to solve.

(In reply to mitz from comment #4)
> Looks like the “soft linking” was introduced in bug 168347 but it’s not
> entirely clear what problem it was meant to solve.

Perhaps because CorePrediction is 64-bit only.

[Alexey Proskuryakov]

Comment on attachment 306580 [details]
Link CorePrediction at build time

EWS says the build is broken indeed.

[John Wilander]

It is soft linked because of Base System. In recovery it's not there even though everything else indicates it is.

(In reply to John Wilander from comment #7)
> It is soft linked because of Base System. In recovery it's not there even
> though everything else indicates it is.

Thanks, John! It’s a misconception that libraries that aren’t present in the Base System need to be soft linked. Attachment 306580 [details] handles this using weak linking. I will update the patch to work with iOS device builds and 32-bit macOS.

Created attachment 306641 [details]
Patch for EWS

Created attachment 306642 [details]
Patch for EWS

[Build Bot]

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


ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:37:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:51:  The parameter name "model" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


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

Created attachment 306644 [details]
Patch for EWS

[Build Bot]

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


ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:37:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:51:  The parameter name "model" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 8 files


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

Created attachment 306772 [details]
Link CorePrediction at build time

[Build Bot]

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


ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:37:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/Platform/spi/Cocoa/CorePredictionSPI.h:51:  The parameter name "model" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 10 files


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

[Sam Weinig]

Comment on attachment 306772 [details]
Link CorePrediction at build time

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

> Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:75
> +static inline bool isNullFunctionPointer(void* functionPointer)

This surprising function should probably have a comment explaining why it needs inline-asm.  Also, is this something that can be lifted into WTF somewhere, as it seems like something that could be generally useful for other weak linking.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:36
> +#if PLATFORM(COCOA) && HAVE(CORE_PREDICTION)

Is it really necessary to have both predicates. Given that CORE_PREDICTION is only on COCOA systems, it seems like just specifying it would be enough.

(In reply to Sam Weinig from comment #16)
> Comment on attachment 306772 [details]
> Link CorePrediction at build time
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306772&action=review
> 
> > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:75
> > +static inline bool isNullFunctionPointer(void* functionPointer)
> 
> This surprising function should probably have a comment explaining why it
> needs inline-asm.

I’ll add a comment.

> Also, is this something that can be lifted into WTF
> somewhere, as it seems like something that could be generally useful for
> other weak linking.

I won’t do this as part of fixing this bug, but I agree. Is there an existing WTF header where this function could live?

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:36
> > +#if PLATFORM(COCOA) && HAVE(CORE_PREDICTION)
> 
> Is it really necessary to have both predicates. Given that CORE_PREDICTION
> is only on COCOA systems, it seems like just specifying it would be enough.

I’ll change to check only for HAVE(CORE_PREDICTION).

Fixed in <https://trac.webkit.org/r215247>.