Bug 129091 - [GTK] Multimedia controls captions icon needs its own metaphor
Summary: [GTK] Multimedia controls captions icon needs its own metaphor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL: https://bugzilla.gnome.org/show_bug.c...
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-20 02:11 PST by Xabier Rodríguez Calvar
Modified: 2014-03-03 00:31 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.79 KB, patch)
2014-02-28 09:27 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2014-02-28 13:34 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (7.05 KB, patch)
2014-03-01 14:39 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (7.05 KB, patch)
2014-03-02 03:00 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2014-02-20 02:11:51 PST
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.
Comment 1 Xabier Rodríguez Calvar 2014-02-28 09:27:33 PST
Created attachment 225472 [details]
Patch
Comment 2 Martin Robinson 2014-02-28 10:31:29 PST
We should try to fall back to the old icon, I think. This is so new it will surely break on old systems, no?
Comment 3 Xabier Rodríguez Calvar 2014-02-28 10:58:17 PST
(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.
Comment 4 Martin Robinson 2014-02-28 11:28:15 PST
Ah, I see. Why do we need to bump the jhbuild requirements if you are not updating test results?
Comment 5 Xabier Rodríguez Calvar 2014-02-28 11:46:53 PST
(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 :)
Comment 6 Martin Robinson 2014-02-28 11:52:52 PST
We need to rebaseline the pixel tests then.
Comment 7 Xabier Rodríguez Calvar 2014-02-28 12:43:05 PST
(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.
Comment 8 Xabier Rodríguez Calvar 2014-02-28 13:22:03 PST
(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.
Comment 9 Xabier Rodríguez Calvar 2014-02-28 13:34:02 PST
Created attachment 225486 [details]
Patch
Comment 10 Martin Robinson 2014-02-28 13:37:10 PST
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.
Comment 11 Xabier Rodríguez Calvar 2014-02-28 14:54:20 PST
(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?
Comment 12 Martin Robinson 2014-02-28 15:53:31 PST
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.
Comment 13 Xabier Rodríguez Calvar 2014-03-01 14:39:19 PST
Created attachment 225569 [details]
Patch
Comment 14 Martin Robinson 2014-03-01 21:02:48 PST
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?
Comment 15 Xabier Rodríguez Calvar 2014-03-02 03:00:36 PST
Created attachment 225591 [details]
Patch
Comment 16 Xabier Rodríguez Calvar 2014-03-02 03:04:30 PST
(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.
Comment 17 WebKit Commit Bot 2014-03-03 00:31:18 PST
Comment on attachment 225591 [details]
Patch

Clearing flags on attachment: 225591

Committed r164977: <http://trac.webkit.org/changeset/164977>
Comment 18 WebKit Commit Bot 2014-03-03 00:31:21 PST
All reviewed patches have been landed.  Closing bug.