Cleanup some audio classes with C++11 shine!

media/base/audio_renderer_mixer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/audio_renderer_mixer.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 
@@ skipping 40 matching lines @@
   audio_converter_.RemoveInput(input);
 }
 
 void AudioRendererMixer::AddErrorCallback(const base::Closure& error_cb) {
   base::AutoLock auto_lock(lock_);
   error_callbacks_.push_back(error_cb);
 }
 
 void AudioRendererMixer::RemoveErrorCallback(const base::Closure& error_cb) {
   base::AutoLock auto_lock(lock_);
   for (ErrorCallbackList::iterator it = error_callbacks_.begin();

[wolenetz, 2014/09/26 01:22:31]

I wish for a range-based equivalent here, but we need the iterator for erase.
Any better ideas that the C++11 stuff could give us here?

[Peter Kasting, 2014/09/26 01:52:32]

The only thing I can think of is something like this (didn't test this):

  base::AutoLock auto_lock(lock_);
  const auto& i =
      std::find_if(error_callbacks_.begin(), error_callbacks_.end(),
                   std::bind2nd(std::mem_fun(&base::Closure::Equals),
                                error_cb));
  DCHECK(i != error_callbacks_.end());
  error_callbacks_.erase(i);

This isn't actually C++11, and some people are allergic to things like bind2nd
and mem_fun, but to someone comfortable with algorithm it expresses better than
the original code that the purpose here is to find an element matching a
predicate (and assert such an element exists) as opposed to looping over all
elements and performing some kind of operation.

I don't know if there's a better way to do this in C++11, but if there is, it
probably involves lambdas, which aren't OKed yet.

[wolenetz, 2014/09/26 07:23:15]

Bring on the lambdas :). Thanks for the insight - rarely do I see mem_fun and
bind2nd in common code.
The behavior of this alternative would be different vs the original if the
error_cb isn't in error_callbacks_: in the original: NOTREACHED() in release
build would be LOG(ERR, ...), not erase(error_callbacks_.end()). Since not C++11
related, probably doesn't belong in this CL anyway.

[DaleCurtis, 2014/09/26 20:39:51]

It should never not find the callback, hence the NOTREACHED().

While find_if+erase works, I wouldn't recommend it where only a single element
is expected to be removed. find_if must iterate over the entire container every
time, versus bailing on first match.

Notably we could introduce a templated "zipper" similar to Python's enumerate()
which would zip values such that we could do something like this:

for (const auto& v : base::Enumerate(container)) {
  // v.first == index, v.second == value.
  if (v.second == search_target) {
    container.erase(container.begin() + v.first);
    break;
  }
}

Some googling reveals I'm not the only one with this idea:

http://stackoverflow.com/questions/10962290/find-position-of-element-in-c11-r...

        it != error_callbacks_.end();
        ++it) {
     if (it->Equals(error_cb)) {
       error_callbacks_.erase(it);
       return;
     }
   }
 
   // An error callback should always exist when called.
   NOTREACHED();
@@ skipping 15 matching lines @@
   }
 
   audio_converter_.ConvertWithDelay(
       base::TimeDelta::FromMilliseconds(audio_delay_milliseconds), audio_bus);
   return audio_bus->frames();
 }
 
 void AudioRendererMixer::OnRenderError() {
   // Call each mixer input and signal an error.
   base::AutoLock auto_lock(lock_);
-  for (ErrorCallbackList::const_iterator it = error_callbacks_.begin();
-       it != error_callbacks_.end();
-       ++it) {
-    it->Run();
-  }
+  for (const auto& cb : error_callbacks_)

[wolenetz, 2014/09/26 01:22:31]

aside (I'm filing a crbug): rietveld's lint doesn't like C++11: "Storage class
(static, extern, typedef, etc) should be first."

+    cb.Run();
 }
 
 }  // namespace media