70594 2011-10-21 03:34:22 -0700 [GTK] Add helper function to set the loader client in WebKitWebView 2011-10-24 08:16:35 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) PC Linux RESOLVED FIXED Gtk P2 Normal --- 1 cgarcia webkit-unassigned pnormand xan.lopez oldest_to_newest 488292 0 cgarcia 2011-10-21 03:34:22 -0700 It's set in two places, the constructed method and the public method set_loader_client(). In the constructor we don't need to check that view is actually a WebView and loader client is a LoaderClient, and that current client is not the same. 488295 1 111934 cgarcia 2011-10-21 03:38:12 -0700 Created attachment 111934 Patch 488358 2 111934 mrobinson 2011-10-21 07:01:35 -0700 Comment on attachment 111934 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111934&action=review > Source/WebKit2/ChangeLog:5 > + [GTK] Add helper function to set the loader client in WebKitWebView > + https://bugs.webkit.org/show_bug.cgi?id=70594 > + These checks are incredibly cheap. Is there some other problem? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:68 > - > - webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0); > + WebPageProxy* page = webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0); > It seems like instead of doing this manually we should be chaining up to the parent constructor. It's mentioned here that you should always chain up to the parent constructor: http://developer.gnome.org/gobject/stable/chapter-gobject.html#gobject-instantiation 488394 3 cgarcia 2011-10-21 08:12:50 -0700 (In reply to comment #2) > (From update of attachment 111934 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111934&action=review > > > Source/WebKit2/ChangeLog:5 > > + [GTK] Add helper function to set the loader client in WebKitWebView > > + https://bugs.webkit.org/show_bug.cgi?id=70594 > > + > > These checks are incredibly cheap. Is there some other problem? g_return macros are not so cheap. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:68 > > - > > - webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0); > > + WebPageProxy* page = webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0); > > > > It seems like instead of doing this manually we should be chaining up to the parent constructor. It's mentioned here that you should always chain up to the parent constructor: http://developer.gnome.org/gobject/stable/chapter-gobject.html#gobject-instantiation This method is constructed, not constructor. 488432 4 111934 mrobinson 2011-10-21 09:21:05 -0700 Comment on attachment 111934 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111934&action=review Okay. Looks fine, but I have a minor change requested below. Thank for cleaning this up. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:60 > + webView->priv->loaderClient = loaderClient; > + webkitWebLoaderClientAttachLoaderClientToPage(loaderClient, wkPage); Instead of passing the page as an argument, I think it'd be better to get it here with: WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView)); >>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:68 >>> >> >> It seems like instead of doing this manually we should be chaining up to the parent constructor. It's mentioned here that you should always chain up to the parent constructor: http://developer.gnome.org/gobject/stable/chapter-gobject.html#gobject-instantiation > > This method is constructed, not constructor. Still the docs suggest chaining up: "the constructed function is called by g_object_new() as the final step of the object creation process. At the point of the call, all construction properties have been set on the object. The purpose of this call is to allow for object initialisation steps that can only be performed after construction properties have been set. constructed implementors should chain up to the constructed call of their parent class to allow it to complete its initialisation." > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:439 > -void webkitWebViewBaseCreateWebPage(WebKitWebViewBase* webkitWebViewBase, WKContextRef context, WKPageGroupRef pageGroup) > +WebPageProxy* webkitWebViewBaseCreateWebPage(WebKitWebViewBase* webkitWebViewBase, WKContextRef context, WKPageGroupRef pageGroup) > { > WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv; > I dislike this change because we ignore the return value in WebKitWebViewBase. Do you mind just getting the Page in WebKitWebViewSetLoader client? 489336 5 cgarcia 2011-10-24 08:16:35 -0700 Committed r98241: <http://trac.webkit.org/changeset/98241> 111934 2011-10-21 03:38:12 -0700 2011-10-21 09:21:05 -0700 Patch wk2-set-loader-client-helper.diff text/plain 4787 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA4ZDA3MzFjLi45YTYwNmVmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjQg QEAKIDIwMTEtMTAtMjEgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgogCisgICAgICAgIFtHVEtdIEFkZCBoZWxwZXIgZnVuY3Rpb24gdG8gc2V0IHRoZSBsb2FkZXIg Y2xpZW50IGluIFdlYktpdFdlYlZpZXcKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTcwNTk0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRXZWJWaWV3LmNwcDoKKyAg ICAgICAgKHdlYmtpdFdlYlZpZXdTZXRMb2FkZXJDbGllbnQpOiBIZWxwZXIgZnVuY3Rpb24gdG8g c2V0IHRoZSBsb2FkZXIKKyAgICAgICAgY2xpZW50LgorICAgICAgICAod2Via2l0V2ViVmlld0Nv bnN0cnVjdGVkKTogVXNlIHdlYmtpdFdlYlZpZXdTZXRMb2FkZXJDbGllbnQoKSB0bworICAgICAg ICBhdm9pZCB1bm5lY2Vzc2FyeSBjaGVja3MuCisgICAgICAgICh3ZWJraXRfd2ViX3ZpZXdfc2V0 X2xvYWRlcl9jbGllbnQpOiBVc2UKKyAgICAgICAgd2Via2l0V2ViVmlld1NldExvYWRlckNsaWVu dCgpLgorICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlLmNwcDoK KyAgICAgICAgKHdlYmtpdFdlYlZpZXdCYXNlQ3JlYXRlV2ViUGFnZSk6IFJldHVybiB0aGUgbmV3 bHkgY3JlYXRlZCBwYWdlIHRvCisgICAgICAgIGF2b2lkIGhhdmluZyB0byBjYWxsIEdldFBhZ2Uo KSBhZnRlciBjcmVhdGluZyBpdC4KKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRX ZWJWaWV3QmFzZVByaXZhdGUuaDoKKworMjAxMS0xMC0yMSAgQ2FybG9zIEdhcmNpYSBDYW1wb3Mg IDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKICAgICAgICAgVW5yZXZpZXdlZC4gRml4IFdlYktpdDIg R1RLKyBidWlsZCBhZnRlciByOTgwODEuCiAKICAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9X ZWJLaXRXZWJWaWV3Lmg6CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJ L2d0ay9XZWJLaXRXZWJWaWV3LmNwcCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3Rr L1dlYktpdFdlYlZpZXcuY3BwCmluZGV4IDIyYjA3NTMuLjI2YzliNmIgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXcuY3BwCisrKyBiL1Nv dXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXcuY3BwCkBAIC01NCwx NSArNTQsMjAgQEAgc3RydWN0IF9XZWJLaXRXZWJWaWV3UHJpdmF0ZSB7CiAKIEdfREVGSU5FX1RZ UEUoV2ViS2l0V2ViVmlldywgd2Via2l0X3dlYl92aWV3LCBXRUJLSVRfVFlQRV9XRUJfVklFV19C QVNFKQogCitzdGF0aWMgdm9pZCB3ZWJraXRXZWJWaWV3U2V0TG9hZGVyQ2xpZW50KFdlYktpdFdl YlZpZXcqIHdlYlZpZXcsIFdlYktpdFdlYkxvYWRlckNsaWVudCogbG9hZGVyQ2xpZW50LCBXS1Bh Z2VSZWYgd2tQYWdlKQoreworICAgIHdlYlZpZXctPnByaXYtPmxvYWRlckNsaWVudCA9IGxvYWRl ckNsaWVudDsKKyAgICB3ZWJraXRXZWJMb2FkZXJDbGllbnRBdHRhY2hMb2FkZXJDbGllbnRUb1Bh Z2UobG9hZGVyQ2xpZW50LCB3a1BhZ2UpOworfQorCiBzdGF0aWMgdm9pZCB3ZWJraXRXZWJWaWV3 Q29uc3RydWN0ZWQoR09iamVjdCogb2JqZWN0KQogewogICAgIFdlYktpdFdlYlZpZXcqIHdlYlZp ZXcgPSBXRUJLSVRfV0VCX1ZJRVcob2JqZWN0KTsKICAgICBXZWJLaXRXZWJWaWV3UHJpdmF0ZSog cHJpdiA9IHdlYlZpZXctPnByaXY7Ci0KLSAgICB3ZWJraXRXZWJWaWV3QmFzZUNyZWF0ZVdlYlBh Z2UoV0VCS0lUX1dFQl9WSUVXX0JBU0Uod2ViVmlldyksIHdlYmtpdFdlYkNvbnRleHRHZXRXS0Nv bnRleHQocHJpdi0+Y29udGV4dCksIDApOworICAgIFdlYlBhZ2VQcm94eSogcGFnZSA9IHdlYmtp dFdlYlZpZXdCYXNlQ3JlYXRlV2ViUGFnZShXRUJLSVRfV0VCX1ZJRVdfQkFTRSh3ZWJWaWV3KSwg d2Via2l0V2ViQ29udGV4dEdldFdLQ29udGV4dChwcml2LT5jb250ZXh0KSwgMCk7CiAKICAgICBz dGF0aWMgR1JlZlB0cjxXZWJLaXRXZWJMb2FkZXJDbGllbnQ+IGRlZmF1bHRMb2FkZXJDbGllbnQg PSBhZG9wdEdSZWYoV0VCS0lUX1dFQl9MT0FERVJfQ0xJRU5UKGdfb2JqZWN0X25ldyhXRUJLSVRf VFlQRV9XRUJfTE9BREVSX0NMSUVOVCwgTlVMTCkpKTsKLSAgICB3ZWJraXRfd2ViX3ZpZXdfc2V0 X2xvYWRlcl9jbGllbnQod2ViVmlldywgZGVmYXVsdExvYWRlckNsaWVudC5nZXQoKSk7CisgICAg d2Via2l0V2ViVmlld1NldExvYWRlckNsaWVudCh3ZWJWaWV3LCBkZWZhdWx0TG9hZGVyQ2xpZW50 LmdldCgpLCB0b0FQSShwYWdlKSk7CiB9CiAKIHN0YXRpYyB2b2lkIHdlYmtpdFdlYlZpZXdTZXRQ cm9wZXJ0eShHT2JqZWN0KiBvYmplY3QsIGd1aW50IHByb3BJZCwgY29uc3QgR1ZhbHVlKiB2YWx1 ZSwgR1BhcmFtU3BlYyogcGFyYW1TcGVjKQpAQCAtMjM1LDggKzI0MCw3IEBAIHZvaWQgd2Via2l0 X3dlYl92aWV3X3NldF9sb2FkZXJfY2xpZW50KFdlYktpdFdlYlZpZXcqIHdlYlZpZXcsIFdlYktp dFdlYkxvYWRlckNsCiAgICAgICAgIHJldHVybjsKIAogICAgIFdlYlBhZ2VQcm94eSogcGFnZSA9 IHdlYmtpdFdlYlZpZXdCYXNlR2V0UGFnZShXRUJLSVRfV0VCX1ZJRVdfQkFTRSh3ZWJWaWV3KSk7 Ci0gICAgd2Via2l0V2ViTG9hZGVyQ2xpZW50QXR0YWNoTG9hZGVyQ2xpZW50VG9QYWdlKGxvYWRl ckNsaWVudCwgdG9BUEkocGFnZSkpOwotICAgIHByaXYtPmxvYWRlckNsaWVudCA9IGxvYWRlckNs aWVudDsKKyAgICB3ZWJraXRXZWJWaWV3U2V0TG9hZGVyQ2xpZW50KHdlYlZpZXcsIGxvYWRlckNs aWVudCwgdG9BUEkocGFnZSkpOwogfQogCiAvKioKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlLmNwcCBiL1NvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlLmNwcAppbmRleCA2ZGRlNjIzLi4w MzNlMWU3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJL aXRXZWJWaWV3QmFzZS5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9ndGsv V2ViS2l0V2ViVmlld0Jhc2UuY3BwCkBAIC00MzMsMTIgKzQzMywxMyBAQCBXZWJQYWdlUHJveHkq IHdlYmtpdFdlYlZpZXdCYXNlR2V0UGFnZShXZWJLaXRXZWJWaWV3QmFzZSogd2Via2l0V2ViVmll d0Jhc2UpCiAgICAgcmV0dXJuIHdlYmtpdFdlYlZpZXdCYXNlLT5wcml2LT5wYWdlUHJveHkuZ2V0 KCk7CiB9CiAKLXZvaWQgd2Via2l0V2ViVmlld0Jhc2VDcmVhdGVXZWJQYWdlKFdlYktpdFdlYlZp ZXdCYXNlKiB3ZWJraXRXZWJWaWV3QmFzZSwgV0tDb250ZXh0UmVmIGNvbnRleHQsIFdLUGFnZUdy b3VwUmVmIHBhZ2VHcm91cCkKK1dlYlBhZ2VQcm94eSogd2Via2l0V2ViVmlld0Jhc2VDcmVhdGVX ZWJQYWdlKFdlYktpdFdlYlZpZXdCYXNlKiB3ZWJraXRXZWJWaWV3QmFzZSwgV0tDb250ZXh0UmVm IGNvbnRleHQsIFdLUGFnZUdyb3VwUmVmIHBhZ2VHcm91cCkKIHsKICAgICBXZWJLaXRXZWJWaWV3 QmFzZVByaXZhdGUqIHByaXYgPSB3ZWJraXRXZWJWaWV3QmFzZS0+cHJpdjsKIAogICAgIHByaXYt PnBhZ2VQcm94eSA9IHRvSW1wbChjb250ZXh0KS0+Y3JlYXRlV2ViUGFnZShwcml2LT5wYWdlQ2xp ZW50LmdldCgpLCB0b0ltcGwocGFnZUdyb3VwKSk7CiAgICAgcHJpdi0+cGFnZVByb3h5LT5pbml0 aWFsaXplV2ViUGFnZSgpOworICAgIHJldHVybiBwcml2LT5wYWdlUHJveHkuZ2V0KCk7CiB9CiAK IHZvaWQgd2Via2l0V2ViVmlld0Jhc2VTZXRUb29sdGlwVGV4dChXZWJLaXRXZWJWaWV3QmFzZSog d2ViVmlld0Jhc2UsIGNvbnN0IGNoYXIqIHRvb2x0aXApCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi S2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRXZWJWaWV3QmFzZVByaXZhdGUuaCBiL1NvdXJj ZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlUHJpdmF0ZS5oCmlu ZGV4IDNhOWE0MGIuLjlkODJmMGMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vz cy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlUHJpdmF0ZS5oCisrKyBiL1NvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlUHJpdmF0ZS5oCkBAIC00Miw3ICs0 Miw3IEBAIEd0a0lNQ29udGV4dCogd2Via2l0V2ViVmlld0Jhc2VHZXRJTUNvbnRleHQoV2ViS2l0 V2ViVmlld0Jhc2UqKTsKIAogV2ViUGFnZVByb3h5KiB3ZWJraXRXZWJWaWV3QmFzZUdldFBhZ2Uo V2ViS2l0V2ViVmlld0Jhc2UqKTsKIAotdm9pZCB3ZWJraXRXZWJWaWV3QmFzZUNyZWF0ZVdlYlBh Z2UoV2ViS2l0V2ViVmlld0Jhc2UqLCBXS0NvbnRleHRSZWYsIFdLUGFnZUdyb3VwUmVmKTsKK1dl YlBhZ2VQcm94eSogd2Via2l0V2ViVmlld0Jhc2VDcmVhdGVXZWJQYWdlKFdlYktpdFdlYlZpZXdC YXNlKiwgV0tDb250ZXh0UmVmLCBXS1BhZ2VHcm91cFJlZik7CiAKIHZvaWQgd2Via2l0V2ViVmll d0Jhc2VTZXRUb29sdGlwVGV4dChXZWJLaXRXZWJWaWV3QmFzZSosIGNvbnN0IGNoYXIqKTsKIAo=