Bug 168056 – Transition "WebKit Library Version" checks to SDK version checks

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

[Brady Eidson]

Transition "WebKit Library Version" checks to SDK version checks

Apps can use WebKit without linking directly to it (e.g. They link to a different framework which links to WebKit)

In these cases the WebKit Library Version check will fail.

Whenever possible it's much better to reference the SDK version the app was built against.

<rdar://problem/30313696>

[Brady Eidson]

Created attachment 301056 [details]
Patch

[Geoffrey Garen]

Comment on attachment 301056 [details]
Patch

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

r=me

> Source/WebKit2/UIProcess/Cocoa/VersionChecks.h:32
> +enum class SDKVersion {

Since we're going to cast to uint32_t, we should inherit from uint32_t here.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 301056 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301056&action=review
> 
> r=me
> 
> > Source/WebKit2/UIProcess/Cocoa/VersionChecks.h:32
> > +enum class SDKVersion {
> 
> Since we're going to cast to uint32_t, we should inherit from uint32_t here.

👍👍

[Brady Eidson]

https://trac.webkit.org/changeset/211980

[Brady Eidson]

Build fix followup in https://trac.webkit.org/changeset/211987

[WebKit Commit Bot]

Re-opened since this is blocked by bug 168072

[Brady Eidson]

This change didn't allow ToT WebKit changes to work if you build against an older or not-yet-released SDK.

We can't just ignore WebKit library version numbers altogether - We need to try them first and then fall back to the SDK version number when necessary.

[Brady Eidson]

Created attachment 301093 [details]
Patch

[Geoffrey Garen]

Comment on attachment 301093 [details]
Patch

r=me

Is there any way for a developer to avoid using or specifying LibraryVersion?

LibraryVersion seems like an anti-pattern because we communicate compatibility issues to developers based on SDK version and not library version.

[Brady Eidson]

(In reply to comment #9)
> Comment on attachment 301093 [details]
> Patch
> 
> r=me
> 
> Is there any way for a developer to avoid using or specifying LibraryVersion? 

I don't know.

> LibraryVersion seems like an anti-pattern because we communicate
> compatibility issues to developers based on SDK version and not library
> version.

I agree with this, but we do have this dilemma...

Maybe the right approach is to do the SDK check *first*, then fallback to the library version if the SDK check returns false.

That would prefer SDK for 3rd party apps, but let ToT WebKit development prefer the library version for testing purposes.

[Brady Eidson]

(In reply to comment #10)
> (In reply to comment #9)
> > Comment on attachment 301093 [details]
> > Patch
> > 
> > r=me
> > 
> > Is there any way for a developer to avoid using or specifying LibraryVersion? 
> 
> I don't know.
> 
> > LibraryVersion seems like an anti-pattern because we communicate
> > compatibility issues to developers based on SDK version and not library
> > version.
> 
> I agree with this, but we do have this dilemma...
> 
> Maybe the right approach is to do the SDK check *first*, then fallback to
> the library version if the SDK check returns false.
> 
> That would prefer SDK for 3rd party apps, but let ToT WebKit development
> prefer the library version for testing purposes.

I'm going to do it this way and assume you're okay with it, and then you can yell at me later if that was a bad assumption.

[Brady Eidson]

https://trac.webkit.org/changeset/212000

[Alexey Proskuryakov]

Comment on attachment 301093 [details]
Patch

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

> Source/WTF/wtf/spi/darwin/dyldSPI.h:61
> +#define DYLD_MACOSX_VERSION_10_12_4 0x000A0C04

As previously commented in follow-up bug, we should never check for minor SDK version - it's not OK for WebKit to change behavior when the developer switches to a new minor Xcode version.

[Brady Eidson]

(In reply to comment #13)
> Comment on attachment 301093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301093&action=review
> 
> > Source/WTF/wtf/spi/darwin/dyldSPI.h:61
> > +#define DYLD_MACOSX_VERSION_10_12_4 0x000A0C04
> 
> As previously commented in follow-up bug, we should never check for minor
> SDK version - it's not OK for WebKit to change behavior when the developer
> switches to a new minor Xcode version.

Replying in the other bug.