Bug 209686

<rdar://problem/60888891>

Created attachment 394765 [details] Patch

Comment on attachment 394765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394765&action=review > Source/WebKit/ChangeLog:13 > + This patch adds a [WKWebView _willOpenAppLink] SPI that the client needs to call before opening the > + app link. Why SPI instead of API? > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1550 > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(25 * NSEC_PER_SEC)), dispatch_get_main_queue(), [weakThis = makeWeakPtr(*this)] { Seems like typecasting 25 seconds to int64_t *after* doing the multiplication is too late to avoid overflow. I understand know why a typecast is needed at all, but if one is needed, I’d think we’d need to cast before doing the math. The comment should explain where the 25 second duration comes from, its rationale, so someone doesn’t just change it randomly later without understanding how we chose 25 seconds.

(In reply to Darin Adler from comment #3) > Comment on attachment 394765 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394765&action=review > > > Source/WebKit/ChangeLog:13 > > + This patch adds a [WKWebView _willOpenAppLink] SPI that the client needs to call before opening the > > + app link. > > Why SPI instead of API? For now, I am merely trying to address the issue in MobileSafari and SVC so an SPI is sufficient. I am not aware of this being requested by app developers. If it ever does, we can think about it harder and provide some kind of SPI. > > > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1550 > > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(25 * NSEC_PER_SEC)), dispatch_get_main_queue(), [weakThis = makeWeakPtr(*this)] { > > Seems like typecasting 25 seconds to int64_t *after* doing the > multiplication is too late to avoid overflow. I understand know why a > typecast is needed at all, but if one is needed, I’d think we’d need to cast > before doing the math. > > The comment should explain where the 25 second duration comes from, its > rationale, so someone doesn’t just change it randomly later without > understanding how we chose 25 seconds. I will check. I copy / pasted from existing code :)

Created attachment 394768 [details] Patch

Committed r259146: <https://trac.webkit.org/changeset/259146> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394768 [details].

Comment on attachment 394765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394765&action=review >>> Source/WebKit/ChangeLog:13 >>> + app link. >> >> Why SPI instead of API? > > For now, I am merely trying to address the issue in MobileSafari and SVC so an SPI is sufficient. I am not aware of this being requested by app developers. If it ever does, we can think about it harder and provide some kind of SPI. Not needed by other iOS browsers?

(In reply to Darin Adler from comment #7) > Comment on attachment 394765 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394765&action=review > > >>> Source/WebKit/ChangeLog:13 > >>> + app link. > >> > >> Why SPI instead of API? > > > > For now, I am merely trying to address the issue in MobileSafari and SVC so an SPI is sufficient. I am not aware of this being requested by app developers. If it ever does, we can think about it harder and provide some kind of SPI. > > Not needed by other iOS browsers? Looks like in other apps than Safari, WebKit takes care of opening AppLinks in tryInterceptNavigation(), in NavigationState.mm. I will update that method in a follow-up.

Reopening for follow-up.

Created attachment 394783 [details] Follow-up

Committed r259166: <https://trac.webkit.org/changeset/259166> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394783 [details].

Comment on attachment 394783 [details] Follow-up View in context: https://bugs.webkit.org/attachment.cgi?id=394783&action=review > Source/WebKit/UIProcess/Cocoa/NavigationState.mm:511 > + if (success && weakPage) > + weakPage->willOpenAppLink(); This breaking the build on newer versions of Mac. Needs to be inside #if PLATFORM(IOS_FAMILY) because the code in WebPageProxy.h is inside that.

Comment on attachment 394783 [details] Follow-up View in context: https://bugs.webkit.org/attachment.cgi?id=394783&action=review >> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:511 >> + weakPage->willOpenAppLink(); > > This breaking the build on newer versions of Mac. Needs to be inside #if PLATFORM(IOS_FAMILY) because the code in WebPageProxy.h is inside that. Fixed in commit r259177: <https://trac.webkit.org/changeset/259177/webkit> Fixed my missing ChangeLog entry in r259178: <https://trac.webkit.org/changeset/259178/webkit>