Bug 220620 – [GTK] webkit_settings_set_enable_plugins deprecation warning breaks unit tests

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Gratton]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 417735 [details]
Patch

[EWS Watchlist]

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

[Carlos Garcia Campos]

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().

[Michael Gratton]

> 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?

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

(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....

[Michael Gratton]

Yeah the patch as-is sounds like decent practice in general.

[Carlos Garcia Campos]

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?

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

Created attachment 419187 [details]
Patch for landing

[EWS]

Committed r272342: <https://trac.webkit.org/changeset/272342>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419187 [details].