Currently we are using "user-invisible-symbolic" which is nice but it could change as the theme changes. A bug was filed in GNOME to get the new icon: https://bugzilla.gnome.org/show_bug.cgi?id=722198 As soon as the GNOME bug is resolved we need to do two things in WebKitGTK+: * Bump the dependencies of the gnome-theme-icons-symbolic package to the version containing the fix. * Load the new icon with a fallback to the "user-invisible-symbolic" for compatibility with older versions of the theme.
Created attachment 225472 [details] Patch
We should try to fall back to the old icon, I think. This is so new it will surely break on old systems, no?
(In reply to comment #2) > We should try to fall back to the old icon, I think. This is so new it will surely break on old systems, no? We do fall back on the old icon, that's why the function we use for the other media buttons wasn't enough.
Ah, I see. Why do we need to bump the jhbuild requirements if you are not updating test results?
(In reply to comment #4) > Ah, I see. Why do we need to bump the jhbuild requirements if you are not updating test results? It shouldn't be necessary, I guess. But it looks nicer :)
We need to rebaseline the pixel tests then.
(In reply to comment #6) > We need to rebaseline the pixel tests then. It's weird, but I checked them and none of them required rebaseline. I guess having the trackmenu tests skipped can be the reason.
(In reply to comment #7) > (In reply to comment #6) > > We need to rebaseline the pixel tests then. > > It's weird, but I checked them and none of them required rebaseline. I guess having the trackmenu tests skipped can be the reason. Well, at least point, what seems best is removing the jhbuild part of the patch as the track menu tests are skipped.
Created attachment 225486 [details] Patch
Comment on attachment 225486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225486&action=review > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:659 > + if (!iconName) > + return nullptr; Just noticed this but wouldn't an ASSERT make more sense here, since we always pass a static string. > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:883 > + return nullptr; > + Ditto.
(In reply to comment #10) > (From update of attachment 225486 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225486&action=review > > > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:659 > > + if (!iconName) > > + return nullptr; > > Just noticed this but wouldn't an ASSERT make more sense here, since we always pass a static string. Not in the piece of code that I wrote (RenderThemeGtk.cpp:571). If I hadn't done this, the result would be the same but some assertions would be triggered inside the GTK+ code. To avoid it I needed to bail out before getting to the GTK+ code, but the ASSERT doesn't bail out. I can avoid that "if" but I would need to refactor RenderThemeGtk::paintMediaButton and RenderThemeGtk::getStockSymbolicIconForWidgetType. Which option do you prefer?
I think that you should change the if statements to an ASSERT and then in getStockSymbolicIconForWidgetType, not call getStockIconForWidgetType if the fallback icon is null.
Created attachment 225569 [details] Patch
Comment on attachment 225569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225569&action=review > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:674 > + if (fallbackStockIconName) > + return nullptr; > + Not sure I understand this check. If the argument can never be non-null why have it?
Created attachment 225591 [details] Patch
(In reply to comment #14) > (From update of attachment 225569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225569&action=review > > > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:674 > > + if (fallbackStockIconName) > > + return nullptr; > > + > > Not sure I understand this check. If the argument can never be non-null why have it? Good catch, I should check for !fallbackStockIconName instead. I added the assertions as you had asked but to prevent hitting them I check and bail out before calling the function.
Comment on attachment 225591 [details] Patch Clearing flags on attachment: 225591 Committed r164977: <http://trac.webkit.org/changeset/164977>
All reviewed patches have been landed. Closing bug.