Moving the DirectiveHandler to be owned by CopresenceManager.

Moving the DirectiveHandler to be owned by CopresenceManager. This also adds the ability to queue Directives. Tests will be forthcoming.

BUG=424253
Committed: https://crrev.com/4a0582439389d0f6142312f81317d1ba13fa34bb
Cr-Commit-Position: refs/heads/master@{#302362}

[Charlie, 2014-10-30 22:26:13]

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

[rkc, 2014-10-31 17:18:06]

https://codereview.chromium.org/684273004/diff/70001/components/copresence/co...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:41: // Public functions
Any reason why this is functions instead? They all still seem to be members of a
class :)
Also, nit, add a period, here and below.

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
File components/copresence/handlers/directive_handler.h (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:22: // TODO(ckehoe): Add
tests for this class.
This class is now complex enough that this shouldn't just be a TODO. I'd feel a
lot more comfortable if we checked this in with at least a few basic tests.

https://codereview.chromium.org/684273004/diff/70001/components/copresence/rp...
File components/copresence/rpc/rpc_handler_unittest.cc (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/rp...
components/copresence/rpc/rpc_handler_unittest.cc:46: class FakeDirectiveHandler
: public DirectiveHandler {
You've made a believer out of me. We should separate out the directive handler
interface, implementation and stub like we did with the AudioManager. Don't need
to in this CL, but add do add a TODO.

[Charlie, 2014-10-31 17:47:06]

https://codereview.chromium.org/684273004/diff/70001/components/copresence/co...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:41: // Public functions
On 2014/10/31 17:18:06, Rahul Chaturvedi wrote:
> Any reason why this is functions instead? They all still seem to be members of
a
> class :)
> Also, nit, add a period, here and below.

C++ has functions, not methods.

http://stackoverflow.com/questions/8596461/in-c-what-is-the-difference-betwee...

I don't really believe in periods for headings (as opposed to descriptive
comments). But we can put them in if they make you more comfortable :-)

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
File components/copresence/handlers/directive_handler.h (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:22: // TODO(ckehoe): Add
tests for this class.
On 2014/10/31 17:18:06, Rahul Chaturvedi wrote:
> This class is now complex enough that this shouldn't just be a TODO. I'd feel
a
> lot more comfortable if we checked this in with at least a few basic tests.

I agree, but there's some refactoring required to allow a fake
AudioDirectiveHandler to be injected properly. So I was going to do that plus
the tests in a separate CL. Instead, I'll do just the refactoring separately,
and add the tests here once I sync.

https://codereview.chromium.org/684273004/diff/70001/components/copresence/rp...
File components/copresence/rpc/rpc_handler_unittest.cc (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/rp...
components/copresence/rpc/rpc_handler_unittest.cc:46: class FakeDirectiveHandler
: public DirectiveHandler {
On 2014/10/31 17:18:06, Rahul Chaturvedi wrote:
> You've made a believer out of me. We should separate out the directive handler
> interface, implementation and stub like we did with the AudioManager. Don't
need
> to in this CL, but add do add a TODO.

Um. How is this different from the TODO above? :-)

[rkc, 2014-10-31 18:06:16]

lgtm

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
File components/copresence/handlers/directive_handler.h (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:22: // TODO(ckehoe): Add
tests for this class.
On 2014/10/31 17:47:06, Charlie wrote:
> On 2014/10/31 17:18:06, Rahul Chaturvedi wrote:
> > This class is now complex enough that this shouldn't just be a TODO. I'd
feel
> a
> > lot more comfortable if we checked this in with at least a few basic tests.
> 
> I agree, but there's some refactoring required to allow a fake
> AudioDirectiveHandler to be injected properly. So I was going to do that plus
> the tests in a separate CL. Instead, I'll do just the refactoring separately,
> and add the tests here once I sync.

That's fine, you can do them both in another CL. As long as they get done right
after this CL and not Soon(TM). :)

https://codereview.chromium.org/684273004/diff/70001/components/copresence/rp...
File components/copresence/rpc/rpc_handler_unittest.cc (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/rp...
components/copresence/rpc/rpc_handler_unittest.cc:46: class FakeDirectiveHandler
: public DirectiveHandler {
On 2014/10/31 17:47:06, Charlie wrote:
> On 2014/10/31 17:18:06, Rahul Chaturvedi wrote:
> > You've made a believer out of me. We should separate out the directive
handler
> > interface, implementation and stub like we did with the AudioManager. Don't
> need
> > to in this CL, but add do add a TODO.
> 
> Um. How is this different from the TODO above? :-)

Err right. Ignore me.

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

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
File components/copresence/handlers/directive_handler.h (right):

https://codereview.chromium.org/684273004/diff/70001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:22: // TODO(ckehoe): Add
tests for this class.
On 2014/10/31 18:06:15, Rahul Chaturvedi wrote:
> On 2014/10/31 17:47:06, Charlie wrote:
> > On 2014/10/31 17:18:06, Rahul Chaturvedi wrote:
> > > This class is now complex enough that this shouldn't just be a TODO. I'd
> feel
> > a
> > > lot more comfortable if we checked this in with at least a few basic
tests.
> > 
> > I agree, but there's some refactoring required to allow a fake
> > AudioDirectiveHandler to be injected properly. So I was going to do that
plus
> > the tests in a separate CL. Instead, I'll do just the refactoring
separately,
> > and add the tests here once I sync.
> 
> That's fine, you can do them both in another CL. As long as they get done
right
> after this CL and not Soon(TM). :)

The CL is in progress already. And you know how much I like refactoring :-)

[Charlie, 2014-10-31 19:22:50]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-10-31 19:23:15]

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

[commit-bot: I haz the power, 2014-11-01 00:11:35]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2014-11-01 00:13:43]

Patchset 8 (id:??) landed as
https://crrev.com/4a0582439389d0f6142312f81317d1ba13fa34bb
Cr-Commit-Position: refs/heads/master@{#302362}