RESOLVED FIXED 236476
[GTK] Sync gtk-overlay-scrolling setting to web process
https://bugs.webkit.org/show_bug.cgi?id=236476
Summary [GTK] Sync gtk-overlay-scrolling setting to web process
Michael Catanzaro
Reported 2022-02-10 17:21:34 PST
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.
Attachments
Patch (10.69 KB, patch)
2022-02-10 17:24 PST, Michael Catanzaro
no flags
Patch (10.60 KB, patch)
2022-02-11 06:03 PST, Michael Catanzaro
no flags
Patch (11.12 KB, patch)
2022-02-21 15:06 PST, Michael Catanzaro
no flags
Michael Catanzaro
Comment 1 2022-02-10 17:24:57 PST
Michael Catanzaro
Comment 2 2022-02-10 17:29:05 PST
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
Comment 3 2022-02-11 06:01:20 PST
I suppose I'll change it to be not static.
Michael Catanzaro
Comment 4 2022-02-11 06:03:06 PST
Michael Catanzaro
Comment 5 2022-02-17 11:57:01 PST
Ping reviewers.
Alice Mikhaylenko
Comment 6 2022-02-17 12:00:54 PST
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 7 2022-02-17 12:25:06 PST
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
Comment 8 2022-02-17 12:32:09 PST
That's UI process, I mean on the web process side.
Alice Mikhaylenko
Comment 9 2022-02-17 12:33:04 PST
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
Comment 10 2022-02-17 13:26:54 PST
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
Comment 11 2022-02-17 13:44:06 PST
Well, hence I wonder if it should call themeChanged() so it notices the changes immediately? :)
Michael Catanzaro
Comment 12 2022-02-21 15:06:34 PST
OK, sure.
Michael Catanzaro
Comment 13 2022-02-21 15:06:57 PST
EWS
Comment 14 2022-03-08 06:51:44 PST
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].
Note You need to log in before you can comment on or make changes to this bug.