[webkit-reviews] review denied: [Bug 216655] Enable ITP in WKWebViews for apps with the full browser entitlement who are not linked to iOS 14.0 : [Attachment 409051] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied katherine_cheney at apple.com's
request for review:
Bug 216655: Enable ITP in WKWebViews for apps with the full browser entitlement
who are not linked to iOS 14.0
https://bugs.webkit.org/show_bug.cgi?id=216655

Attachment 409051: Patch

https://bugs.webkit.org/attachment.cgi?id=409051&action=review




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 409051
  --> https://bugs.webkit.org/attachment.cgi?id=409051
Patch

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

I don't think this is quite right (see inline comments)

> Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:109
> +    if (isFullWebBrowser(bundleIdentifier) && !appWasLinkedOnOrAfter)

Instead of this new test, I think we should change the existing linkedOnOrAfter
check to only bail out if the default browser entitlement was NOT used.

if (!appWasLinkedOnOrAfter && !isFullWebBrowser(bundleIdentifier))
    return false;

This way, we fall through and use the User's choice about ITP state for a
default browser, even if it was built against the older SDK.

> Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:246
> +    return appWasLinkedOnOrAfter &&
isFullWebBrowser(WebCore::applicationBundleIdentifier());

After thinking about it, I don't think we need this check. We don't care if the
app was linked against an older SDK. We just want to make sure that default
browsers honor the user's ITP setting which you handle in your change to
'determineITPStateInternal'.