220620 2021-01-14 02:43:27 -0800 [GTK] webkit_settings_set_enable_plugins deprecation warning breaks unit tests 2021-02-03 15:17:43 -0800 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build PC Linux RESOLVED FIXED Gtk P3 Normal --- 1 mike mcatanzaro berto bugs-noreply cgarcia ews-watchlist gustavo mcatanzaro oldest_to_newest 1719564 0 mike 2021-01-14 02:43:27 -0800 Geary unit tests are currently failing on Fedora because the warning printed about calling `webkit_settings_set_enable_plugins` (Geary sets it to false) is presumably using g_warning(), causing the test harness to bail out: > Bail out! FATAL-WARNING: webkit_settings_set_enable_plugins is deprecated and does nothing. Plugins are no longer supported. Please don't do this. There's no way I can keep that disabled for older versions and not get the warning with new versions without resorting to hasty hacks like preprocessor conditionals. 1720004 1 mcatanzaro 2021-01-15 14:02:09 -0800 There's also a warning for webkit_settings_set_enable_private_browsing(), but that one looks safe to keep since it's been quite a long time since it was deprecated, and using it surely indicates an application bug. 1720009 2 417735 mcatanzaro 2021-01-15 14:08:08 -0800 Created attachment 417735 Patch 1720012 3 ews-watchlist 2021-01-15 14:09:06 -0800 Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API 1720278 4 cgarcia 2021-01-18 00:32:16 -0800 You can temporary change the log fatal flag before calling webkit_settings_set_enable_plugins() and restore it after the call. That's what we do in WebKit tests too, using g_log_set_always_fatal(). 1720302 5 mike 2021-01-18 02:14:01 -0800 > You can temporary change the log fatal flag before calling webkit_settings_set_enable_plugins() and restore it after the call. Sure, but this call is made by an abstract base class that gets used in a lot of different places and hence different tests, so either it has to get disabled for all tests (what's the point having it then?) or it's disabled for a bunch of seemingly unrelated tests (more random boilerplate), and still requires a code fix to unbreak CI. So I reckon it's much better that libraries just not break existing, non-problematic code in the first place. Isn't there a convention like defining `GTK_DISABLE_DEPRECATED` for people who are interested in that? 1720304 6 cgarcia 2021-01-18 02:22:59 -0800 (In reply to Michael Gratton from comment #5) > > You can temporary change the log fatal flag before calling webkit_settings_set_enable_plugins() and restore it after the call. > > Sure, but this call is made by an abstract base class that gets used in a > lot of different places and hence different tests, so either it has to get > disabled for all tests (what's the point having it then?) or it's disabled > for a bunch of seemingly unrelated tests (more random boilerplate), and > still requires a code fix to unbreak CI. I see. > So I reckon it's much better that libraries just not break existing, > non-problematic code in the first place. Sure, a g_warning is supposed to be harmless, tough, but it's true it's problematic for unit tests. So, maybe we can just turn that into a normal printf. > Isn't there a convention like defining `GTK_DISABLE_DEPRECATED` for people > who are interested in that? We don't print a warning for every deprecated method, only when they become noop. 1720340 7 mcatanzaro 2021-01-18 07:15:25 -0800 (In reply to Michael Gratton from comment #5) > Isn't there a convention like defining `GTK_DISABLE_DEPRECATED` for people > who are interested in that? G_ENABLE_DIAGNOSTIC controls runtime deprecation warnings for properties and signals, so it seemed like a good fit for this to me.... 1720420 8 mike 2021-01-18 15:15:06 -0800 Yeah the patch as-is sounds like decent practice in general. 1720484 9 cgarcia 2021-01-19 00:59:39 -0800 Well, this is not a "normal" deprecation warning saying that the functionality will be removed in a future version, it has already been removed, and it's important to let applications know it. I don't think it's common to run applications with G_ENABLE_DIAGNOSTIC=1, what makes the warning useless. Why not just making the call conditional to the WebKit version in Geary? 1724947 10 mcatanzaro 2021-02-02 15:59:24 -0800 The argument for not making it conditional in Geary is: (In reply to Michael Gratton from comment #5) > Sure, but this call is made by an abstract base class that gets used in a > lot of different places and hence different tests, so either it has to get > disabled for all tests (what's the point having it then?) or it's disabled > for a bunch of seemingly unrelated tests (more random boilerplate), and > still requires a code fix to unbreak CI. I don't think it's important to let applications know the functionality has been removed if they are trying to *disable* plugins. That's why the proposed patch still warns only if applications are trying to *enable* plugins. 1725061 11 417735 cgarcia 2021-02-03 01:33:32 -0800 Comment on attachment 417735 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417735&action=review > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1910 > + if (enabled || !g_strcmp0(g_getenv("G_ENABLE_DIAGNOSTIC"), "1")) Ok, but let's remove the G_ENABLE_DIAGNOSTIC check. 1725340 12 419187 mcatanzaro 2021-02-03 14:43:05 -0800 Created attachment 419187 Patch for landing 1725359 13 ews-feeder 2021-02-03 15:17:40 -0800 Committed r272342: <https://trac.webkit.org/changeset/272342> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419187. 417735 2021-01-15 14:08:08 -0800 2021-02-03 14:43:02 -0800 Patch bug-220620-20210115160807.patch text/plain 2225 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjcxNTI2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGE5YWZiNzIxNDJlOWZhZmM0 ODJhZTY4NmE4OGExZDkzNjJlN2E0ZmQuLmU3ODA5Y2Y3NTJmZDYwZWQ2NmQxMjVhZWY1YjM3YTg5 ZWYxNTlhOWEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMjEtMDEtMTUgIE1pY2hhZWwg Q2F0YW56YXJvICA8bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgW0dUS10gd2Via2l0 X3NldHRpbmdzX3NldF9lbmFibGVfcGx1Z2lucyBkZXByZWNhdGlvbiB3YXJuaW5nIGJyZWFrcyB1 bml0IHRlc3RzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0yMjA2MjAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBDaGVjayBpZiBHX0VOQUJMRV9ESUFHTk9TVElDIGlzIHNldCBiZWZvcmUgcHJpbnRpbmcgYSB3 YXJuaW5nIHdoZW4gY2FsbGluZworICAgICAgICB3ZWJraXRfc2V0dGluZ3Nfc2V0X2VuYWJsZV9w bHVnaW5zKCkgdG8gZGlzYWJsZSBwbHVnaW5zLiBXZSdsbCBzdGlsbCBwcmludCBhIHdhcm5pbmcg aWYKKyAgICAgICAgYXR0ZW1wdGluZyB0byBlbmFibGUgcGx1Z2lucywgb3IgaWYgY2hlY2tpbmcg d2hldGhlciBwbHVnaW5zIGFyZSBlbmFibGVkLgorCisgICAgICAgICogVUlQcm9jZXNzL0FQSS9n bGliL1dlYktpdFNldHRpbmdzLmNwcDoKKyAgICAgICAgKHdlYmtpdF9zZXR0aW5nc19nZXRfZW5h YmxlX3BsdWdpbnMpOgorICAgICAgICAod2Via2l0X3NldHRpbmdzX3NldF9lbmFibGVfcGx1Z2lu cyk6CisKIDIwMjEtMDEtMTUgIENocmlzIER1bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KIAogICAg ICAgICBbR1BVUHJvY2Vzc10gSW1wcm92ZSB0aGUgR1BVUHJvY2VzcycgbWVtb3J5IHByZXNzdXJl IGhhbmRsZXIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9nbGliL1dl YktpdFNldHRpbmdzLmNwcCBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9nbGliL1dlYktp dFNldHRpbmdzLmNwcAppbmRleCBkYTk2MjM3YWNmNjI5NjNlZTIxYjVjZmMxZjkxMTk2ZTI0Njc5 NzE0Li5mMWFlNzA3NmQzMDRmMTM5NTdjY2IyOTIyYjg3ZDVlYmU3OTg5ZGNkIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9BUEkvZ2xpYi9XZWJLaXRTZXR0aW5ncy5jcHAKKysr IGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL2dsaWIvV2ViS2l0U2V0dGluZ3MuY3BwCkBA IC0xOTAxLDExICsxOTAxLDE0IEBAIGdib29sZWFuIHdlYmtpdF9zZXR0aW5nc19nZXRfZW5hYmxl X3BsdWdpbnMoV2ViS2l0U2V0dGluZ3MqIHNldHRpbmdzKQogICoKICAqIERlcHJlY2F0ZWQ6IDIu MzIKICAqLwotdm9pZCB3ZWJraXRfc2V0dGluZ3Nfc2V0X2VuYWJsZV9wbHVnaW5zKFdlYktpdFNl dHRpbmdzKiBzZXR0aW5ncywgZ2Jvb2xlYW4pCit2b2lkIHdlYmtpdF9zZXR0aW5nc19zZXRfZW5h YmxlX3BsdWdpbnMoV2ViS2l0U2V0dGluZ3MqIHNldHRpbmdzLCBnYm9vbGVhbiBlbmFibGVkKQog ewogICAgIGdfcmV0dXJuX2lmX2ZhaWwoV0VCS0lUX0lTX1NFVFRJTkdTKHNldHRpbmdzKSk7CiAK LSAgICBnX3dhcm5pbmcoIndlYmtpdF9zZXR0aW5nc19zZXRfZW5hYmxlX3BsdWdpbnMgaXMgZGVw cmVjYXRlZCBhbmQgZG9lcyBub3RoaW5nLiBQbHVnaW5zIGFyZSBubyBsb25nZXIgc3VwcG9ydGVk LiIpOworICAgIC8vIFdlJ2xsIHByaW50IGEgd2FybmluZyBpZiB0cnlpbmcgdG8gZW5hYmxlIHBs dWdpbnMsIHNpbmNlIHRoZSBhcHBsaWNhdGlvbiBpcyBicm9rZW4gaWYgaXQKKyAgICAvLyBuZWVk cyBwbHVnaW5zLiBEaXNhYmxpbmcgcGx1Z2lucyBkb2VzIG5vdCBuZWVkIGEgd2FybmluZyB1bmxl c3Mgc3BlY2lmaWNhbGx5IHJlcXVlc3RlZC4KKyAgICBpZiAoZW5hYmxlZCB8fCAhZ19zdHJjbXAw KGdfZ2V0ZW52KCJHX0VOQUJMRV9ESUFHTk9TVElDIiksICIxIikpCisgICAgICAgIGdfd2Fybmlu Zygid2Via2l0X3NldHRpbmdzX3NldF9lbmFibGVfcGx1Z2lucyBpcyBkZXByZWNhdGVkIGFuZCBk b2VzIG5vdGhpbmcuIFBsdWdpbnMgYXJlIG5vIGxvbmdlciBzdXBwb3J0ZWQuIik7CiB9CiAKIC8q Kgo= 419187 2021-02-03 14:43:05 -0800 2021-02-03 15:17:42 -0800 Patch for landing bug-220620-20210203164304.patch text/plain 1952 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjcyMTQyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDJjYWQ3OGYxZTZlZjQ4OTlk NmZlMzA0N2U5MzkzMmRjZWI5M2IxZTIuLmE2ZDkzODk0ZGEzN2Q5YzAxZmRmY2ZiY2I3ZTc4Njk5 OTVjYTY4MjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMjEtMDItMDMgIE1pY2hhZWwg Q2F0YW56YXJvICA8bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgW0dUS10gd2Via2l0 X3NldHRpbmdzX3NldF9lbmFibGVfcGx1Z2lucyBkZXByZWNhdGlvbiB3YXJuaW5nIGJyZWFrcyB1 bml0IHRlc3RzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0yMjA2MjAKKworICAgICAgICBSZXZpZXdlZCBieSBDYXJsb3MgR2FyY2lhIENhbXBvcy4KKwor ICAgICAgICBEb24ndCBwcmludCBhIHdhcm5pbmcgd2hlbiBjYWxsaW5nIHdlYmtpdF9zZXR0aW5n c19zZXRfZW5hYmxlX3BsdWdpbnMoKSB0byBkaXNhYmxlIHBsdWdpbnMuCisgICAgICAgIFdlJ2xs IHN0aWxsIHByaW50IGEgd2FybmluZyBpZiBhdHRlbXB0aW5nIHRvIGVuYWJsZSBwbHVnaW5zLCBv ciBpZiBjaGVja2luZyB3aGV0aGVyIHBsdWdpbnMKKyAgICAgICAgYXJlIGVuYWJsZWQuCisKKyAg ICAgICAgKiBVSVByb2Nlc3MvQVBJL2dsaWIvV2ViS2l0U2V0dGluZ3MuY3BwOgorICAgICAgICAo d2Via2l0X3NldHRpbmdzX2dldF9lbmFibGVfcGx1Z2lucyk6CisgICAgICAgICh3ZWJraXRfc2V0 dGluZ3Nfc2V0X2VuYWJsZV9wbHVnaW5zKToKKwogMjAyMS0wMi0wMSAgQ2FybG9zIEdhcmNpYSBD YW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKICAgICAgICAgW1NPVVBdIEFkZCBzdXBwb3J0 IGZvciBkYXRhIFVSTHMgaW4gdGhlIG5ldHdvcmsgcHJvY2VzcwpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYktpdC9VSVByb2Nlc3MvQVBJL2dsaWIvV2ViS2l0U2V0dGluZ3MuY3BwIGIvU291cmNlL1dl YktpdC9VSVByb2Nlc3MvQVBJL2dsaWIvV2ViS2l0U2V0dGluZ3MuY3BwCmluZGV4IGRhOTYyMzdh Y2Y2Mjk2M2VlMjFiNWNmYzFmOTExOTZlMjQ2Nzk3MTQuLmIyZGQ0Y2JhMTA2ZTM1ZGY0N2E1MmFk NjAzODBhNGFkOGM3ZWU4ZjggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQ SS9nbGliL1dlYktpdFNldHRpbmdzLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9B UEkvZ2xpYi9XZWJLaXRTZXR0aW5ncy5jcHAKQEAgLTE5MDEsMTEgKzE5MDEsMTIgQEAgZ2Jvb2xl YW4gd2Via2l0X3NldHRpbmdzX2dldF9lbmFibGVfcGx1Z2lucyhXZWJLaXRTZXR0aW5ncyogc2V0 dGluZ3MpCiAgKgogICogRGVwcmVjYXRlZDogMi4zMgogICovCi12b2lkIHdlYmtpdF9zZXR0aW5n c19zZXRfZW5hYmxlX3BsdWdpbnMoV2ViS2l0U2V0dGluZ3MqIHNldHRpbmdzLCBnYm9vbGVhbikK K3ZvaWQgd2Via2l0X3NldHRpbmdzX3NldF9lbmFibGVfcGx1Z2lucyhXZWJLaXRTZXR0aW5ncyog c2V0dGluZ3MsIGdib29sZWFuIGVuYWJsZWQpCiB7CiAgICAgZ19yZXR1cm5faWZfZmFpbChXRUJL SVRfSVNfU0VUVElOR1Moc2V0dGluZ3MpKTsKIAotICAgIGdfd2FybmluZygid2Via2l0X3NldHRp bmdzX3NldF9lbmFibGVfcGx1Z2lucyBpcyBkZXByZWNhdGVkIGFuZCBkb2VzIG5vdGhpbmcuIFBs dWdpbnMgYXJlIG5vIGxvbmdlciBzdXBwb3J0ZWQuIik7CisgICAgaWYgKGVuYWJsZWQpCisgICAg ICAgIGdfd2FybmluZygid2Via2l0X3NldHRpbmdzX3NldF9lbmFibGVfcGx1Z2lucyBpcyBkZXBy ZWNhdGVkIGFuZCBkb2VzIG5vdGhpbmcuIFBsdWdpbnMgYXJlIG5vIGxvbmdlciBzdXBwb3J0ZWQu Iik7CiB9CiAKIC8qKgo=