Bug 206722

Summary: WebCore: Remove iOS 11 macros from WebContentReaderCocoa.mm
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: WebCore Misc.Assignee: Jonathan Bedard <jbedard>
Status: NEW ---    
Severity: Normal CC: ap, darin, ews-watchlist, krollin, mifenton, simon.fraser, thorton, wenson_hsieh
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch jbedard: review?

Description Jonathan Bedard 2020-01-23 17:37:35 PST 
We should be checking PLATFORM(WATCHOS) and PLATFORM(APPLETV) instead of comparing against iOS 11.
Comment 1 Jonathan Bedard 2020-01-23 17:41:54 PST 
Created attachment 388628 [details]
Patch
Comment 2 Alexey Proskuryakov 2020-01-23 20:40:47 PST 
Comment on attachment 388628 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No functional changes, covered by existing tests.

This is super unlikely to be correct. I expect the new code path to be applicable everywhere. Maybe we should go straight to correct behavior here.
Comment 3 Jonathan Bedard 2020-01-24 08:22:38 PST 
Comment on attachment 388628 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No functional changes, covered by existing tests.
> 
> This is super unlikely to be correct. I expect the new code path to be applicable everywhere. Maybe we should go straight to correct behavior here.

According to https://trac.webkit.org/changeset/222239/webkit, that was just a stub implementation for old OSes, so this isn't a PLATFORM macro hiding as a version check, it's a genuine version check. I'll post a patch to remove it.
Comment 4 Jonathan Bedard 2020-01-24 08:27:40 PST 
Created attachment 388696 [details]
Patch
Comment 5 Darin Adler 2020-01-26 14:29:52 PST 
Comment on attachment 388696 [details]
Patch

Remind me about the situation on watchOS and tvOS again? Do they both have __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000? If so, then an obvious review+ from me. Or maybe this file is disabled entirely on those platforms? If neither of those is the case then this is a behavior change on those platforms?
Comment 6 Jonathan Bedard 2020-01-27 08:05:05 PST 
(In reply to Darin Adler from comment #5)
> Comment on attachment 388696 [details]
> Patch
> 
> Remind me about the situation on watchOS and tvOS again? Do they both have
> __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000? If so, then an obvious review+
> from me. Or maybe this file is disabled entirely on those platforms? If
> neither of those is the case then this is a behavior change on those
> platforms?

I suppose I need to fix my changelog here, because I was attempting to address Alexey's comment:

"This is super unlikely to be correct. I expect the new code path to be applicable everywhere."

when my original patch retained behavior. Looking at the code change which added the version checks in the first place, it seems like they were genuinely intended as version checks and that we do want this code running on the latest watchOS and tvOS versions.
Comment 7 Jonathan Bedard 2020-01-27 08:18:14 PST 
Created attachment 388861 [details]
Patch
Comment 8 Alexey Proskuryakov 2021-03-05 19:50:02 PST 
Is this still needed?
Comment 9 Darin Adler 2021-03-08 14:20:33 PST 
Someone landed a different version of this, where we changed it to:

    #if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

There is some follow-up still needed as expressed by this FIXME:

    // FIXME: Do we really need to keep the legacy code path around for watchOS and tvOS?

But we don’t necessarily need a bug to track that.