Bug 203728 - macCatalyst: Get TestWebKitAPI running
Summary: macCatalyst: Get TestWebKitAPI running
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-01 00:17 PDT by Tim Horton
Modified: 2019-11-01 14:30 PDT (History)
12 users (show)

See Also:


Attachments
Patch (16.99 KB, patch)
2019-11-01 00:17 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (16.98 KB, patch)
2019-11-01 00:34 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (19.50 KB, patch)
2019-11-01 01:12 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (20.47 KB, patch)
2019-11-01 12:48 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2019-11-01 00:17:18 PDT
macCatalyst: Get TestWebKitAPI building
Comment 1 Tim Horton 2019-11-01 00:17:52 PDT
Created attachment 382564 [details]
Patch
Comment 2 Tim Horton 2019-11-01 00:34:46 PDT
Created attachment 382566 [details]
Patch
Comment 3 Tim Horton 2019-11-01 01:12:12 PDT
Created attachment 382567 [details]
Patch
Comment 4 Jonathan Bedard 2019-11-01 08:23:54 PDT
Comment on attachment 382567 [details]
Patch

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

> Tools/Scripts/webkitpy/port/mac.py:294
> +        self._config = Config(self._executive, self._filesystem, MacCatalystPort.port_name)

What are we adding this for? Wouldn’t expect we needed it to get things building (we will need it to run them, but that’s a different change)
Comment 5 Wenson Hsieh 2019-11-01 08:37:56 PDT
Comment on attachment 382567 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Challenge.mm:148
> +#if HAVE(SEC_KEY_PROXY) && HAVE(SSL)

Nit - let’s update the #endif comment to match the new #if.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm:28
> +#if ENABLE(DRAG_SUPPORT) && !PLATFORM(MACCATALYST)

Ditto.

> Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm:28
> +#if ENABLE(DRAG_SUPPORT) && PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST)

Ditto.

> Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm:29
> +#if ENABLE(DRAG_SUPPORT) && PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST)

Ditto.
Comment 6 Tim Horton 2019-11-01 08:46:40 PDT
(In reply to Jonathan Bedard from comment #4)
> Comment on attachment 382567 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382567&action=review
> 
> > Tools/Scripts/webkitpy/port/mac.py:294
> > +        self._config = Config(self._executive, self._filesystem, MacCatalystPort.port_name)
> 
> What are we adding this for? Wouldn’t expect we needed it to get things
> building (we will need it to run them, but that’s a different change)

This gets the tests building AND running. I should update the title.
Comment 7 Alexey Proskuryakov 2019-11-01 10:21:41 PDT
Comment on attachment 382567 [details]
Patch

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

We don't have any support for macCatalyst in build-webkit, do we?

> Tools/TestWebKitAPI/config.h:75
> +#if PLATFORM(MACCATALYST)
> +#import <WebKit/WebKitLegacy.h>
> +#endif

I see the explanation in ChangeLog, but it doesn't tell me why it's needed at all - and for something as unexpected as this, a code comment would be appropriate.
Comment 8 Tim Horton 2019-11-01 10:29:10 PDT
(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 382567 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382567&action=review
> 
> We don't have any support for macCatalyst in build-webkit, do we?

🤷‍♂️

> > Tools/TestWebKitAPI/config.h:75
> > +#if PLATFORM(MACCATALYST)
> > +#import <WebKit/WebKitLegacy.h>
> > +#endif
> 
> I see the explanation in ChangeLog, but it doesn't tell me why it's needed
> at all - and for something as unexpected as this, a code comment would be
> appropriate.

Because many of the WebKitLegacy tests expect to get WebKitLegacy.h through WebKit.h? I thought the changelog comment was sufficient but I can beef it up and leave a comment in the code too.
Comment 9 Jonathan Bedard 2019-11-01 10:54:44 PDT
(In reply to Tim Horton from comment #8)
> (In reply to Alexey Proskuryakov from comment #7)
> > Comment on attachment 382567 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=382567&action=review
> > 
> > We don't have any support for macCatalyst in build-webkit, do we?
> 
> 🤷‍♂️
>


We do not. I looked into this a few weeks ago, it's not a small task, and probably closely related to some work Keith Rollin has been doing with tvOS and watchOS (to be clear, not volunteering him, just providing some context)
Comment 10 Tim Horton 2019-11-01 12:48:41 PDT
Created attachment 382611 [details]
Patch
Comment 11 WebKit Commit Bot 2019-11-01 14:28:39 PDT
The commit-queue encountered the following flaky tests while processing attachment 382611 [details]:

requestidlecallback/requestidlecallback-document-gc.html bug 203758 (author: rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2019-11-01 14:29:32 PDT
Comment on attachment 382611 [details]
Patch

Clearing flags on attachment: 382611

Committed r251941: <https://trac.webkit.org/changeset/251941>
Comment 13 WebKit Commit Bot 2019-11-01 14:29:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-11-01 14:30:19 PDT
<rdar://problem/56828694>