Refactoring AudioDirectiveHandler to support testing

components/copresence/handlers/audio/audio_directive_handler_impl.cc

Index: components/copresence/handlers/audio/audio_directive_handler_impl.cc
diff --git a/components/copresence/handlers/audio/audio_directive_handler.cc b/components/copresence/handlers/audio/audio_directive_handler_impl.cc
similarity index 54%
rename from components/copresence/handlers/audio/audio_directive_handler.cc
rename to components/copresence/handlers/audio/audio_directive_handler_impl.cc
index 7563193a23a42206bd450af616898c44b84707fb..2863b321a599499112f4d3afcad291bc578e7cad 100644
--- a/components/copresence/handlers/audio/audio_directive_handler.cc
+++ b/components/copresence/handlers/audio/audio_directive_handler_impl.cc
@@ -2,7 +2,7 @@
 // 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"
@@ -10,6 +10,7 @@
 #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"
@@ -25,26 +26,44 @@ base::TimeTicks GetEarliestEventTime(AudioDirectiveList* list,
   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_)
@@ -52,9 +71,10 @@ void AudioDirectiveHandler::Initialize(
   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: "
@@ -63,11 +83,11 @@ void AudioDirectiveHandler::AddInstruction(const TokenInstruction& instruction,
                << " 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:
@@ -80,10 +100,10 @@ void AudioDirectiveHandler::AddInstruction(const TokenInstruction& instruction,
                << " 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();
@@ -97,60 +117,47 @@ void AudioDirectiveHandler::AddInstruction(const TokenInstruction& instruction,
   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);
-}
-
-void AudioDirectiveHandler::set_timer_for_testing(
-    scoped_ptr<base::Timer> timer) {
-  audio_event_timer_.swap(timer);
-}
 
-// Private methods.
+// Private functions.
 
-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);
@@ -160,18 +167,19 @@ void AudioDirectiveHandler::ProcessNextInstruction() {
     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();
 }