All the GTK_STOCK_* types are deprecated since GTK 3.10 I'm going to attach a patch.
Created attachment 371950 [details] Patch
Comment on attachment 371950 [details] Patch Well the GTK_CHECK_VERSIONs aren't really needed here, are they? Can't we do this unconditionally? (Nobody is going to be using GTK 3.6 anymore anyway; it's long past time to bump our requirement.)
(In reply to Michael Catanzaro from comment #2) > Comment on attachment 371950 [details] > Patch > > Well the GTK_CHECK_VERSIONs aren't really needed here, are they? Can't we do > this unconditionally? > > (Nobody is going to be using GTK 3.6 anymore anyway; it's long past time to > bump our requirement.) Sure, the code will be even easier to follow. I added the check because I saw here that GTK 3.6 was still listed https://trac.webkit.org/wiki/WebKitGTK/Dependencies
Created attachment 371953 [details] Patch
Comment on attachment 371953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371953&action=review Yeah but hardcoding the icon names should work just fine with older versions of GTK, so I think this newer version is fine. Now, the function should no longer be called gtkStockIDFromContextMenuAction, since that's clearly no longer what the function does. But... the GtkAction API actually does work with stock IDs. It's not documented to accept icon names. So I really don't think this is right. I think the only correct way to get rid of the stock items here would be to just remove this function entirely and always pass nullptr for the stock ID instead, at the cost of no longer having icons in the context menu in whatever weird non-default environments still allow those. Or we could leave them as-is. The GtkAction we are creating is deprecated anyway, but we can't get rid of that because it's part of the API. So getting rid of deprecated stock items when creating a deprecated GtkAction doesn't seem as clear a win as your other patches, where you were able to fully replace use of deprecated APIs with non-deprecated ones. Either way is OK with me. > Source/WebKit/Shared/glib/WebContextMenuItemGlib.cpp:75 > case ContextMenuItemTagSpellingGuess: > - return nullptr; > case ContextMenuItemTagIgnoreSpelling: > - return GTK_STOCK_NO; > case ContextMenuItemTagLearnSpelling: > - return GTK_STOCK_OK; > + return nullptr; I would move these down to the bottom of the list, with the others that return nullptr.
(In reply to Michael Catanzaro from comment #5) > Yeah but hardcoding the icon names should work just fine with older versions > of GTK, so I think this newer version is fine. Oops, this should generally be true, but I wrote this before I realized the problem that the GtkAction API really wants stock IDs and not icon names.
Created attachment 371962 [details] Patch
(In reply to Michael Catanzaro from comment #5) > Comment on attachment 371953 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371953&action=review > > Yeah but hardcoding the icon names should work just fine with older versions > of GTK, so I think this newer version is fine. > > Now, the function should no longer be called > gtkStockIDFromContextMenuAction, since that's clearly no longer what the > function does. > > But... the GtkAction API actually does work with stock IDs. It's not > documented to accept icon names. So I really don't think this is right. I > think the only correct way to get rid of the stock items here would be to > just remove this function entirely and always pass nullptr for the stock ID > instead, at the cost of no longer having icons in the context menu in > whatever weird non-default environments still allow those. > > Or we could leave them as-is. The GtkAction we are creating is deprecated > anyway, but we can't get rid of that because it's part of the API. So > getting rid of deprecated stock items when creating a deprecated GtkAction > doesn't seem as clear a win as your other patches, where you were able to > fully replace use of deprecated APIs with non-deprecated ones. > > Either way is OK with me. > > > Source/WebKit/Shared/glib/WebContextMenuItemGlib.cpp:75 > > case ContextMenuItemTagSpellingGuess: > > - return nullptr; > > case ContextMenuItemTagIgnoreSpelling: > > - return GTK_STOCK_NO; > > case ContextMenuItemTagLearnSpelling: > > - return GTK_STOCK_OK; > > + return nullptr; > > I would move these down to the bottom of the list, with the others that > return nullptr. Thank you for the review. I've uploaded a new patch where I removed the function entirely.
Comment on attachment 371962 [details] Patch Clearing flags on attachment: 371962 Committed r246374: <https://trac.webkit.org/changeset/246374>
All reviewed patches have been landed. Closing bug.