Bug 227854 - [GTK] Propagate GtkSettings to web process
Summary: [GTK] Propagate GtkSettings to web process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alice Mikhaylenko
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-10 15:00 PDT by Alice Mikhaylenko
Modified: 2021-07-21 12:01 PDT (History)
13 users (show)

See Also:


Attachments
Patch (26.94 KB, patch)
2021-07-10 15:02 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (26.94 KB, patch)
2021-07-10 15:06 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (27.85 KB, patch)
2021-07-16 00:35 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (45.62 KB, patch)
2021-07-19 07:41 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (45.52 KB, patch)
2021-07-20 02:07 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Mikhaylenko 2021-07-10 15:00:31 PDT
See the patch.
Comment 1 Alice Mikhaylenko 2021-07-10 15:02:20 PDT
Created attachment 433275 [details]
Patch

The amount of settings here makes me wonder if they should all be contained in a separate struct instead of passing each and every one separately.
Comment 2 Alice Mikhaylenko 2021-07-10 15:03:28 PDT
And the reason I made scrollSettingsDidChange() even though it only contains one property is, it will contain another one in future - gtk-overlay-scrolling.
Comment 3 EWS Watchlist 2021-07-10 15:03:33 PDT
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
Comment 4 Alice Mikhaylenko 2021-07-10 15:06:16 PDT
Created attachment 433276 [details]
Patch

Looks like the style checker took the list of properties as a list of changed files.
Comment 5 Alice Mikhaylenko 2021-07-11 08:52:52 PDT
This fails with PSON.
Comment 6 Alice Mikhaylenko 2021-07-11 10:24:52 PDT
Ok, so we need to do the same thing in reinitializeWebPage().
Comment 7 Alice Mikhaylenko 2021-07-16 00:35:53 PDT
Created attachment 433662 [details]
Patch

The code duplication in WebPage.cpp makes me wonder if we should pass the parameters into platformInitialize() and platformReinitialize(), and contain this all in WebPageGtk.cpp in a shared function instead.
Comment 8 Alice Mikhaylenko 2021-07-19 07:41:08 PDT
Created attachment 433788 [details]
Patch
Comment 9 Alice Mikhaylenko 2021-07-19 07:41:43 PDT
Reworked as discussed in Matrix.
Comment 10 Michael Catanzaro 2021-07-19 09:21:34 PDT
Comment on attachment 433788 [details]
Patch

LGTM
Comment 11 Alice Mikhaylenko 2021-07-19 10:45:58 PDT
Ah yeah, back to the forgetting `--request-commit` in `webkit-patch upload`. :) Fixed.
Comment 12 Carlos Garcia Campos 2021-07-20 01:23:32 PDT
Comment on attachment 433788 [details]
Patch

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

Looks great, thank you!

> Source/WebKit/Shared/gtk/GtkSettingsState.h:38
> +    GtkSettingsState();

I guess you can do = default here and remove the empty implementation from the cpp?

> Source/WebKit/UIProcess/gtk/GtkSettingsManager.cpp:171
> +    , m_settingsState(GtkSettingsState())

This is not needed, right?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:736
> -    themeDidChange(WTFMove(parameters.themeName));
> +    GtkSettingsManagerProxy::singleton().applySettings(parameters.gtkSettings);

You can WTFMove here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:973
> +#if PLATFORM(GTK)
> +    GtkSettingsManagerProxy::singleton().applySettings(parameters.gtkSettings);
> +#endif

And here

> Source/WebKit/WebProcess/gtk/GtkSettingsManagerProxy.cpp:50
> +    applySettings(state);

And move here as well

> Source/WebKit/WebProcess/gtk/GtkSettingsManagerProxy.cpp:53
> +void GtkSettingsManagerProxy::applySettings(GtkSettingsState& state)

And make this receive a GtkSettingsState&&
Comment 13 Alice Mikhaylenko 2021-07-20 02:07:18 PDT
Created attachment 433860 [details]
Patch
Comment 14 EWS 2021-07-20 04:08:09 PDT
Committed r280077 (239803@main): <https://commits.webkit.org/239803@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433860 [details].
Comment 15 Carlos Alberto Lopez Perez 2021-07-21 12:01:41 PDT
(In reply to EWS from comment #14)
> Committed r280077 (239803@main): <https://commits.webkit.org/239803@main>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 433860 [details].

This caused 155 new unexpected test failures on GTK, see bug 228153