Bug 153165 – Add a Modern WebKit analogue for WKPageIsWebProcessResponsive

• Component: New Bugs
• Status: NEW

[Geoffrey Garen]

Add a WebKit2 analogue for WKPageIsWebProcessResponsive

[Geoffrey Garen]

Created attachment 269121 [details]
Patch

[Tim Horton]

Comment on attachment 269121 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3630
> +- (void) _isWebProcessResponsive:(void (^)(bool isWebProcessResponsive))completionHandler

There should be no space after the (void). Should the argument be called webProcessIsResponsive? It's not a question.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3633
> +    if (completionHandler)

Is there any reason to do the work if there's no completion handler? Will anyone else care?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3637
> +        if (completionHandlerCopy) {

Early return?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:237
>  @property (nonatomic, readonly) BOOL _webProcessIsResponsive WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);
> +- (void) _isWebProcessResponsive:(void (^)(bool isWebProcessResponsive))completionHandler WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);

A bit confused. Why do we have both of these?

Also, ditto on the space.

[Geoffrey Garen]

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3630
> > +- (void) _isWebProcessResponsive:(void (^)(bool isWebProcessResponsive))completionHandler
> 
> There should be no space after the (void). Should the argument be called
> webProcessIsResponsive? It's not a question.

Fixed.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3633
> > +    if (completionHandler)
> 
> Is there any reason to do the work if there's no completion handler? Will
> anyone else care?

Technically, eagerly polling will also update KVO state, possibly letting you know that the process has hung when you didn't know before. WebKit uses this feature internally in exactly one place. Perhaps clients will want to do the same.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3637
> > +        if (completionHandlerCopy) {
> 
> Early return?

Fixed.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:237
> >  @property (nonatomic, readonly) BOOL _webProcessIsResponsive WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);
> > +- (void) _isWebProcessResponsive:(void (^)(bool isWebProcessResponsive))completionHandler WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);
> 
> A bit confused. Why do we have both of these?

The KVO property is lazy, and may be out of date if we have not sent any messages recently.

I've renamed the two to be similar. Maybe this is better.

In the end, I think we need both features, since it is not scalable to call the polling mechanism all the time and the KVO mechanism is not always up to date.

> 
> Also, ditto on the space.

Fixed.

[Geoffrey Garen]

Created attachment 269128 [details]
Patch

Comment on attachment 269128 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        (-[WKWebView _webProcessIsResponsive:]): Manually copy this block since
> +        we are transitioning from ARC to C++.

We don’t use ARC in WebKit, and whether the caller uses ARC is irrelevant, so I think you meant to say something other than “ARC” here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3630
> +- (void)_webProcessIsResponsive:(void (^)(bool webProcessIsResponsive))completionHandler

Use BOOL instead of bool.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3634
> +    void (^completionHandlerCopy)(bool) = nil;
> +    if (completionHandler)
> +        completionHandlerCopy = Block_copy(completionHandler);

Use BlockPtr:
auto completionHandlerCopy = makeBlockPtr(completionHandler);

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:237
> +-(void) _webProcessIsResponsive:(void (^)(bool webProcessIsResponsive))completionHandler WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);

Space should go before (void), not after. Use BOOL instead of bool.

[Anders Carlsson]

FWIW we now have a BlockPtr class that can be used instead of manually managing the block object lifetime.

[Geoffrey Garen]

Created attachment 269292 [details]
Patch

[Stefan Arentz]

Is there a change this will become a public API? This would be very useful to have for general usage in third-party apps that use WKWebViews. Firefox for iOS would most certainly make use of it.