Bug 236476 – [GTK] Sync gtk-overlay-scrolling setting to web process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Instead of checking only this environment variable, let's check the GtkSetting value as well. This is possible since r280077 now that we have GtkSettingsManager and GtkSettingsManagerProxy. Note the environment variable surprisingly actually overrides the value in settings. Also note the environment variable is specific to GTK 3 only.

[Michael Catanzaro]

Created attachment 451624 [details]
Patch

[Michael Catanzaro]

Comment on attachment 451624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451624&action=review

> Source/WebCore/platform/adwaita/ScrollbarThemeAdwaita.cpp:74
> -    static bool shouldUuseOverlayScrollbars = g_strcmp0(g_getenv("GTK_OVERLAY_SCROLLING"), "0");
> -    return shouldUuseOverlayScrollbars;
> +    return shouldUseOverlayScrollbars();

Note this isn't quite right, because it's no longer static, so ScrollbarThemeAdwaita will check for settings changes, but RenderThemeScrollbar actually is still static so it won't check... I should change one or the other before landing this, probably RenderThemeScrollbar. Opinions welcome.

[Michael Catanzaro]

I suppose I'll change it to be not static.

[Michael Catanzaro]

Created attachment 451683 [details]
Patch

[Michael Catanzaro]

Ping reviewers.

[Alice Mikhaylenko]

Do scrollbar invalidate if you change the setting in runtime? Existing code listens to "notify::gtk-theme-name", so I suppose you'll need to listen to "notify::gtk-overlay-scrolling" as well.

[Michael Catanzaro]

Comment on attachment 451683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451683&action=review

> Source/WebKit/UIProcess/gtk/GtkSettingsManager.cpp:209
>      g_signal_connect_swapped(m_settings, "notify::gtk-cursor-blink", G_CALLBACK(settingsChangedCallback), this);
>      g_signal_connect_swapped(m_settings, "notify::gtk-cursor-blink-time", G_CALLBACK(settingsChangedCallback), this);
>      g_signal_connect_swapped(m_settings, "notify::gtk-primary-button-warps-slider", G_CALLBACK(settingsChangedCallback), this);
> +    g_signal_connect_swapped(m_settings, "notify::gtk-overlay-scrolling", G_CALLBACK(settingsChangedCallback), this);

We have notifies for everything here.

[Alice Mikhaylenko]

That's UI process, I mean on the web process side.

[Alice Mikhaylenko]

To be more specific, check this line in ScrollbarThemeGtk.cpp:

g_signal_connect(gtk_settings_get_default(), "notify::gtk-theme-name", G_CALLBACK(themeChangedCallback), nullptr);

[Michael Catanzaro]

Well it won't *immediately* notice changes, but it should notice next time scrollbars are drawn, because the change does get synced to the web process, yes? It's checking the setting each time. The setting is just a bool, so there isn't anything else to sync like there is in themeChangedCallback.

[Alice Mikhaylenko]

Well, hence I wonder if it should call themeChanged() so it notices the changes immediately? :)

[Michael Catanzaro]

OK, sure.

[Michael Catanzaro]

Created attachment 452780 [details]
Patch

[EWS]

Committed r290988 (248166@main): <https://commits.webkit.org/248166@main>

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