Bug 173341 - Consolidate WebKit UIKitSPI.h and UIKitTestSPI.h
Summary: Consolidate WebKit UIKitSPI.h and UIKitTestSPI.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks: 191596
  Show dependency treegraph
 
Reported: 2017-06-13 16:52 PDT by Jonathan Bedard
Modified: 2018-11-13 13:52 PST (History)
4 users (show)

See Also:


Attachments
Patch (19.54 KB, patch)
2018-11-12 20:45 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (19.96 KB, patch)
2018-11-12 20:57 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (19.97 KB, patch)
2018-11-12 21:02 PST, Daniel Bates
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-06-13 16:52:06 PDT
Currently, we have 3 different UIKitSPI headers.  We should consolidate these headers.
Comment 1 Radar WebKit Bug Importer 2017-06-13 16:52:33 PDT
<rdar://problem/32752890>
Comment 2 Jonathan Bedard 2017-06-15 08:19:27 PDT
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.
Comment 3 Daniel Bates 2018-11-12 20:30:17 PST
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.
Comment 4 Daniel Bates 2018-11-12 20:45:08 PST
Created attachment 354639 [details]
Patch
Comment 5 Daniel Bates 2018-11-12 20:57:33 PST
Created attachment 354640 [details]
Patch
Comment 6 Daniel Bates 2018-11-12 21:02:44 PST
Created attachment 354642 [details]
Patch
Comment 7 Jonathan Bedard 2018-11-13 08:46:46 PST
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.
Comment 8 Daniel Bates 2018-11-13 09:31:28 PST
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?
Comment 9 Daniel Bates 2018-11-13 09:36:04 PST
Committed r238134: <https://trac.webkit.org/changeset/238134>
Comment 10 Wenson Hsieh 2018-11-13 10:44:20 PST
> > 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.