Stylizing AudioConferenceMixer.

Stylizing AudioConferenceMixer.

Cleaning AudioConferenceMixer APIs to match Chromium style guide.

Main changes:
1. change all mutable references to pointers
2. add const to all non-mutable references
3. add const to as many methods as possible

BUG=
R=andrew@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/0f4b3731c34e796da92572380855dbc7321c8cfe

[minyue-webrtc, 2015-08-26 09:43:46]

minyue@webrtc.org changed reviewers:
+ ajm@chromium.org

[minyue-webrtc, 2015-08-26 11:04:39]

Hi Andrew,

Would you take a look at this CL?

[Andrew MacDonald, 2015-08-26 15:56:00]

andrew@webrtc.org changed reviewers:
+ andrew@webrtc.org

[Andrew MacDonald, 2015-08-26 15:56:01]

Good stuff.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer.h
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer.h:51:
virtual int32_t SetMixabilityStatus(MixerParticipant* participant,
Looks like participant can be const?

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer.h:55:
bool* mixable) const = 0;
Can you have this return the bool instead?

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h:32:
int32_t IsMixed(bool* mixed) const;
Return bool?

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc:514:
for (MixerParticipantList::const_iterator iter = mixList->begin();
You should be able to use range based for loops in several places in this file:
for (const auto& participant : mixList) ...

Feel free to change in a separate CL or not at all.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc:903:
bool AudioConferenceMixerImpl::LimitMixedAudio(AudioFrame* mixedAudio) const {
Even though you can, I don't think you should mark this const. It's modifying
the object pointed at by _limiter.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:39:
int32_t IsMixed(bool* mixed) const;
Return bool on both of these?

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:107:
const MixerParticipantList* mixList) const;
const reference

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:127:
const MixerParticipantList* participantList) const;
const reference

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:140:
const AudioFrameList* audioFrameList) const;
const reference

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:146:
const AudioFrameList* audioFrameList) const;
const reference

[minyue-webrtc, 2015-08-27 11:31:15]

Hi Andrew,

Thanks for the comments! Here is an update.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer.h
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer.h:51:
virtual int32_t SetMixabilityStatus(MixerParticipant* participant,
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> Looks like participant can be const?

This will finally invoke operations (push, erase) in participant list, which is
a list<participant*>. push_back(const participant*) is not defined, and hence
const participant& cannot be passed down.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer.h:55:
bool* mixable) const = 0;
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> Can you have this return the bool instead?

Yes, that sounds good.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h:32:
int32_t IsMixed(bool* mixed) const;
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> Return bool?

good idea

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc:903:
bool AudioConferenceMixerImpl::LimitMixedAudio(AudioFrame* mixedAudio) const {
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> Even though you can, I don't think you should mark this const. It's modifying
> the object pointed at by _limiter. 

const means that the class members are unaltered. _limiter as pointer is not
altered. and therefore it function can be const.

I do not think making it a const function brings confusion.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:39:
int32_t IsMixed(bool* mixed) const;
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> Return bool on both of these?

Done.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:107:
const MixerParticipantList* mixList) const;
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> const reference

Done.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:127:
const MixerParticipantList* participantList) const;
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> const reference

Done.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:140:
const AudioFrameList* audioFrameList) const;
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> const reference

Done.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:146:
const AudioFrameList* audioFrameList) const;
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> const reference

Done.

[Andrew MacDonald, 2015-08-28 18:33:20]

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc
(right):

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc:514:
for (MixerParticipantList::const_iterator iter = mixList->begin();
On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> You should be able to use range based for loops in several places in this
file:
> for (const auto& participant : mixList) ...
> 
> Feel free to change in a separate CL or not at all.

No change necessary, just wanted to make sure you saw this comment.

https://codereview.webrtc.org/1311733003/diff/1/webrtc/modules/audio_conferen...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc:903:
bool AudioConferenceMixerImpl::LimitMixedAudio(AudioFrame* mixedAudio) const {
On 2015/08/27 11:31:15, minyue-webrtc wrote:
> On 2015/08/26 15:56:01, Andrew MacDonald wrote:
> > Even though you can, I don't think you should mark this const. It's
modifying
> > the object pointed at by _limiter. 
> 
> const means that the class members are unaltered. _limiter as pointer is not
> altered. and therefore it function can be const.
> 
> I do not think making it a const function brings confusion.

Right, I know what const methods are. There's the concept of physical and
logical constness:
https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const

Since this changes the visible state of the object I would argue it should not
be const, but I don't insist.

https://codereview.webrtc.org/1311733003/diff/20001/webrtc/modules/audio_conf...
File
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h
(right):

https://codereview.webrtc.org/1311733003/diff/20001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h:31:
// Returns true if the participant was mixed this mix iteration
nit: end in a period.

https://codereview.webrtc.org/1311733003/diff/20001/webrtc/modules/audio_conf...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h
(right):

https://codereview.webrtc.org/1311733003/diff/20001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.h:38:
// MixerParticipant function
What is this comment trying to indicate? MixHistory isn't derived from
MixerParticipant so this isn't an override. Can you either remove it or add more
explanation? It's very misleading.

[Andrew MacDonald, 2015-08-28 18:34:41]

lgtm

[minyue-webrtc, 2015-08-31 08:10:38]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2015-08-31 08:10:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311733003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311733003/20001

[minyue-webrtc, 2015-08-31 08:10:53]

The CQ bit was unchecked by minyue@webrtc.org

[minyue-webrtc, 2015-08-31 14:04:49]

Committed patchset #3 (id:40001) manually as
0f4b3731c34e796da92572380855dbc7321c8cfe (presubmit successful).