[GTK] Fix logic of dark theme detection
Created attachment 379656 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 379656 [details] Patch Now that you are a committer, you should have a trac account and can add this to https://trac.webkit.org/wiki/WebKitGTK/2.26.x. Otherwise your change might not reach users until 2.28.
Comment on attachment 379656 [details] Patch Clearing flags on attachment: 379656 Committed r250388: <https://trac.webkit.org/changeset/250388>
All reviewed patches have been landed. Closing bug.
Comment on attachment 379656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379656&action=review > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:569 > if (g_str_has_suffix(themeName.get(), "-dark")) Um, well ":dark" isn't handled here. Surely this is a mistake? Looks like a mistake to me. Nowadays we also have PageClientImpl::themeName that would need to be fixed as well.
(Assuming this is a mistake....)
Created attachment 394977 [details] Patch
Comment on attachment 394977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394977&action=review > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:574 > GUniqueOutPtr<char> themeName; > g_object_get(settings, "gtk-theme-name", &themeName.outPtr(), nullptr); > - if (g_str_has_suffix(themeName.get(), "-dark")) > + if (g_str_has_suffix(themeName.get(), "-dark") || g_str_has_suffix(themeName.get(), ":dark")) > return true; Patrick, is this OK?
(In reply to Michael Catanzaro from comment #9) > Comment on attachment 394977 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394977&action=review > > > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:574 > > GUniqueOutPtr<char> themeName; > > g_object_get(settings, "gtk-theme-name", &themeName.outPtr(), nullptr); > > - if (g_str_has_suffix(themeName.get(), "-dark")) > > + if (g_str_has_suffix(themeName.get(), "-dark") || g_str_has_suffix(themeName.get(), ":dark")) > > return true; > > Patrick, is this OK? I don't think it is. Just check GTK code instead of guessing: https://gitlab.gnome.org/GNOME/gtk/-/blob/gtk-3-24/gtk/gtksettings.c#L3255 ':' is only used to check the theme variant for the GTK_THEME env variable.
Comment on attachment 394977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394977&action=review >>> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:574 >>> return true; >> >> Patrick, is this OK? > > I don't think it is. Just check GTK code instead of guessing: > > https://gitlab.gnome.org/GNOME/gtk/-/blob/gtk-3-24/gtk/gtksettings.c#L3255 > > ':' is only used to check the theme variant for the GTK_THEME env variable. As carlos said :dark is a variant and not part of the theme name.
OK!