Summary: | [Monterey] Support building WebKit | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||||||
Component: | Tools / Tests | Assignee: | 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
Jonathan Bedard
2021-06-09 13:28:15 PDT
Created attachment 431005 [details]
Patch
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 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 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 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] Created attachment 431013 [details]
Patch
Tools/Scripts/svn-apply failed to apply attachment 431013 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Created attachment 431222 [details]
Patch for landing
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]. 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) { Committed r278781 (238738@main): <https://commits.webkit.org/238738@main> Reopening to attach new patch. Created attachment 431285 [details]
Patch
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. Created attachment 431338 [details]
Patch
(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. 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]. Reopening to attach new patch. Created attachment 431455 [details]
Patch for landing
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]. |