<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add objC delegate callback for webProcessDidBecomeResponsive and webProcessDidBecomeUnresponsive."
href="https://bugs.webkit.org/show_bug.cgi?id=150778#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add objC delegate callback for webProcessDidBecomeResponsive and webProcessDidBecomeUnresponsive."
href="https://bugs.webkit.org/show_bug.cgi?id=150778">bug 150778</a>
from <span class="vcard"><a class="email" href="mailto:yongjun_zhang@apple.com" title="Yongjun Zhang <yongjun_zhang@apple.com>"> <span class="fn">Yongjun Zhang</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=150778#c5">comment #5</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=264526&action=diff" name="attach_264526" title="Fix style issues.">attachment 264526</a> <a href="attachment.cgi?id=264526&action=edit" title="Fix style issues.">[details]</a></span>
> Fix style issues.
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=264526&action=review">https://bugs.webkit.org/attachment.cgi?id=264526&action=review</a>
>
> > 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”.</span >
I will change.
<span class="quote">>
> 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?</span >
>
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).
<span class="quote">> > 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).</span >
Fixed.
<span class="quote">>
> > 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?</span >
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).
<span class="quote">>
> > 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.</span >
Again, this is following the pattern of WebPageProxy::processDidCrash(). I agree it is odd to skip the loader client. Will file another bug.
<span class="quote">>
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:4884
> > - m_loaderClient->processDidBecomeResponsive(*this);
> > + if (m_navigationClient)
> > + m_navigationClient->processDidBecomeResponsive(*this);
> > + else
> > + m_loaderClient->processDidBecomeResponsive(*this);
>
> Ditto.</span ></pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>