| Summary: | [GTK] webkit_settings_set_enable_plugins deprecation warning breaks unit tests | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Gratton <mike> | ||||||
| Component: | WebKitGTK | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, mcatanzaro | ||||||
| Priority: | P3 | Keywords: | Gtk | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Attachments: |
|
||||||||
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. Created attachment 417735 [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 https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API 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(). > 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?
(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. (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.... Yeah the patch as-is sounds like decent practice in general. 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? 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. Comment on attachment 417735 [details] 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. Created attachment 419187 [details]
Patch for landing
Committed r272342: <https://trac.webkit.org/changeset/272342> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419187 [details]. |
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.