Let the effective media volume be 0 if the media element is muted

Let the effective media volume be 0 if the media element is muted

The bug was with a muted media element slaved to an unmuted media
controller, in which case the media element's muted state was ignored.

Attaching a media controller to an already-playing media element also
wouldn't update the volume, so do that in bringElementUpToSpeed().

BUG=399623
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179738

[philipj_slow, 2014-08-06 21:39:09]

PTAL. playerVolume() is untested, is it OK to leave it like that or should I
make it testable via Internals?

[acolwell GONE FROM CHROMIUM, 2014-08-06 23:51:05]

lgtm

https://codereview.chromium.org/444173002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/444173002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3295: shouldMute =
m_mediaController->muted();
So is the bug here then? IIUC the media controller can be "not-muted" eventhough
the media element is muted?

[philipj_slow, 2014-08-07 07:34:23]

https://codereview.chromium.org/444173002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/444173002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3295: shouldMute =
m_mediaController->muted();
On 2014/08/06 23:51:05, acolwell wrote:
> So is the bug here then? IIUC the media controller can be "not-muted"
eventhough
> the media element is muted?

Precisely, the media element's and the media controller's script-visible muted
state are independent, so the bug was with a muted media element and unmuted
media controller.

[philipj_slow, 2014-08-07 15:35:20]

I decided to make this more like effectivePlaybackRate(), moved things around a
little and added a test. The test made me realize that there's a bug when
attaching a controller to an existing element, even though the test doesn't
actually cover it.

Please take another look.

[philipj_slow, 2014-08-07 15:39:09]

rebase

[acolwell GONE FROM CHROMIUM, 2014-08-07 15:52:59]

lgtm

https://codereview.chromium.org/444173002/diff/40001/LayoutTests/media/effect...
File LayoutTests/media/effective-media-volume.html (right):

https://codereview.chromium.org/444173002/diff/40001/LayoutTests/media/effect...
LayoutTests/media/effective-media-volume.html:7: [0, 0.5,
1].forEach(function(volume)
ooo.. fancy. :)

https://codereview.chromium.org/444173002/diff/40001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/444173002/diff/40001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:109: double
WebMediaPlayerClientImpl::volume() const
I'm assuming a followup CL to remove this function is coming after
https://codereview.chromium.org/443393002/ lands.

[philipj_slow, 2014-08-07 15:59:28]

https://codereview.chromium.org/444173002/diff/40001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/444173002/diff/40001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:109: double
WebMediaPlayerClientImpl::volume() const
On 2014/08/07 15:52:59, acolwell wrote:
> I'm assuming a followup CL to remove this function is coming after
> https://codereview.chromium.org/443393002/ lands.

Exactly, this CL is how I noticed this oddity. It would be nice to get rid of
the setVolume in load() as well.

[philipj_slow, 2014-08-07 15:59:50]

adamk, can you rubberstamp Source/web?

[adamk, 2014-08-07 16:49:40]

rslgtm for Source/web

[philipj_slow, 2014-08-07 18:02:32]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-08-07 18:06:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/444173002/40001

[commit-bot: I haz the power, 2014-08-07 18:47:41]

Change committed as 179738