Bug 173341 – Consolidate WebKit UIKitSPI.h and UIKitTestSPI.h

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

[Jonathan Bedard]

Currently, we have 3 different UIKitSPI headers.  We should consolidate these headers.

[Radar WebKit Bug Importer]

<rdar://problem/32752890>

[Jonathan Bedard]

Dan Bates mentioned in bug 173319 that we should try and avoid declaring UIKit SPI in WebCore as a disincentive to using UIKit from the WebProcess.

This is a good point, although, currently, we have two UIKitSPI headers.  (One in WebCore and one in WebKit2) and a UIKitTestSPI header (as of <http://trac.webkit.org/changeset/218275>) which used to be name UIKitSPI as well.

The UIKitSPI header in WebCore doesn't have much in it, I wonder if, per Dan's point, we could remove it from WebCore entirely.

[Daniel Bates]

I am repurposing this bug to consolidate UIKitTestSPI.h and WebKit's UIKitSPI.h such that we only use WebKit's UIKitSPI.h. We likely will need to keep the UIKitSPI header in WebCore so long as we support Legacy WebKit. Reducing three SPI headers to two seems like an improvement and hence why I am repurposing this bug.

[Daniel Bates]

Created attachment 354639 [details]
Patch

[Daniel Bates]

Created attachment 354640 [details]
Patch

[Daniel Bates]

Created attachment 354642 [details]
Patch

[Jonathan Bedard]

Comment on attachment 354642 [details]
Patch

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

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:163
> +

We have an identical define in IOKitSPI.h, any reason we can't include that?

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:-956
> -@end

Let's double-check with Wenson here...in one header we have it guarded by ENABLE(DRAG_SUPPORT), in the other, we don't.

[Daniel Bates]

Comment on attachment 354642 [details]
Patch

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

>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:163
>> +
> 
> We have an identical define in IOKitSPI.h, any reason we can't include that?

That seems excessive as we only need this typedef.

>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:-956
>> -@end
> 
> Let's double-check with Wenson here...in one header we have it guarded by ENABLE(DRAG_SUPPORT), in the other, we don't.

What is the benefit of guarding this or any SPI forward declarations behind a WebKit feature flag?

[Daniel Bates]

Committed r238134: <https://trac.webkit.org/changeset/238134>

[Wenson Hsieh]

> > Source/WebKit/Platform/spi/ios/UIKitSPI.h:-956
> > -@end
> 
> Let's double-check with Wenson here...in one header we have it guarded by
> ENABLE(DRAG_SUPPORT), in the other, we don't.

Generally, it seems odd to guard declarations in SPI headers behind feature flags. Interfaces don't suddenly vanish from the SDK if WebKit decides to turn a feature flag off!

Ideally, version checks in SPI headers should use MAX_ALLOWED checks, since their purpose is to reflect what's available in the SDK being used to build.