Refactoring AudioDirectiveHandler to support testing

Refactoring AudioDirectiveHandler to support testing

Committed: https://crrev.com/63d3aa3c66b77df08c0143e18f983bb6747967e0
Cr-Commit-Position: refs/heads/master@{#302531}

[Charlie, 2014-10-31 20:38:51]

ckehoe@chromium.org changed reviewers:
+ rkc@chromium.org

[Charlie, 2014-10-31 20:38:52]

I had already started this, so might as well keep the refactoring in a separate
CL. That way we can argue about it in parallel with everything else :-)

Note that default parameters are allowed in constructors, per the Google C++
style guide.

[Charlie, 2014-10-31 20:55:17]

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_list.h (right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_list.h:46: explicit
AudioDirectiveList(const scoped_refptr<TickClockRefCounted>& clock =
I think we're always passing in a clock now. So we can make this argument
required if you want.

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

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.cc
(right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:58:
transmits_lists_.push_back(new AudioDirectiveList(clock));
Why aren't these in the Initialize function? Won't that prevent the code
duplication?

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:85: case
AUDIO_ULTRASOUND_PASSBAND:
DCHECK(transmits_lists_[INAUDIBLE]);
Then audible and receive lists below.

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_list.h (right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_list.h:46: explicit
AudioDirectiveList(const scoped_refptr<TickClockRefCounted>& clock =
On 2014/10/31 20:55:17, Charlie wrote:
> I think we're always passing in a clock now. So we can make this argument
> required if you want.

Sure.

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/tick_clock_ref_counted.h (right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/tick_clock_ref_counted.h:25: explicit
TickClockRefCounted(base::TickClock* clock);
Why do we need this?

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

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.cc
(right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:58:
transmits_lists_.push_back(new AudioDirectiveList(clock));
On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> Why aren't these in the Initialize function? Won't that prevent the code
> duplication?
> 

Done.

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:85: case
AUDIO_ULTRASOUND_PASSBAND:
On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> DCHECK(transmits_lists_[INAUDIBLE]);
> Then audible and receive lists below.

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....

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_list.h (right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_list.h:46: explicit
AudioDirectiveList(const scoped_refptr<TickClockRefCounted>& clock =
On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> On 2014/10/31 20:55:17, Charlie wrote:
> > I think we're always passing in a clock now. So we can make this argument
> > required if you want.
> 
> Sure.

I take it back. The unit test gets ugly without this. And using the default
clock is a good, well, default behavior.

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/tick_clock_ref_counted.h (right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/tick_clock_ref_counted.h:25: explicit
TickClockRefCounted(base::TickClock* clock);
On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> Why do we need this?

There's already a lot of boilerplate to create one of these things. In the unit
test, for example, the current syntax is:

make_scoped_refptr(new TickClockRefCounted(clock_ptr_))

Without this convenience constructor, we would instead write:

make_scoped_refptr(new TickClockRefCounted(
    make_scoped_ptr<base::TickClock>(clock_ptr_)))

which is terrible to read. We just want a reference-counted clock!

Given that the only purpose of this class is to wrap a preexisting type for
reference counting, I don't think it's surprising that the constructor would
take ownership of the pointer passed in. But it's documented anyway.

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

lgtm

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.cc
(right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:85: case
AUDIO_ULTRASOUND_PASSBAND:
On 2014/11/03 22:45:13, Charlie wrote:
> On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> > DCHECK(transmits_lists_[INAUDIBLE]);
> > Then audible and receive lists below.
> 
> 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....
> 

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.

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/tick_clock_ref_counted.h (right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/tick_clock_ref_counted.h:25: explicit
TickClockRefCounted(base::TickClock* clock);
On 2014/11/03 22:45:13, Charlie wrote:
> On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> > Why do we need this?
> 
> There's already a lot of boilerplate to create one of these things. In the
unit
> test, for example, the current syntax is:
> 
> make_scoped_refptr(new TickClockRefCounted(clock_ptr_))
> 
> Without this convenience constructor, we would instead write:
> 
> make_scoped_refptr(new TickClockRefCounted(
>     make_scoped_ptr<base::TickClock>(clock_ptr_)))
> 
> which is terrible to read. We just want a reference-counted clock!
> 
> Given that the only purpose of this class is to wrap a preexisting type for
> reference counting, I don't think it's surprising that the constructor would
> take ownership of the pointer passed in. But it's documented anyway.

That does look pretty ugly.

[Charlie, 2014-11-03 23:08:59]

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.cc
(right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:85: case
AUDIO_ULTRASOUND_PASSBAND:
On 2014/11/03 22:52:15, Rahul Chaturvedi wrote:
> On 2014/11/03 22:45:13, Charlie wrote:
> > On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> > > DCHECK(transmits_lists_[INAUDIBLE]);
> > > Then audible and receive lists below.
> > 
> > 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....
> > 
> 
> 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.

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.

[Charlie, 2014-11-03 23:52:12]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-11-03 23:53:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/690213004/120001

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

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.cc
(right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:85: case
AUDIO_ULTRASOUND_PASSBAND:
On 2014/11/03 23:08:58, Charlie wrote:
> On 2014/11/03 22:52:15, Rahul Chaturvedi wrote:
> > On 2014/11/03 22:45:13, Charlie wrote:
> > > On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> > > > DCHECK(transmits_lists_[INAUDIBLE]);
> > > > Then audible and receive lists below.
> > > 
> > > 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....
> > > 
> > 
> > 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.
> 
> 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.

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.

[commit-bot: I haz the power, 2014-11-04 00:45:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-04 00:47:08]

Patchset 7 (id:??) landed as
https://crrev.com/63d3aa3c66b77df08c0143e18f983bb6747967e0
Cr-Commit-Position: refs/heads/master@{#302531}

[Charlie, 2014-11-04 01:07:47]

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.cc
(right):

https://codereview.chromium.org/690213004/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.cc:85: case
AUDIO_ULTRASOUND_PASSBAND:
On 2014/11/04 00:37:22, Rahul Chaturvedi wrote:
> On 2014/11/03 23:08:58, Charlie wrote:
> > On 2014/11/03 22:52:15, Rahul Chaturvedi wrote:
> > > On 2014/11/03 22:45:13, Charlie wrote:
> > > > On 2014/11/03 20:14:43, Rahul Chaturvedi wrote:
> > > > > DCHECK(transmits_lists_[INAUDIBLE]);
> > > > > Then audible and receive lists below.
> > > > 
> > > > 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....
> > > > 
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.