Summary: | [GTK] Mute/unmute button on <video> elements are backwards | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Hommey <mh+webkit> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, eric.carlson, gustavo, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Mike Hommey
2010-01-21 12:37:39 PST
Created attachment 47138 [details]
Patch
With the patch
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 on attachment 47138 [details] Patch Every change requires a ChangeLog. see http://webkit.org/coding/contributing.html (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.) Or you could change the name to indicate the action they will take: m_willMuteButton, m_willPauseButton, etc. (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. (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. 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): 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 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> All reviewed patches have been landed. Closing bug. |