[Webkit-unassigned] [Bug 150778] Add objC delegate callback for webProcessDidBecomeResponsive and webProcessDidBecomeUnresponsive.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150778

--- Comment #6 from Yongjun Zhang <yongjun_zhang at apple.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151101/5dffb987/attachment.html>