Bug 206722 – WebCore: Remove iOS 11 macros from WebContentReaderCocoa.mm

• Component: WebCore Misc.
• Status: NEW

[Jonathan Bedard]

We should be checking PLATFORM(WATCHOS) and PLATFORM(APPLETV) instead of comparing against iOS 11.

[Jonathan Bedard]

Created attachment 388628 [details]
Patch

[Alexey Proskuryakov]

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.

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 388696 [details]
Patch

[Darin Adler]

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?

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Created attachment 388861 [details]
Patch

[Alexey Proskuryakov]

Is this still needed?

[Darin Adler]

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.