Bug 198787 - [GTK] GTK_STOCK_* types have been deprecated since GTK 3.10
Summary: [GTK] GTK_STOCK_* types have been deprecated since GTK 3.10
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-12 07:16 PDT by Ludovico de Nittis
Modified: 2019-06-12 13:43 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.81 KB, patch)
2019-06-12 07:21 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2019-06-12 07:28 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2019-06-12 08:42 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ludovico de Nittis 2019-06-12 07:16:43 PDT
All the GTK_STOCK_* types are deprecated since GTK 3.10

I'm going to attach a patch.
Comment 1 Ludovico de Nittis 2019-06-12 07:21:06 PDT
Created attachment 371950 [details]
Patch
Comment 2 Michael Catanzaro 2019-06-12 07:23:53 PDT
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.)
Comment 3 Ludovico de Nittis 2019-06-12 07:26:11 PDT
(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
Comment 4 Ludovico de Nittis 2019-06-12 07:28:48 PDT
Created attachment 371953 [details]
Patch
Comment 5 Michael Catanzaro 2019-06-12 07:56:07 PDT
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.
Comment 6 Michael Catanzaro 2019-06-12 07:57:36 PDT
(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.
Comment 7 Ludovico de Nittis 2019-06-12 08:42:08 PDT
Created attachment 371962 [details]
Patch
Comment 8 Ludovico de Nittis 2019-06-12 08:43:48 PDT
(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 9 WebKit Commit Bot 2019-06-12 13:43:42 PDT
Comment on attachment 371962 [details]
Patch

Clearing flags on attachment: 371962

Committed r246374: <https://trac.webkit.org/changeset/246374>
Comment 10 WebKit Commit Bot 2019-06-12 13:43:43 PDT
All reviewed patches have been landed.  Closing bug.