Summary: | [GTK] getPage() in WebKitWebView should return a reference instead of a pointer | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | berto, bugs-noreply, buildbot, gustavo, mcatanzaro, zan | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 173254 | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2017-06-12 02:50:33 PDT
Created attachment 312645 [details]
Patch
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 Committed r218097: <http://trac.webkit.org/changeset/218097> 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. (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. I don't see an explanation for why signals should be disconnected before the object is constructed. That is unusual. (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. OK! |