Bug 226846

Summary: [Monterey] Support building WebKit
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, benjamin, cdumez, changseok, cmarcelo, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, jbedard, sam, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226780
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch
none
Patch for landing none

Description Jonathan Bedard 2021-06-09 13:28:15 PDT
We need to add some SPI declaration to build WebKit with the Monterey SDK.
Comment 1 Radar WebKit Bug Importer 2021-06-09 13:28:41 PDT
<rdar://problem/79095148>
Comment 2 Jonathan Bedard 2021-06-09 13:39:31 PDT
Created attachment 431005 [details]
Patch
Comment 3 Tim Horton 2021-06-09 13:48:42 PDT
Comment on attachment 431005 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/CryptoKitCBridgingSPI.h:51
> +- (instancetype)initWithPublicKey:(NSData*)spkiBytes error:(NSError * __autoreleasing *)error;

It seems like in nearby code we have fixed up the star-sidedness of the copied SPI to match WebKit style. But I can also see an argument for "just paste it as-is". Not sure which is preferable.

> Source/WebCore/testing/Internals.cpp:5644
> +        , Vector<TextRecognitionDataDetector>()

I don't understand this and the changelog doesn't help

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:457
> -                return { WebCore::ExceptionData { WebCore::InvalidStateError } };
> +                return { WebCore::ExceptionData { WebCore::InvalidStateError, String() } };

I don't understand these, and the changelog only says what you are doing, not why.
Comment 4 Wenson Hsieh 2021-06-09 13:56:29 PDT
Comment on attachment 431005 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/QuickLookMacSPI.h:76
> +#if ENABLE(IMAGE_ANALYSIS)

Nit - we should avoid using an ENABLE feature flag to guard this SPI header. Maybe we could add `HAVE_QUICKLOOK_PREVIEW_ACTIVITY` and use it here?
Comment 5 Wenson Hsieh 2021-06-09 14:00:54 PDT
Comment on attachment 431005 [details]
Patch

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

> Source/WebKit/Platform/spi/Cocoa/VisionKitSPI.h:26
> +#if ENABLE(IMAGE_ANALYSIS)

(Nit - similar comment here — maybe `HAVE_VK_IMAGE_ANALYSIS`?)
Comment 6 Jonathan Bedard 2021-06-09 14:25:42 PDT
Comment on attachment 431005 [details]
Patch

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

>> Source/WebCore/PAL/pal/spi/cocoa/CryptoKitCBridgingSPI.h:51
>> +- (instancetype)initWithPublicKey:(NSData*)spkiBytes error:(NSError * __autoreleasing *)error;
> 
> It seems like in nearby code we have fixed up the star-sidedness of the copied SPI to match WebKit style. But I can also see an argument for "just paste it as-is". Not sure which is preferable.

Agree with fixing the star-sidedness, style checker didn't complain, so I didn't notice it.

That being said, I'm unclear what the right thing to do with star-sidedness and __autoreleasing is

>> Source/WebCore/testing/Internals.cpp:5644
>> +        , Vector<TextRecognitionDataDetector>()
> 
> I don't understand this and the changelog doesn't help

I'll add the information to the changelog, but basically, we're getting this compiler error:

missing field 'message' initializer [-Werror,-Wmissing-field-initializers]
Comment 7 Jonathan Bedard 2021-06-09 14:46:02 PDT
Created attachment 431013 [details]
Patch
Comment 8 EWS 2021-06-11 12:21:04 PDT
Tools/Scripts/svn-apply failed to apply attachment 431013 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 9 Jonathan Bedard 2021-06-11 12:49:05 PDT
Created attachment 431222 [details]
Patch for landing
Comment 10 EWS 2021-06-11 13:44:28 PDT
Committed r278780 (238737@main): <https://commits.webkit.org/238737@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431222 [details].
Comment 11 Chris Dumez 2021-06-11 13:54:57 PDT
Seems to have broken internal build?

/Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/pal/spi/mac/QuickLookMacSPI.h:78:28: error: redefinition of 'QLPreviewActivity'
typedef NS_ENUM(NSInteger, QLPreviewActivity) {
Comment 12 Jonathan Bedard 2021-06-11 13:59:56 PDT
Committed r278781 (238738@main): <https://commits.webkit.org/238738@main>
Comment 13 Jonathan Bedard 2021-06-13 12:36:12 PDT
Reopening to attach new patch.
Comment 14 Jonathan Bedard 2021-06-13 12:36:14 PDT
Created attachment 431285 [details]
Patch
Comment 15 Chris Dumez 2021-06-14 08:55:03 PDT
Comment on attachment 431285 [details]
Patch

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

> Source/WebCore/html/HTMLElement.cpp:1377
> +                TextRecognitionLineElements lineElements { lineOrDataDetector, Vector<Ref<HTMLDivElement>>() };

Can probably use:
TextRecognitionLineElements lineElements { lineOrDataDetector, { } };

> Source/WebCore/html/HTMLElement.cpp:1430
> +            TextRecognitionLineElements lineElements { lineContainer, Vector<Ref<HTMLDivElement>>() };

ditto.
Comment 16 Jonathan Bedard 2021-06-14 09:33:36 PDT
Created attachment 431338 [details]
Patch
Comment 17 Jonathan Bedard 2021-06-14 09:39:25 PDT
(In reply to Chris Dumez from comment #15)
> Comment on attachment 431285 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431285&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:1377
> > +                TextRecognitionLineElements lineElements { lineOrDataDetector, Vector<Ref<HTMLDivElement>>() };
> 
> Can probably use:
> TextRecognitionLineElements lineElements { lineOrDataDetector, { } };
> 
> > Source/WebCore/html/HTMLElement.cpp:1430
> > +            TextRecognitionLineElements lineElements { lineContainer, Vector<Ref<HTMLDivElement>>() };
> 
> ditto.

Good call, that does work is certainly an improvement.
Comment 18 EWS 2021-06-14 10:34:31 PDT
Committed r278834 (238785@main): <https://commits.webkit.org/238785@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431338 [details].
Comment 19 Jonathan Bedard 2021-06-15 10:32:21 PDT
Reopening to attach new patch.
Comment 20 Jonathan Bedard 2021-06-15 10:32:24 PDT
Created attachment 431455 [details]
Patch for landing
Comment 21 EWS 2021-06-15 12:04:51 PDT
Committed r278889 (238831@main): <https://commits.webkit.org/238831@main>

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