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?

Jonathan Bedard
Reported 2020-01-23 17:37:35 PST
We should be checking PLATFORM(WATCHOS) and PLATFORM(APPLETV) instead of comparing against iOS 11.
Attachments
Patch (2.03 KB, patch)
2020-01-23 17:41 PST, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (2.31 KB, patch)
2020-01-24 08:27 PST, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (2.32 KB, patch)
2020-01-27 08:18 PST, Jonathan Bedard 		jbedard: review?
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Jonathan Bedard
Comment 1 2020-01-23 17:41:54 PST
Created attachment 388628 [details] Patch
Alexey Proskuryakov
Comment 2 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.
Jonathan Bedard
Comment 3 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.
Jonathan Bedard
Comment 4 2020-01-24 08:27:40 PST
Created attachment 388696 [details] Patch
Darin Adler
Comment 5 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?
Jonathan Bedard
Comment 6 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.
Jonathan Bedard
Comment 7 2020-01-27 08:18:14 PST
Created attachment 388861 [details] Patch
Alexey Proskuryakov
Comment 8 2021-03-05 19:50:02 PST
Is this still needed?
Darin Adler
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.