[webkit-reviews] review granted: [Bug 227854] [GTK] Propagate GtkSettings to web process : [Attachment 433788] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Alexander Mikhaylenko
<alexm at gnome.org>'s request for review:
Bug 227854: [GTK] Propagate GtkSettings to web process
https://bugs.webkit.org/show_bug.cgi?id=227854

Attachment 433788: Patch

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




--- Comment #12 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 433788
  --> https://bugs.webkit.org/attachment.cgi?id=433788
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&&