Bug 150778 – Add objC delegate callback for webProcessDidBecomeResponsive and webProcessDidBecomeUnresponsive.

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

[Yongjun Zhang]

It would be nice for a WebKit client to get notified if the web process becomes unresponsive or responsive. To that, we should consider to add delegate methods for webProcessDidBecomeResponsive and webProcessDidBecomeUnresponsive.

[Yongjun Zhang]

<rdar://problem/23288706>

[Yongjun Zhang]

Created attachment 264525 [details]
Patch.

[WebKit Commit Bot]

Attachment 264525 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/NavigationState.h:104:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/NavigationState.h:105:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yongjun Zhang]

Created attachment 264526 [details]
Fix style issues.

[Darin Adler]

Comment on attachment 264526 [details]
Fix style issues.

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

> Source/WebKit2/UIProcess/API/APINavigationClient.h:87
>      // FIXME: This function should not be part of this client.
>      virtual void processDidCrash(WebKit::WebPageProxy&) { }
> +    virtual void processDidBecomeResponsive(WebKit::WebPageProxy&) { }
> +    virtual void processDidBecomeUnresponsive(WebKit::WebPageProxy&) { }

If the comment applies to all three of these, then please reword it so it says “These functions”.

Why can’t WebKit2 implement the delegate by implementing the existing loader client function? Why do we have to add this to the navigation client?

> Source/WebKit2/UIProcess/Cocoa/NavigationState.h:105
> +        virtual void processDidBecomeResponsive(WebKit::WebPageProxy&) override;
> +        virtual void processDidBecomeUnresponsive(WebKit::WebPageProxy&) override;

Please, no WebKit:: prefix here for the argument types for these new functions (match the function just above).

> Source/WebKit2/UIProcess/Cocoa/NavigationState.h:188
> +        bool webViewWebProcessDidBecomeResponsive : 1;
> +        bool webViewWebProcessDidBecomeUnresponsive : 1;

Not sure what order these booleans are in. It’s strange to have the new ones at the bottom after the QuickLook ones. Maybe we should keep these alphabetical instead of randomly ordered?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4863
> -    m_loaderClient->processDidBecomeUnresponsive(*this);
> +    if (m_navigationClient)
> +        m_navigationClient->processDidBecomeUnresponsive(*this);
> +    else
> +        m_loaderClient->processDidBecomeUnresponsive(*this);

Skipping the loader client function just because we have a navigation client doesn’t seem logical. Seems we should call both.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4884
> -    m_loaderClient->processDidBecomeResponsive(*this);
> +    if (m_navigationClient)
> +        m_navigationClient->processDidBecomeResponsive(*this);
> +    else
> +        m_loaderClient->processDidBecomeResponsive(*this);

Ditto.

[Yongjun Zhang]

(In reply to comment #5)
> Comment on attachment 264526 [details]
> Fix style issues.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264526&action=review
> 
> > Source/WebKit2/UIProcess/API/APINavigationClient.h:87
> >      // FIXME: This function should not be part of this client.
> >      virtual void processDidCrash(WebKit::WebPageProxy&) { }
> > +    virtual void processDidBecomeResponsive(WebKit::WebPageProxy&) { }
> > +    virtual void processDidBecomeUnresponsive(WebKit::WebPageProxy&) { }
> 
> If the comment applies to all three of these, then please reword it so it
> says “These functions”.

I will change.
> 
> Why can’t WebKit2 implement the delegate by implementing the existing loader
> client function? Why do we have to add this to the navigation client?
>

Looks like WebKit2 does have loader client delegate, but it is only implemented as the deprecated WKBrowsingContextLoadDelegatePrivate, which is where browsingContextControllerWebProcessDidCrash: stays. This patch follows the way how _webViewWebProcessDidCrash and webViewWebContentProcessDidTerminate: are implemented, and they are currently staying in the negation client (WKNavigationDelegate).
 
> > Source/WebKit2/UIProcess/Cocoa/NavigationState.h:105
> > +        virtual void processDidBecomeResponsive(WebKit::WebPageProxy&) override;
> > +        virtual void processDidBecomeUnresponsive(WebKit::WebPageProxy&) override;
> 
> Please, no WebKit:: prefix here for the argument types for these new
> functions (match the function just above).

Fixed.

> 
> > Source/WebKit2/UIProcess/Cocoa/NavigationState.h:188
> > +        bool webViewWebProcessDidBecomeResponsive : 1;
> > +        bool webViewWebProcessDidBecomeUnresponsive : 1;
> 
> Not sure what order these booleans are in. It’s strange to have the new ones
> at the bottom after the QuickLook ones. Maybe we should keep these
> alphabetical instead of randomly ordered?

Yeah, the ordering seems to random - they are not alphabetical. I will move these two bools next to webViewWebProcessDidCrash since they belong to the same category (webProcess related).
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:4863
> > -    m_loaderClient->processDidBecomeUnresponsive(*this);
> > +    if (m_navigationClient)
> > +        m_navigationClient->processDidBecomeUnresponsive(*this);
> > +    else
> > +        m_loaderClient->processDidBecomeUnresponsive(*this);
> 
> Skipping the loader client function just because we have a navigation client
> doesn’t seem logical. Seems we should call both.

Again, this is following the pattern of WebPageProxy::processDidCrash(). I agree it is odd to skip the loader client. Will file another bug.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:4884
> > -    m_loaderClient->processDidBecomeResponsive(*this);
> > +    if (m_navigationClient)
> > +        m_navigationClient->processDidBecomeResponsive(*this);
> > +    else
> > +        m_loaderClient->processDidBecomeResponsive(*this);
> 
> Ditto.

[Yongjun Zhang]

Created attachment 264530 [details]
Address review comments for landing.

[Yongjun Zhang]

Created attachment 264531 [details]
Address review comments before landing.

[WebKit Commit Bot]

Comment on attachment 264531 [details]
Address review comments before landing.

Clearing flags on attachment: 264531

Committed r191895: <http://trac.webkit.org/changeset/191895>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.