[webkit-reviews] review denied: [Bug 185370] WKPageSetMediaVolume() API couldn't be used to restore the volume : [Attachment 339715] .

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied  review:
Bug 185370: WKPageSetMediaVolume() API couldn't be used to restore the volume
https://bugs.webkit.org/show_bug.cgi?id=185370

Attachment 339715: .

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




--- Comment #8 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 339715
  --> https://bugs.webkit.org/attachment.cgi?id=339715
.

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

We can't land this as it does not have ChangeLog.

> Source/WebCore/html/HTMLMediaElement.cpp:4820
> +	       Page* page = document().page();
> +	       if (!page || page->mediaVolume() == 1)
> +		   m_referenceVolume = vol;

I see a big problem with this code. Let's think of this use case:

1. m_volume = 1; m_referenceVolume = 1; page->m_mediaVolume = 1;
2. setPageMediaVolume(0.5); -> m_volume = 0.5; m_referenceVolume = 1;
page->m_mediaVolume = 0.5;
3. Lower the stream volume to 0.25 -> m_volume = 0.25; m_referenceVolume = 1;
page->m_mediaVolume = 0.5;
4. setPageMediaVolume(1); -> m_volume = 1; m_referenceVolume = 1;
page->m_mediaVolume = 1;

Step 4 is a problem, because you lowered the volume to almost nothing and then
the set page volume ramps it up to 1 when the user had lowered the volume to
0.25.

Eric, I think this behavior is wrong. We should consider, at least,
m_referenceVolume to scale as if page volume were always 1 since we are already
multiplying it with the volumeMultiplier when going back down to the player. If
we go for this solution, we should, IMHO, instead of this code, do:
m_referenceVolume = m_volume;
if (page)
    m_referenceVolume /= page->mediaVolume();

And if we accept my premise, we could even consider m_referenceVolume to be
redundant and consider that m_volume is always scaled with reference to the
page being 1. If we consider this alternative, we might want to touch
HTMLMediaElement::setReadyState and HTMLMediaElement::volume() to correct with
the page volume or we would be getting lots of tests failing. We might just
change HTMLMediaElement::volume() and have HTMLMediaElement::setReadyState
using HTMLMediaElement::volume(). I would also add a couple of comments in
m_volume and HTMLMediaElement::volume() regarding this.