Bug 33967

Summary: [GTK] Mute/unmute button on <video> elements are backwards
Product: WebKit Reporter: Mike Hommey <mh+webkit>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eric.carlson, gns, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Attachments:
Description Flags
Patch
eric: review-
2010-04-21 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-21 Diego Escalante Urrelo <descalante@igalia.com> none

Description Mike Hommey 2010-01-21 12:37:39 PST
The button was added as part of #26304.

When the sound is muted, the icon displays the "audio-volume-high" icon, and on the contrary, when the sound is unmuted, the "audio-volume-muted" icon is displayed. While when you read the latter this way it can sound normal, it actually makes no sense, because it doesn't reflect the current state of the sound.

The patch I'm attaching will fix the obvious problem, but I think the variable names should be changed to something that makes it more obvious, like "m_mutedButton" and "m_unmutedButton" (which would reverse their meaning).

What do you think ?
Comment 1 Mike Hommey 2010-01-21 12:39:06 PST
Created attachment 47138 [details]
Patch

With the patch
Comment 2 Eric Seidel (no email) 2010-01-21 16:46:04 PST
Comment on attachment 47138 [details]
Patch

I don't like these variable names.  m_pauseButton will cause pause to happen, or shows the fact that it's currently paused?  The way this is written, it seems mute is being used the opposite.
Comment 3 Eric Seidel (no email) 2010-01-21 16:46:47 PST
Comment on attachment 47138 [details]
Patch

Every change requires a ChangeLog.  see http://webkit.org/coding/contributing.html
Comment 4 Mike Hommey 2010-01-22 00:10:52 PST
(In reply to comment #3)
> (From update of attachment 47138 [details])
> Every change requires a ChangeLog.  see
> http://webkit.org/coding/contributing.html

I know that ;)

I first wanted to know how this should be fixed. We seem to agree on the fact that the variable names are ambiguous. 
How about renaming them to a past participle form as proposed in comment #0 ? (m_mutedButton, m_pausedButton, etc.)
Comment 5 Eric Carlson 2010-02-03 20:16:47 PST
Or you could change the name to indicate the action they will take: m_willMuteButton, m_willPauseButton, etc.
Comment 6 Xan Lopez 2010-02-12 02:30:44 PST
(In reply to comment #5)
> Or you could change the name to indicate the action they will take:
> m_willMuteButton, m_willPauseButton, etc.

I think considering how they are used:

return paintMediaButton(paintInfo.context, r, mediaElement->muted() ? m_unmuteButton.get() : m_muteButton.get(), m_panelColor, m_mediaIconSize);

the names of the buttons are already OK (and consistent with the other buttons). It's just that what they do when clicked is the opposite to the status they should reflect, since they reverse themselves. So if we do what comment #0 suggests at the end, or similar, we'd make one thing clear but the other unclear (ie, the state would match the name but not the action).

So IMHO it's fine to just commit what the patch does, perhaps adding a small comment explaining it.
Comment 7 Eric Carlson 2010-02-12 08:04:07 PST
(In reply to comment #6)
> I think considering how they are used:
> 
> return paintMediaButton(paintInfo.context, r, mediaElement->muted() ?
> m_unmuteButton.get() : m_muteButton.get(), m_panelColor, m_mediaIconSize);
> 
> the names of the buttons are already OK (and consistent with the other
> buttons). It's just that what they do when clicked is the opposite to the
> status they should reflect, since they reverse themselves. So if we do what
> comment #0 suggests at the end, or similar, we'd make one thing clear but the
> other unclear (ie, the state would match the name but not the action).
> 
> So IMHO it's fine to just commit what the patch does, perhaps adding a small
> comment explaining it.

I am also fine with adding comments to the code and leaving the names as-is.
Comment 8 Diego Escalante Urrelo 2010-04-21 23:33:05 PDT
Created attachment 54030 [details]
2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

Reviewed by NOBODY (OOPS!).

        [GTK] Mute/unmute button on <video> elements are backwards
        https://bugs.webkit.org/show_bug.cgi?id=33967

        Fix mute/unmute buttons icons-action relation and explain why their
        variable names and corresponding icons seem to be misleading but are
        actually right. Original patch by Mike Hommey.

        * platform/gtk/RenderThemeGtk.cpp:
        (WebCore::RenderThemeGtk::initMediaStyling):
Comment 9 Diego Escalante Urrelo 2010-04-21 23:36:12 PDT
Created attachment 54031 [details]
2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

review!

2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

Reviewed by NOBODY (OOPS!).

        [GTK] Mute/unmute button on <video> elements are backwards
        https://bugs.webkit.org/show_bug.cgi?id=33967

        Fix mute/unmute buttons icons-action relation and explain why their
        variable names and corresponding icons seem to be misleading but are
        actually right. Original patch by Mike Hommey.

        * platform/gtk/RenderThemeGtk.cpp:
        (WebCore::RenderThemeGtk::initMediaStyling):
Comment 10 WebKit Commit Bot 2010-04-22 08:36:51 PDT
Comment on attachment 54031 [details]
2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 54031

Committed r58097: <http://trac.webkit.org/changeset/58097>
Comment 11 WebKit Commit Bot 2010-04-22 08:36:57 PDT
All reviewed patches have been landed.  Closing bug.