Bug 212994 – WebKit: Add ClockKitSPI.h

• Component: Platform
• Status: RESOLVED FIXED

[Jonathan Bedard]

To build watchOS, we need an SPI header for ClockKit.

[Radar WebKit Bug Importer]

<rdar://problem/64181092>

[Jonathan Bedard]

Created attachment 401480 [details]
Patch

[Wenson Hsieh]

Comment on attachment 401480 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        * UIProcess/ios/forms/WKTimePickerViewController.mm: Import ClockKitSPI.h and UIkitSSPI.

Nit - “UIkitSSPI” => “UIKitSPI.h"

> Source/WebKit/Platform/spi/watchos/ClockKitSPI.h:46
> +@protocol CLKUIWheelsOfTimeDelegate <NSObject> @end

Nit - the @end should go on the next line.

[Jonathan Bedard]

Created attachment 401513 [details]
Patch

[Wenson Hsieh]

Comment on attachment 401513 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        * Platform/spi/watchos/ClockKitSPI.h: Added.

Hm...I don’t see ClockKitSPI.h in the updated patch.

[Wenson Hsieh]

Comment on attachment 401513 [details]
Patch

I’ve unset the cq+ flag for now, based on the above comment. Please feel free to cq+ again if I’ve missed something!

[Jonathan Bedard]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 401513 [details]
> Patch
> 
> I’ve unset the cq+ flag for now, based on the above comment. Please feel
> free to cq+ again if I’ve missed something!

Thanks Wenson! Not sure how svn dropped the actual header..... :/

[Jonathan Bedard]

Created attachment 401540 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #8)
> Created attachment 401540 [details]
> Patch

I think we might be relying on https://bugs.webkit.org/show_bug.cgi?id=212996 with PepperUICoreSPI.h imported via WKQuickboardListViewController.h, so I'm going to hold off landing this one.

[Darin Adler]

Comment on attachment 401513 [details]
Patch

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

>> Source/WebKit/ChangeLog:12
>> +        * Platform/spi/watchos/ClockKitSPI.h: Added.
> 
> Hm...I don’t see ClockKitSPI.h in the updated patch.

I think our strategy is to add new SPI headers to PAL, even when the usage is in WebKit, not WebCore. Is that not right?

[Tim Horton]

(In reply to Darin Adler from comment #10)
> Comment on attachment 401513 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401513&action=review
> 
> >> Source/WebKit/ChangeLog:12
> >> +        * Platform/spi/watchos/ClockKitSPI.h: Added.
> > 
> > Hm...I don’t see ClockKitSPI.h in the updated patch.
> 
> I think our strategy is to add new SPI headers to PAL, even when the usage
> is in WebKit, not WebCore. Is that not right?

I think: not for things that are "above" WebCore in build order.

[Tim Horton]

(At least, I have seen that justification (and "to avoid suggesting that it's OK to use these things in WebCore or WebKitLegacy") provided for the two UIKit SPI headers, and for choosing which to add to).

[Darin Adler]

(In reply to Tim Horton from comment #11)
> I think: not for things that are "above" WebCore in build order.

I’d like someone to write out our SPI strategy explicitly. I’d possibly like to debate it and make it explicit.

We have redundancy and conflicts between SPI headers, headers with the same names and different contents, headers that define the same things in different ways, and other messy things.

Having WTF, WebCore, WebKit, and WebKitLegacy each have its own set of SPI headers seems like a recipe for duplication and trouble since these projects are built on top of each other and could intersect in new ways as we change includes. I’d be tempted to say we should put it all in one place rather than refining and having each framework only declare what it uses.

[Jonathan Bedard]

(In reply to Darin Adler from comment #13)
> (In reply to Tim Horton from comment #11)
> > I think: not for things that are "above" WebCore in build order.
> 
> I’d like someone to write out our SPI strategy explicitly. I’d possibly like
> to debate it and make it explicit.
> 
> We have redundancy and conflicts between SPI headers, headers with the same
> names and different contents, headers that define the same things in
> different ways, and other messy things.
> 
> Having WTF, WebCore, WebKit, and WebKitLegacy each have its own set of SPI
> headers seems like a recipe for duplication and trouble since these projects
> are built on top of each other and could intersect in new ways as we change
> includes. I’d be tempted to say we should put it all in one place rather
> than refining and having each framework only declare what it uses.

I think the biggest source of contention is that many of these frameworks should only ever be used in WebKit (as I believe is the case with ClockKit and PepperUICore, the two SPI headers I'm adding to make watchOS build), so there has been a movement towards making such frameworks difficult to use inside WTF/WebCore/WebKitLegacy.

[Darin Adler]

(In reply to Jonathan Bedard from comment #14)
> I think the biggest source of contention is that many of these frameworks
> should only ever be used in WebKit (as I believe is the case with ClockKit
> and PepperUICore, the two SPI headers I'm adding to make watchOS build), so
> there has been a movement towards making such frameworks difficult to use
> inside WTF/WebCore/WebKitLegacy.

Yes, a conflicting goal. Again, I’d like to debate this point.

I think there may be simple ways to achieve this other goal.

[Darin Adler]

For example, it’s not the fact that these are *SPI* that makes it incorrect to use them from WebCore, so we are entangling two different concerns here unnecessarily.

Making sure we don’t use frameworks inappropriately can apply equally to API. We should come up with solutions that work for both.

[Jonathan Bedard]

Created attachment 401580 [details]
Patch

[EWS]

Committed r262862: <https://trac.webkit.org/changeset/262862>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401580 [details].