[webkit-reviews] review granted: [Bug 223195] [GStreamer] Videos start playing muted in epiphany with no unmute icon visible in tab, webkit_web_view_get_is_muted() returns incorrect results : [Attachment 424494] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 223195: [GStreamer] Videos start playing muted in epiphany with no unmute
icon visible in tab, webkit_web_view_get_is_muted() returns incorrect results
https://bugs.webkit.org/show_bug.cgi?id=223195

Attachment 424494: Patch

https://bugs.webkit.org/attachment.cgi?id=424494&action=review




--- Comment #20 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 424494
  --> https://bugs.webkit.org/attachment.cgi?id=424494
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424494&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:7690
> +    bool isPlayingAudio = hasAudio && volume();

Shouldn't this be `bool isPlayingAudio = hasAudio && !muted() && volume()` to
avoid changing behavior on non-GStreamer ports?

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1678
> +    auto volume = this->volume();
> +    auto oldVolume = volume;
> +
>      // get_volume() can return values superior to 1.0 if the user
>      // applies software user gain via third party application (GNOME
>      // volume control for instance).
>      volume = CLAMP(volume, 0.0, 1.0);

Nit: this would be sightly more efficient as:

auto oldVolume = this->volume();
auto volume = CLAMP(oldVolume, 0.0, 1.0);