Let MediaSession store duck state instead of volume multiplier

Let MediaSession store duck state instead of volume multiplier

BUG=642361
Committed: https://crrev.com/238868bfde91027175160a06ce46382124acc1d9
Cr-Commit-Position: refs/heads/master@{#415608}

[Zhiqiang Zhang (Slow), 2016-08-30 15:12:56]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 15:13:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhiqiang Zhang (Slow), 2016-08-30 15:20:33]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2016-08-30 15:20:33]

Splitting this patch out to reduce the forthcoming Flash CL.

Should be simple enough. Adding both of you in case anyone have time to review
;)

[commit-bot: I haz the power, 2016-08-30 15:35:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 15:35:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhiqiang Zhang (Slow), 2016-08-30 15:44:51]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 15:45:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 16:46:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 16:46:25]

Dry run: This issue passed the CQ dry run.

[whywhat, 2016-08-30 20:44:38]

lgtm % nits

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:96:
MediaSession::Get(focus_entry_->web_contents())->Unduck();
nit:

I don't believe unduck is a word? Are there examples in tech docs about this
term?
If not, Start/StopDuck[ing] would be my preference.

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager_unittest.cc (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/audio_focus_manager_unittest.cc:48: return
session->is_ducking_;
nit: add a comment like // Quack! Quack! :)

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_session.cc (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session.cc:220: for (const auto& it :
players_)
nit: duck/unduck seem to have very similar implementations - maybe extract the
common parts into a private helper method?

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_session.h (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session.h:202: CONTENT_EXPORT static const
double kDefaultVolumeMultiplier;
nit: document these public constants.

are these public only for the test? make them private and make the test a friend
or just hardcode the same values in the test. if you change the values here, the
test will break so you will update them.

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session.h:214: bool is_ducking_;
nit: comment when this becomes true and false.

[Zhiqiang Zhang (Slow), 2016-08-31 10:11:18]

Patchset #3 (id:40001) has been deleted

[Zhiqiang Zhang (Slow), 2016-08-31 10:11:28]

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:96:
MediaSession::Get(focus_entry_->web_contents())->Unduck();
On 2016/08/30 20:44:38, whywhat wrote:
> nit:
> 
> I don't believe unduck is a word? Are there examples in tech docs about this
> term?
> If not, Start/StopDuck[ing] would be my preference.

Done.

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager_unittest.cc (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/audio_focus_manager_unittest.cc:48: return
session->is_ducking_;
On 2016/08/30 20:44:38, whywhat wrote:
> nit: add a comment like // Quack! Quack! :)

Great idea ;)

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_session.cc (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session.cc:220: for (const auto& it :
players_)
On 2016/08/30 20:44:38, whywhat wrote:
> nit: duck/unduck seem to have very similar implementations - maybe extract the
> common parts into a private helper method?

Done.

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_session.h (right):

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session.h:202: CONTENT_EXPORT static const
double kDefaultVolumeMultiplier;
On 2016/08/30 20:44:38, whywhat wrote:
> nit: document these public constants.
> 
> are these public only for the test? make them private and make the test a
friend
> or just hardcode the same values in the test. if you change the values here,
the
> test will break so you will update them.

OK, put it back in anonymous namespace in .cc file, and let the tests have a
copy of the variables.

https://codereview.chromium.org/2291163002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session.h:214: bool is_ducking_;
On 2016/08/30 20:44:38, whywhat wrote:
> nit: comment when this becomes true and false.

Done.

[Zhiqiang Zhang (Slow), 2016-08-31 10:11:34]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-08-31 10:11:35]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2291163002/#ps60001
(title: "addressed Anton's comments")

[commit-bot: I haz the power, 2016-08-31 10:11:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 11:09:38]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-31 11:11:22]

Description was changed from

==========
Let MediaSession store duck state instead of volume multiplier

BUG=642361
==========

to

==========
Let MediaSession store duck state instead of volume multiplier

BUG=642361

Committed: https://crrev.com/238868bfde91027175160a06ce46382124acc1d9
Cr-Commit-Position: refs/heads/master@{#415608}
==========

[commit-bot: I haz the power, 2016-08-31 11:11:23]

Patchset 3 (id:??) landed as
https://crrev.com/238868bfde91027175160a06ce46382124acc1d9
Cr-Commit-Position: refs/heads/master@{#415608}

[mlamouri (slow - plz ping), 2016-08-31 20:39:02]

lgtm. Thanks for doing this! :)