[webkit-reviews] review denied: [Bug 198787] [GTK] GTK_STOCK_* types have been deprecated since GTK 3.10 : [Attachment 371953] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Ludovico de Nittis
<ludovico.denittis at collabora.com>'s request for review:
Bug 198787: [GTK] GTK_STOCK_* types have been deprecated since GTK 3.10
https://bugs.webkit.org/show_bug.cgi?id=198787

Attachment 371953: Patch

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




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 371953
  --> https://bugs.webkit.org/attachment.cgi?id=371953
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.