Bug 238655

Summary: _WKDataTask doesn't work in macCatalyst
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, akeerthi, ap, benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, jer.noble, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch ews-feeder: commit-queue-

Description Tim Horton 2022-04-01 00:34:22 PDT
_WKDataTask doesn't work in macCatalyst
Comment 1 Tim Horton 2022-04-01 00:34:35 PDT
Created attachment 456331 [details]
Patch
Comment 2 Alexey Proskuryakov 2022-04-01 07:32:34 PDT
Comment on attachment 456331 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:458
> +    || (PLATFORM(MACCATALYST) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) \

Please remove obsolete OS version checks instead of adding more.
Comment 3 Brent Fulgham 2022-04-01 10:17:51 PDT
Comment on attachment 456331 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:1063
> +// FIXME: Does this intend to exclude macCatalyst?

I believe we do want this on, too.

> Source/WTF/wtf/PlatformHave.h:1201
> +// FIXME: Does this intend to exclude macCatalyst?

We should support this on macCatalyst, too.
Comment 4 Tim Horton 2022-04-01 10:19:28 PDT
Comment on attachment 456331 [details]
Patch

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

>> Source/WTF/wtf/PlatformHave.h:458
>> +    || (PLATFORM(MACCATALYST) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) \
> 
> Please remove obsolete OS version checks instead of adding more.

Good.... point

> Source/WTF/wtf/PlatformHave.h:1201
> +// FIXME: Does this intend to exclude macCatalyst?

Per Arne says this one should be OK to add catalyst
Comment 5 Tim Horton 2022-04-01 14:46:15 PDT
Created attachment 456405 [details]
Patch
Comment 6 Alexey Proskuryakov 2022-04-01 14:54:47 PDT
Comment on attachment 456405 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:465
>  #if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 140000
> +// FIXME: Does this intend to exclude macCatalyst?

This is always true, so the whole section can be deleted.

> Source/WTF/wtf/PlatformHave.h:746
> +#if (PLATFORM(IOS) || PLATFORM(MACCATALYST)) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000

I don't think that this OS version check is still relevant?
Comment 7 Alexey Proskuryakov 2022-04-01 15:20:15 PDT
Comment on attachment 456405 [details]
Patch

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

>> Source/WTF/wtf/PlatformHave.h:465
>> +// FIXME: Does this intend to exclude macCatalyst?
> 
> This is always true, so the whole section can be deleted.

Always false. Was thinking along the lines of "it's no longer broken" :)
Comment 8 Tim Horton 2022-04-01 15:22:52 PDT
Created attachment 456409 [details]
Patch
Comment 9 Tim Horton 2022-04-01 15:24:57 PDT
Created attachment 456410 [details]
Patch
Comment 10 Tim Horton 2022-04-01 15:26:22 PDT
Created attachment 456411 [details]
Patch
Comment 11 Tim Horton 2022-04-01 15:46:48 PDT
Created attachment 456412 [details]
Patch
Comment 12 EWS 2022-04-03 01:05:16 PDT
Committed r292275 (249173@main): <https://commits.webkit.org/249173@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456412 [details].
Comment 13 Radar WebKit Bug Importer 2022-04-03 01:06:24 PDT
<rdar://problem/91209620>