Bug 129091 – [GTK] Multimedia controls captions icon needs its own metaphor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

Created attachment 225472 [details]
Patch

[Martin Robinson]

We should try to fall back to the old icon, I think. This is so new it will surely break on old systems, no?

[Xabier Rodríguez Calvar]

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

[Martin Robinson]

Ah, I see. Why do we need to bump the jhbuild requirements if you are not updating test results?

[Xabier Rodríguez Calvar]

(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 :)

[Martin Robinson]

We need to rebaseline the pixel tests then.

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

Created attachment 225486 [details]
Patch

[Martin Robinson]

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.

[Xabier Rodríguez Calvar]

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

[Martin Robinson]

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.

[Xabier Rodríguez Calvar]

Created attachment 225569 [details]
Patch

[Martin Robinson]

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?

[Xabier Rodríguez Calvar]

Created attachment 225591 [details]
Patch

[Xabier Rodríguez Calvar]

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

[WebKit Commit Bot]

Comment on attachment 225591 [details]
Patch

Clearing flags on attachment: 225591

Committed r164977: <http://trac.webkit.org/changeset/164977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.