Refactoring AudioDirectiveHandler to support testing

components/copresence/handlers/audio/audio_directive_handler_impl.cc

 // Copyright 2014 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 "components/copresence/handlers/audio/audio_directive_handler.h"
+#include "components/copresence/handlers/audio/audio_directive_handler_impl.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "components/copresence/handlers/audio/audio_directive_list.h"
 #include "components/copresence/handlers/audio/tick_clock_ref_counted.h"
 #include "components/copresence/mediums/audio/audio_manager_impl.h"
 #include "components/copresence/proto/data.pb.h"
 #include "components/copresence/public/copresence_constants.h"
 #include "media/base/audio_bus.h"
 
 namespace copresence {
 
 namespace {
 
 base::TimeTicks GetEarliestEventTime(AudioDirectiveList* list,
                                      base::TimeTicks event_time) {
   if (!list->GetActiveDirective())
     return event_time;
 
-  if (event_time.is_null())
-    return list->GetActiveDirective()->end_time;
-  else
-    return std::min(list->GetActiveDirective()->end_time, event_time);
+  return event_time.is_null() ?
+      list->GetActiveDirective()->end_time :
+      std::min(list->GetActiveDirective()->end_time, event_time);
 }
 
 }  // namespace
 
-// Public methods.
 
-AudioDirectiveHandler::AudioDirectiveHandler()
-    : audio_event_timer_(new base::OneShotTimer<AudioDirectiveHandler>),
-      clock_(new TickClockRefCounted(
-          make_scoped_ptr(new base::DefaultTickClock))) {
+// Public functions.
+
+AudioDirectiveHandlerImpl::AudioDirectiveHandlerImpl()
+    : audio_manager_(new AudioManagerImpl),
+      audio_event_timer_(new base::OneShotTimer<AudioDirectiveHandler>),
+      clock_(new TickClockRefCounted(new base::DefaultTickClock)) {
+  transmits_lists_.push_back(new AudioDirectiveList(clock_));
+  transmits_lists_.push_back(new AudioDirectiveList(clock_));
+  receives_lists_.push_back(new AudioDirectiveList(clock_));
+  receives_lists_.push_back(new AudioDirectiveList(clock_));
 }
 
-AudioDirectiveHandler::~AudioDirectiveHandler() {
+// TODO(ckehoe): Merge these two constructors when
+//               delegating constructors are allowed.
+AudioDirectiveHandlerImpl::AudioDirectiveHandlerImpl(
+    scoped_ptr<AudioManager> audio_manager,
+    scoped_ptr<base::Timer> timer,
+    const scoped_refptr<TickClockRefCounted>& clock)
+    : audio_manager_(audio_manager.Pass()),
+      audio_event_timer_(timer.Pass()),
+      clock_(clock) {
+  transmits_lists_.push_back(new AudioDirectiveList(clock));

[rkc, 2014/11/03 20:14:43]

Why aren't these in the Initialize function? Won't that prevent the code
duplication?

[Charlie, 2014/11/03 22:45:13]

Done.

+  transmits_lists_.push_back(new AudioDirectiveList(clock));
+  receives_lists_.push_back(new AudioDirectiveList(clock));
+  receives_lists_.push_back(new AudioDirectiveList(clock));
 }
 
-void AudioDirectiveHandler::Initialize(
+AudioDirectiveHandlerImpl::~AudioDirectiveHandlerImpl() {}
+
+void AudioDirectiveHandlerImpl::Initialize(
     const AudioManager::DecodeSamplesCallback& decode_cb,
     const AudioManager::EncodeTokenCallback& encode_cb) {
   if (!audio_manager_)
     audio_manager_.reset(new AudioManagerImpl());
   audio_manager_->Initialize(decode_cb, encode_cb);
 }
 
-void AudioDirectiveHandler::AddInstruction(const TokenInstruction& instruction,
-                                           const std::string& op_id,
-                                           base::TimeDelta ttl) {
+void AudioDirectiveHandlerImpl::AddInstruction(
+    const TokenInstruction& instruction,
+    const std::string& op_id,
+    base::TimeDelta ttl) {
   switch (instruction.token_instruction_type()) {
     case TRANSMIT:
       DVLOG(2) << "Audio Transmit Directive received. Token: "
                << instruction.token_id()
                << " with medium=" << instruction.medium()
                << " with TTL=" << ttl.InMilliseconds();
       switch (instruction.medium()) {
         case AUDIO_ULTRASOUND_PASSBAND:

[rkc, 2014/11/03 20:14:43]

DCHECK(transmits_lists_[INAUDIBLE]);
Then audible and receive lists below.

[Charlie, 2014/11/03 22:45:13]

I'm not sure this checks for the thing you want. It only verifies that the
AudioDirectiveList pointer, as long as it's already in the vector, is not null.
If the vector index is out of range, std::vector will throw an error:

https://code.google.com/p/chromium/codesearch#chromium/usr/include/c%2B%2B/4....

[rkc, 2014/11/03 22:52:15]

This is true. So lets DCHECK the vector size then. I want us to ensure that if
we get here without running initialize, we have a DCHECK to catch it.

[Charlie, 2014/11/03 23:08:58]

The check inside of std::vector already does what you want. It stack traces with
a clear out-of-bounds error message. Presumably, like DCHECK, it is only enabled
in debug mode.

[rkc, 2014/11/04 00:37:22]

I dislike depending on platform specific implementations of a language,
particularly in multi-platform code.
Do get this DCHECK in another CL, or if you upload another rev to this CL.

[Charlie, 2014/11/04 01:07:46]

In principle I agree, although since this is only a debug check, we would have
to be running a debug build on a non-glibc platform for it to matter. Which
seems like it will not be happening anytime soon. I hope to have moved the
DirectiveList initialization to a delegated constructor before then.

However this is a simple fix in the meantime. CL forthcoming.

-          transmits_list_[INAUDIBLE].AddDirective(op_id, ttl);
+          transmits_lists_[INAUDIBLE]->AddDirective(op_id, ttl);
           audio_manager_->SetToken(INAUDIBLE, instruction.token_id());
           break;
         case AUDIO_AUDIBLE_DTMF:
-          transmits_list_[AUDIBLE].AddDirective(op_id, ttl);
+          transmits_lists_[AUDIBLE]->AddDirective(op_id, ttl);
           audio_manager_->SetToken(AUDIBLE, instruction.token_id());
           break;
         default:
           NOTREACHED();
       }
       break;
     case RECEIVE:
       DVLOG(2) << "Audio Receive Directive received."
                << " with medium=" << instruction.medium()
                << " with TTL=" << ttl.InMilliseconds();
       switch (instruction.medium()) {
         case AUDIO_ULTRASOUND_PASSBAND:
-          receives_list_[INAUDIBLE].AddDirective(op_id, ttl);
+          receives_lists_[INAUDIBLE]->AddDirective(op_id, ttl);
           break;
         case AUDIO_AUDIBLE_DTMF:
-          receives_list_[AUDIBLE].AddDirective(op_id, ttl);
+          receives_lists_[AUDIBLE]->AddDirective(op_id, ttl);
           break;
         default:
           NOTREACHED();
       }
       break;
     case UNKNOWN_TOKEN_INSTRUCTION_TYPE:
     default:
       LOG(WARNING) << "Unknown Audio Transmit Directive received. type = "
                    << instruction.token_instruction_type();
   }
   ProcessNextInstruction();
 }
 
-void AudioDirectiveHandler::RemoveInstructions(const std::string& op_id) {
-  transmits_list_[AUDIBLE].RemoveDirective(op_id);
-  transmits_list_[INAUDIBLE].RemoveDirective(op_id);
-  receives_list_[AUDIBLE].RemoveDirective(op_id);
-  receives_list_[INAUDIBLE].RemoveDirective(op_id);
+void AudioDirectiveHandlerImpl::RemoveInstructions(const std::string& op_id) {
+  transmits_lists_[AUDIBLE]->RemoveDirective(op_id);
+  transmits_lists_[INAUDIBLE]->RemoveDirective(op_id);
+  receives_lists_[AUDIBLE]->RemoveDirective(op_id);
+  receives_lists_[INAUDIBLE]->RemoveDirective(op_id);
 
   ProcessNextInstruction();
 }
 
-const std::string AudioDirectiveHandler::PlayingToken(AudioType type) const {
+const std::string AudioDirectiveHandlerImpl::PlayingToken(
+    AudioType type) const {
   return audio_manager_->GetToken(type);
 }
 
-void AudioDirectiveHandler::set_clock_for_testing(
-    const scoped_refptr<TickClockRefCounted>& clock) {
-  clock_ = clock;
 
-  transmits_list_[AUDIBLE].set_clock_for_testing(clock);
-  transmits_list_[INAUDIBLE].set_clock_for_testing(clock);
-  receives_list_[AUDIBLE].set_clock_for_testing(clock);
-  receives_list_[INAUDIBLE].set_clock_for_testing(clock);
-}
+// Private functions.
 
-void AudioDirectiveHandler::set_timer_for_testing(
-    scoped_ptr<base::Timer> timer) {
-  audio_event_timer_.swap(timer);
-}
-
-// Private methods.
-
-void AudioDirectiveHandler::ProcessNextInstruction() {
+void AudioDirectiveHandlerImpl::ProcessNextInstruction() {
   DCHECK(audio_event_timer_);
   audio_event_timer_->Stop();
 
   // Change |audio_manager_| state for audible transmits.
-  if (transmits_list_[AUDIBLE].GetActiveDirective())
+  if (transmits_lists_[AUDIBLE]->GetActiveDirective())
     audio_manager_->StartPlaying(AUDIBLE);
   else
     audio_manager_->StopPlaying(AUDIBLE);
 
   // Change audio_manager_ state for inaudible transmits.
-  if (transmits_list_[INAUDIBLE].GetActiveDirective())
+  if (transmits_lists_[INAUDIBLE]->GetActiveDirective())
     audio_manager_->StartPlaying(INAUDIBLE);
   else
     audio_manager_->StopPlaying(INAUDIBLE);
 
   // Change audio_manager_ state for audible receives.
-  if (receives_list_[AUDIBLE].GetActiveDirective())
+  if (receives_lists_[AUDIBLE]->GetActiveDirective())
     audio_manager_->StartRecording(AUDIBLE);
   else
     audio_manager_->StopRecording(AUDIBLE);
 
   // Change audio_manager_ state for inaudible receives.
-  if (receives_list_[INAUDIBLE].GetActiveDirective())
+  if (receives_lists_[INAUDIBLE]->GetActiveDirective())
     audio_manager_->StartRecording(INAUDIBLE);
   else
     audio_manager_->StopRecording(INAUDIBLE);
 
   base::TimeTicks next_event_time;
   if (GetNextInstructionExpiry(&next_event_time)) {
     audio_event_timer_->Start(
         FROM_HERE,
         next_event_time - clock_->NowTicks(),
-        base::Bind(&AudioDirectiveHandler::ProcessNextInstruction,
+        base::Bind(&AudioDirectiveHandlerImpl::ProcessNextInstruction,
                    base::Unretained(this)));
   }
 }
 
-bool AudioDirectiveHandler::GetNextInstructionExpiry(base::TimeTicks* expiry) {
+bool AudioDirectiveHandlerImpl::GetNextInstructionExpiry(
+    base::TimeTicks* expiry) {
   DCHECK(expiry);
 
-  *expiry = GetEarliestEventTime(&transmits_list_[AUDIBLE], base::TimeTicks());
-  *expiry = GetEarliestEventTime(&transmits_list_[INAUDIBLE], *expiry);
-  *expiry = GetEarliestEventTime(&receives_list_[AUDIBLE], *expiry);
-  *expiry = GetEarliestEventTime(&receives_list_[INAUDIBLE], *expiry);
+  *expiry = GetEarliestEventTime(transmits_lists_[AUDIBLE], base::TimeTicks());
+  *expiry = GetEarliestEventTime(transmits_lists_[INAUDIBLE], *expiry);
+  *expiry = GetEarliestEventTime(receives_lists_[AUDIBLE], *expiry);
+  *expiry = GetEarliestEventTime(receives_lists_[INAUDIBLE], *expiry);
 
   return !expiry->is_null();
 }
 
 }  // namespace copresence