Bug 173251 – [GTK] getPage() in WebKitWebView should return a reference instead of a pointer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

The WebPageProxy is created on WebKitWebView construction, so getPage() can only return nullptr if it's called during construction. And that can only happen when construct properties are set. It's better to check that the view has been constructed only in these particular cases and make getPage() return a reference.

[Carlos Garcia Campos]

Created attachment 312645 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Committed r218097: <http://trac.webkit.org/changeset/218097>

[Michael Catanzaro]

Comment on attachment 312645 [details]
Patch

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

Nice!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:517
>  static void webkitWebViewDisconnectSettingsSignalHandlers(WebKitWebView* webView)
>  {
> +    if (!webkitWebViewIsConstructed(webView))
> +        return;

Can this really ever be legitimately called before the WebKitWebView is constructed? It seems like you should be able to use an ASSERT here instead of an early return.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 312645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312645&action=review
> 
> Nice!
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:517
> >  static void webkitWebViewDisconnectSettingsSignalHandlers(WebKitWebView* webView)
> >  {
> > +    if (!webkitWebViewIsConstructed(webView))
> > +        return;
> 
> Can this really ever be legitimately called before the WebKitWebView is
> constructed? It seems like you should be able to use an ASSERT here instead
> of an early return.

Yes, read the changelog:

"The WebPageProxy is created on WebKitWebView construction, so getPage() can only return nullptr if it's called during construction. And that can only happen when construct properties are set."

construct properties are set on GObjectClass::constructor() that happens before GObjectClass::constructed(). It's also explained in the code in several comments.

[Michael Catanzaro]

I don't see an explanation for why signals should be disconnected before the object is constructed. That is unusual.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #6)
> I don't see an explanation for why signals should be disconnected before the
> object is constructed. That is unusual.

Ah! I'm sorry I thought this was the condition for connecting the signals, not to disconnect them. This is indeed a trick. Of course it can't happen, but webkitWebViewDisconnectSettingsSignalHandlers is called unconditionally in dispose, which can happen multiple times, but in dispose loaderObserer is set to nullptr, making the webkitWebViewIsConstructed false again. So this is basically preventing the signals from being disconnected more than once on destruction.

[Michael Catanzaro]

OK!