Bug 173251 - [GTK] getPage() in WebKitWebView should return a reference instead of a pointer
Summary: [GTK] getPage() in WebKitWebView should return a reference instead of a pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 173254
  Show dependency treegraph
 
Reported: 2017-06-12 02:50 PDT by Carlos Garcia Campos
Modified: 2017-06-13 08:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (28.23 KB, patch)
2017-06-12 02:54 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-06-12 02:50:33 PDT
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.
Comment 1 Carlos Garcia Campos 2017-06-12 02:54:55 PDT
Created attachment 312645 [details]
Patch
Comment 2 Build Bot 2017-06-12 02:56:32 PDT
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
Comment 3 Carlos Garcia Campos 2017-06-12 05:55:05 PDT
Committed r218097: <http://trac.webkit.org/changeset/218097>
Comment 4 Michael Catanzaro 2017-06-12 05:59:17 PDT
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.
Comment 5 Carlos Garcia Campos 2017-06-12 06:04:05 PDT
(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.
Comment 6 Michael Catanzaro 2017-06-12 11:06:32 PDT
I don't see an explanation for why signals should be disconnected before the object is constructed. That is unusual.
Comment 7 Carlos Garcia Campos 2017-06-12 22:34:04 PDT
(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.
Comment 8 Michael Catanzaro 2017-06-13 08:39:58 PDT
OK!