Bug 209686 – [iOS] Delay process suspension for a while after loading an app link

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Delay process suspension for a while after loading an app link. This will allow the page's script to pass information more reliably to the native app handling the navigation.

[Chris Dumez]

<rdar://problem/60888891>

[Chris Dumez]

Created attachment 394765 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

(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 :)

[Chris Dumez]

Created attachment 394768 [details]
Patch

[EWS]

Committed r259146: <https://trac.webkit.org/changeset/259146>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394768 [details].

[Darin Adler]

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?

[Chris Dumez]

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

[Chris Dumez]

Reopening for follow-up.

[Chris Dumez]

Created attachment 394783 [details]
Follow-up

[EWS]

Committed r259166: <https://trac.webkit.org/changeset/259166>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394783 [details].

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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>