Bug 198787 – [GTK] GTK_STOCK_* types have been deprecated since GTK 3.10

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Ludovico de Nittis]

All the GTK_STOCK_* types are deprecated since GTK 3.10

I'm going to attach a patch.

[Ludovico de Nittis]

Created attachment 371950 [details]
Patch

[Michael Catanzaro]

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.)

[Ludovico de Nittis]

(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

[Ludovico de Nittis]

Created attachment 371953 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[Ludovico de Nittis]

Created attachment 371962 [details]
Patch

[Ludovico de Nittis]

(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.

[WebKit Commit Bot]

Comment on attachment 371962 [details]
Patch

Clearing flags on attachment: 371962

Committed r246374: <https://trac.webkit.org/changeset/246374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.