Add polling and audio check to copresence.

Add polling and audio check to copresence.

This CL adds the ability to verify that we heard an audio token that we just
played - otherwise it raises an event to the app as a warning. We also need
to poll the server, whether or not we are actually hearing tokens - this is
so that the server can give us data back even if we are doing a broadcast_only
publish.

This CL also updates us to the latest whispernet.

R=ckehoe@chromium.org, xiyuan@chromium.org
BUG=390388, 426067, 423512
Committed: https://crrev.com/ab25c6e72f0f457f95224656aaad39a1e00b7471
Cr-Commit-Position: refs/heads/master@{#303111}

[Charlie, 2014-11-06 17:28:24]

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
File components/copresence/handlers/audio/audio_directive_handler_unittest.cc
(right):

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:56:
directive_handler_->Initialize(NULL, TokensCallback());
Pass a StubWhispernetClient here instead (owned by a local scoped_ptr). And
DCHECK whispernet in the AudioManager.

https://codereview.chromium.org/704923002/diff/1/components/copresence/test/s...
File components/copresence/test/stub_whispernet_client.cc (right):

https://codereview.chromium.org/704923002/diff/1/components/copresence/test/s...
components/copresence/test/stub_whispernet_client.cc:21: samples_cb_.Run(type,
token, CreateRandomAudioRefCounted(0x123, 1, 0x321));
These samples can get passed into the constructor, right? Does this get called
repeatedly?

https://codereview.chromium.org/704923002/diff/1/components/copresence/test/s...
components/copresence/test/stub_whispernet_client.cc:27: std::vector<AudioToken>
tokens;
Pass these into the constructor instead of hardcoding them here. Keep a
no-argument constructor that leaves the vector empty.

https://codereview.chromium.org/704923002/diff/20001/chrome/browser/copresenc...
File chrome/browser/copresence/chrome_whispernet_client.h (right):

https://codereview.chromium.org/704923002/diff/20001/chrome/browser/copresenc...
chrome/browser/copresence/chrome_whispernet_client.h:34: class
ChromeWhispernetClient final : public copresence::WhispernetClient {
Why not just put this class in the copresence namespace? It's in
chrome/browser/copresence, after all.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:122: // We destroy
|directive_handler_| before |rpc_handler_|, hence passing
It's the other way around. Class members are constructed in the order listed and
then destructed in reverse order.

However, this shouldn't be an issue unless the callback is invoked off the the
browser thread, or from the DirectiveHandler destructor. The same will be true
for any two objects that are nested or have a common owner.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:128: // Startup timers.
Start up

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:44: // TODO(rkc): Too many
audio specific functions here, find a better way to
CopresenceManagerImpl::CheckForAudio() probably belongs in the AudioManager. No
time to fix it in this CL though.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.cc (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:66: whispernet_client_
= whispernet_client;
DCHECK this. You can pass in stubs in from the tests now.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:99: if
(whispernet_client_) {
No if since this will be DCHECK'ed

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:152: // We're
destructed by the destruction chain of
This comment is out of date.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:202: if
(!tokens_cb_.is_null() && !tokens_to_report.empty())
Why should tokens_cb_ be null?

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.h (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.h:73: // Callbacks to
speak with whispernet.
Clarify what this callback actually does.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_unittest.cc:73: new
StubWhispernetClient,
Keep this in a local scoped_ptr. Otherwise it will leak.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_unittest.cc:79: void
GetTokens(const std::vector<AudioToken>& tokens) {
Shouldn't we start by setting last_received_decode_type_ = NONE?

https://codereview.chromium.org/704923002/diff/20001/components/copresence/pu...
File components/copresence/public/copresence_constants.h (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/pu...
components/copresence/public/copresence_constants.h:68: // const std::string&
token: The token that we encoded.
These arguments are now in a different order.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/rp...
components/copresence/rpc/rpc_handler_unittest.cc:103: void
HandleStatusUpdate(CopresenceStatus /* status */) override{};
override {}
No semicolon

[rkc, 2014-11-06 19:58:25]

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
File components/copresence/handlers/audio/audio_directive_handler_unittest.cc
(right):

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:56:
directive_handler_->Initialize(NULL, TokensCallback());
On 2014/11/06 17:28:22, Charlie wrote:
> Pass a StubWhispernetClient here instead (owned by a local scoped_ptr). And
> DCHECK whispernet in the AudioManager.

Why do I need to pass in a Stub to DCHECK in the AudioManager?

https://codereview.chromium.org/704923002/diff/1/components/copresence/test/s...
File components/copresence/test/stub_whispernet_client.cc (right):

https://codereview.chromium.org/704923002/diff/1/components/copresence/test/s...
components/copresence/test/stub_whispernet_client.cc:21: samples_cb_.Run(type,
token, CreateRandomAudioRefCounted(0x123, 1, 0x321));
On 2014/11/06 17:28:22, Charlie wrote:
> These samples can get passed into the constructor, right? Does this get called
> repeatedly?

Done.

https://codereview.chromium.org/704923002/diff/1/components/copresence/test/s...
components/copresence/test/stub_whispernet_client.cc:27: std::vector<AudioToken>
tokens;
On 2014/11/06 17:28:22, Charlie wrote:
> Pass these into the constructor instead of hardcoding them here. Keep a
> no-argument constructor that leaves the vector empty.

Done.

https://codereview.chromium.org/704923002/diff/20001/chrome/browser/copresenc...
File chrome/browser/copresence/chrome_whispernet_client.h (right):

https://codereview.chromium.org/704923002/diff/20001/chrome/browser/copresenc...
chrome/browser/copresence/chrome_whispernet_client.h:34: class
ChromeWhispernetClient final : public copresence::WhispernetClient {
On 2014/11/06 17:28:23, Charlie wrote:
> Why not just put this class in the copresence namespace? It's in
> chrome/browser/copresence, after all.

The copresence namespace defines code in the //components/copresence, not code
in the browser. These code uses copresence constants, but isn't really
copresence code. It is the browser's implementation of the WhispernetClient.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:122: // We destroy
|directive_handler_| before |rpc_handler_|, hence passing
On 2014/11/06 17:28:23, Charlie wrote:
> It's the other way around. Class members are constructed in the order listed
and
> then destructed in reverse order.
> 
> However, this shouldn't be an issue unless the callback is invoked off the the
> browser thread, or from the DirectiveHandler destructor. The same will be true
> for any two objects that are nested or have a common owner.

I am really uncomfortable with having classes depend on a class that depends on
them. At this time though, I am comfortable with this as long as we never use
the directive handler object from the rpc handler's destructor. Not calling this
callback from only the directive handler's dtor is not the only restriction.
Even the dtor of the AudioManager cannot call this callback. So this is a
cleaner solution than placing this restriction on a destructor several levels
lower on the dependency chain. 

Changed the declaration order of |rpc_handler_| and |directive_handler_| to
reflect this change and added a comment to the rpc_handler's destructor.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:128: // Startup timers.
On 2014/11/06 17:28:23, Charlie wrote:
> Start up

Done.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:44: // TODO(rkc): Too many
audio specific functions here, find a better way to
On 2014/11/06 17:28:23, Charlie wrote:
> CopresenceManagerImpl::CheckForAudio() probably belongs in the AudioManager.
No
> time to fix it in this CL though.

It doesn't solve the problem. If we put it in the audio manager, we'd need to
plumb down a callback to report errors; so the net result will be the same. We
have too much coupling between the audio manager (a component several levels
down the chain) and the copresence manager.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.cc (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:66: whispernet_client_
= whispernet_client;
On 2014/11/06 17:28:23, Charlie wrote:
> DCHECK this. You can pass in stubs in from the tests now.

Done.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:99: if
(whispernet_client_) {
On 2014/11/06 17:28:23, Charlie wrote:
> No if since this will be DCHECK'ed

Done but breaks the RPC handler test. Had to fix that.
Done.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:152: // We're
destructed by the destruction chain of
On 2014/11/06 17:28:23, Charlie wrote:
> This comment is out of date.

Done.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:202: if
(!tokens_cb_.is_null() && !tokens_to_report.empty())
On 2014/11/06 17:28:23, Charlie wrote:
> Why should tokens_cb_ be null?

Done.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.h (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.h:73: // Callbacks to
speak with whispernet.
On 2014/11/06 17:28:23, Charlie wrote:
> Clarify what this callback actually does.

Done.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_unittest.cc:73: new
StubWhispernetClient,
On 2014/11/06 17:28:23, Charlie wrote:
> Keep this in a local scoped_ptr. Otherwise it will leak.

Done.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_unittest.cc:79: void
GetTokens(const std::vector<AudioToken>& tokens) {
On 2014/11/06 17:28:23, Charlie wrote:
> Shouldn't we start by setting last_received_decode_type_ = NONE?

Done.

https://codereview.chromium.org/704923002/diff/20001/components/copresence/pu...
File components/copresence/public/copresence_constants.h (right):

https://codereview.chromium.org/704923002/diff/20001/components/copresence/pu...
components/copresence/public/copresence_constants.h:68: // const std::string&
token: The token that we encoded.
On 2014/11/06 17:28:24, Charlie wrote:
> These arguments are now in a different order.

Done.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/rp...
components/copresence/rpc/rpc_handler_unittest.cc:103: void
HandleStatusUpdate(CopresenceStatus /* status */) override{};
On 2014/11/06 17:28:24, Charlie wrote:
> override {}
> No semicolon

Done.

[xiyuan, 2014-11-06 21:27:50]

https://codereview.chromium.org/704923002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/704923002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:185:
service->copresence_apps().insert(extension_id());
When do we remove the app id from service?

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

https://codereview.chromium.org/704923002/diff/40001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:20: const int
kAudioCheckIntervalMs = 1000;  // milliseconds
nit: add "." to be consistent with previous line.

https://codereview.chromium.org/704923002/diff/40001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_unittest.cc
(right):

https://codereview.chromium.org/704923002/diff/40001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:51: // 
  whispernet_client_.reset(new StubWhispernetClient);
nit: Remove?

https://codereview.chromium.org/704923002/diff/40001/components/copresence/pu...
File components/copresence/public/copresence_constants.h (right):

https://codereview.chromium.org/704923002/diff/40001/components/copresence/pu...
components/copresence/public/copresence_constants.h:60: using SuccessCallback =
base::Callback<void(bool)>;
Are we allowed to use "using" in this scoped in a header file? Why not using
typedef?

[rkc, 2014-11-06 21:45:05]

https://codereview.chromium.org/704923002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/704923002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:185:
service->copresence_apps().insert(extension_id());
On 2014/11/06 21:27:50, xiyuan wrote:
> When do we remove the app id from service?

I just realized that we don't need this code at all. We can just broadcast the
event to any apps that are listening for it.

Done.

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

https://codereview.chromium.org/704923002/diff/40001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:20: const int
kAudioCheckIntervalMs = 1000;  // milliseconds
On 2014/11/06 21:27:50, xiyuan wrote:
> nit: add "." to be consistent with previous line.

Done.

https://codereview.chromium.org/704923002/diff/40001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_unittest.cc
(right):

https://codereview.chromium.org/704923002/diff/40001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:51: // 
  whispernet_client_.reset(new StubWhispernetClient);
On 2014/11/06 21:27:50, xiyuan wrote:
> nit: Remove?

Done.

https://codereview.chromium.org/704923002/diff/40001/components/copresence/pu...
File components/copresence/public/copresence_constants.h (right):

https://codereview.chromium.org/704923002/diff/40001/components/copresence/pu...
components/copresence/public/copresence_constants.h:60: using SuccessCallback =
base::Callback<void(bool)>;
On 2014/11/06 21:27:50, xiyuan wrote:
> Are we allowed to use "using" in this scoped in a header file? Why not using
> typedef?

New C++11 rules (http://chromium-cpp.appspot.com/). From "Aliases", "Use instead
of typedef, unless the header needs to be compatible with C."

[xiyuan, 2014-11-06 21:51:12]

LGTM but please also wait for Charlie.

https://codereview.chromium.org/704923002/diff/40001/components/copresence/pu...
File components/copresence/public/copresence_constants.h (right):

https://codereview.chromium.org/704923002/diff/40001/components/copresence/pu...
components/copresence/public/copresence_constants.h:60: using SuccessCallback =
base::Callback<void(bool)>;
On 2014/11/06 21:45:05, Rahul Chaturvedi wrote:
> On 2014/11/06 21:27:50, xiyuan wrote:
> > Are we allowed to use "using" in this scoped in a header file? Why not using
> > typedef?
> 
> New C++11 rules (http://chromium-cpp.appspot.com/). From "Aliases", "Use
instead
> of typedef, unless the header needs to be compatible with C."

Good to know. Apparently, I don't follow chromium-dev close enough. :p

[Charlie, 2014-11-06 21:53:14]

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
File components/copresence/handlers/audio/audio_directive_handler_unittest.cc
(right):

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:56:
directive_handler_->Initialize(NULL, TokensCallback());
On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> On 2014/11/06 17:28:22, Charlie wrote:
> > Pass a StubWhispernetClient here instead (owned by a local scoped_ptr). And
> > DCHECK whispernet in the AudioManager.
> 
> Why do I need to pass in a Stub to DCHECK in the AudioManager?

In the real implementation, WhispernetClient shouldn't be null, and we want an
error when it is. Tests shouldn't interfere with prod behavior.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:122: // We destroy
|directive_handler_| before |rpc_handler_|, hence passing
On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> On 2014/11/06 17:28:23, Charlie wrote:
> > It's the other way around. Class members are constructed in the order listed
> and
> > then destructed in reverse order.
> > 
> > However, this shouldn't be an issue unless the callback is invoked off the
the
> > browser thread, or from the DirectiveHandler destructor. The same will be
true
> > for any two objects that are nested or have a common owner.
> 
> I am really uncomfortable with having classes depend on a class that depends
on
> them. At this time though, I am comfortable with this as long as we never use
> the directive handler object from the rpc handler's destructor. Not calling
this
> callback from only the directive handler's dtor is not the only restriction.
> Even the dtor of the AudioManager cannot call this callback. So this is a
> cleaner solution than placing this restriction on a destructor several levels
> lower on the dependency chain. 
> 
> Changed the declaration order of |rpc_handler_| and |directive_handler_| to
> reflect this change and added a comment to the rpc_handler's destructor.

Our classes are going to have to pass messages back and forth to each other. We
should just find a way to deal with callback cycles.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:44: // TODO(rkc): Too many
audio specific functions here, find a better way to
On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> On 2014/11/06 17:28:23, Charlie wrote:
> > CopresenceManagerImpl::CheckForAudio() probably belongs in the AudioManager.
> No
> > time to fix it in this CL though.
> 
> It doesn't solve the problem. If we put it in the audio manager, we'd need to
> plumb down a callback to report errors; so the net result will be the same. We
> have too much coupling between the audio manager (a component several levels
> down the chain) and the copresence manager.

I don't actually think the AudioManager should be buried like this. Objects that
are hidden inside others should be there because they're an implementation
detail, like the vectors we use to store queues. But it's turning out that the
AudioManager is more than an implementation detail of directives. So it probably
belongs under CopresenceManager directly.

https://codereview.chromium.org/704923002/diff/60001/components/copresence/te...
File components/copresence/test/stub_whispernet_client.cc (right):

https://codereview.chromium.org/704923002/diff/60001/components/copresence/te...
components/copresence/test/stub_whispernet_client.cc:13:
tokens_.push_back(AudioToken("abcdef", true));
The test should pass these into a separate 2-argument constructor. It makes the
test easier to read.

[rkc, 2014-11-06 21:58:34]

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
File components/copresence/handlers/audio/audio_directive_handler_unittest.cc
(right):

https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:56:
directive_handler_->Initialize(NULL, TokensCallback());
On 2014/11/06 21:53:14, Charlie wrote:
> On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> > On 2014/11/06 17:28:22, Charlie wrote:
> > > Pass a StubWhispernetClient here instead (owned by a local scoped_ptr).
And
> > > DCHECK whispernet in the AudioManager.
> > 
> > Why do I need to pass in a Stub to DCHECK in the AudioManager?
> 
> In the real implementation, WhispernetClient shouldn't be null, and we want an
> error when it is. Tests shouldn't interfere with prod behavior.

We will never get to the point that Whispernet will be used. That is the whole
point of having a stub. If at any point this behavior changes, this *test* will
crash, so we should be fine.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:122: // We destroy
|directive_handler_| before |rpc_handler_|, hence passing
On 2014/11/06 21:53:14, Charlie wrote:
> On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> > On 2014/11/06 17:28:23, Charlie wrote:
> > > It's the other way around. Class members are constructed in the order
listed
> > and
> > > then destructed in reverse order.
> > > 
> > > However, this shouldn't be an issue unless the callback is invoked off the
> the
> > > browser thread, or from the DirectiveHandler destructor. The same will be
> true
> > > for any two objects that are nested or have a common owner.
> > 
> > I am really uncomfortable with having classes depend on a class that depends
> on
> > them. At this time though, I am comfortable with this as long as we never
use
> > the directive handler object from the rpc handler's destructor. Not calling
> this
> > callback from only the directive handler's dtor is not the only restriction.
> > Even the dtor of the AudioManager cannot call this callback. So this is a
> > cleaner solution than placing this restriction on a destructor several
levels
> > lower on the dependency chain. 
> > 
> > Changed the declaration order of |rpc_handler_| and |directive_handler_| to
> > reflect this change and added a comment to the rpc_handler's destructor.
> 
> Our classes are going to have to pass messages back and forth to each other.
We
> should just find a way to deal with callback cycles.

Acknowledged.

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

https://codereview.chromium.org/704923002/diff/20001/components/copresence/ha...
components/copresence/handlers/directive_handler.h:44: // TODO(rkc): Too many
audio specific functions here, find a better way to
On 2014/11/06 21:53:14, Charlie wrote:
> On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> > On 2014/11/06 17:28:23, Charlie wrote:
> > > CopresenceManagerImpl::CheckForAudio() probably belongs in the
AudioManager.
> > No
> > > time to fix it in this CL though.
> > 
> > It doesn't solve the problem. If we put it in the audio manager, we'd need
to
> > plumb down a callback to report errors; so the net result will be the same.
We
> > have too much coupling between the audio manager (a component several levels
> > down the chain) and the copresence manager.
> 
> I don't actually think the AudioManager should be buried like this. Objects
that
> are hidden inside others should be there because they're an implementation
> detail, like the vectors we use to store queues. But it's turning out that the
> AudioManager is more than an implementation detail of directives. So it
probably
> belongs under CopresenceManager directly.

We can discuss this offline if you wish. I think the directive handler should
abstract its individual discovery medium handlers - of course, this is not
happening too well.

https://codereview.chromium.org/704923002/diff/60001/components/copresence/te...
File components/copresence/test/stub_whispernet_client.cc (right):

https://codereview.chromium.org/704923002/diff/60001/components/copresence/te...
components/copresence/test/stub_whispernet_client.cc:13:
tokens_.push_back(AudioToken("abcdef", true));
On 2014/11/06 21:53:14, Charlie wrote:
> The test should pass these into a separate 2-argument constructor. It makes
the
> test easier to read.

This is a stub class. This is a test. I am not sure if changing this is truly
going to effect the quality of the code.

[Charlie, 2014-11-06 22:00:42]

On 2014/11/06 21:58:34, Rahul Chaturvedi wrote:
>
https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
> File components/copresence/handlers/audio/audio_directive_handler_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/704923002/diff/1/components/copresence/handle...
> components/copresence/handlers/audio/audio_directive_handler_unittest.cc:56:
> directive_handler_->Initialize(NULL, TokensCallback());
> On 2014/11/06 21:53:14, Charlie wrote:
> > On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> > > On 2014/11/06 17:28:22, Charlie wrote:
> > > > Pass a StubWhispernetClient here instead (owned by a local scoped_ptr).
> And
> > > > DCHECK whispernet in the AudioManager.
> > > 
> > > Why do I need to pass in a Stub to DCHECK in the AudioManager?
> > 
> > In the real implementation, WhispernetClient shouldn't be null, and we want
an
> > error when it is. Tests shouldn't interfere with prod behavior.
> 
> We will never get to the point that Whispernet will be used. That is the whole
> point of having a stub. If at any point this behavior changes, this *test*
will
> crash, so we should be fine.
> 
>
https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
> File components/copresence/copresence_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/704923002/diff/20001/components/copresence/co...
> components/copresence/copresence_manager_impl.cc:122: // We destroy
> |directive_handler_| before |rpc_handler_|, hence passing
> On 2014/11/06 21:53:14, Charlie wrote:
> > On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> > > On 2014/11/06 17:28:23, Charlie wrote:
> > > > It's the other way around. Class members are constructed in the order
> listed
> > > and
> > > > then destructed in reverse order.
> > > > 
> > > > However, this shouldn't be an issue unless the callback is invoked off
the
> > the
> > > > browser thread, or from the DirectiveHandler destructor. The same will
be
> > true
> > > > for any two objects that are nested or have a common owner.
> > > 
> > > I am really uncomfortable with having classes depend on a class that
depends
> > on
> > > them. At this time though, I am comfortable with this as long as we never
> use
> > > the directive handler object from the rpc handler's destructor. Not
calling
> > this
> > > callback from only the directive handler's dtor is not the only
restriction.
> > > Even the dtor of the AudioManager cannot call this callback. So this is a
> > > cleaner solution than placing this restriction on a destructor several
> levels
> > > lower on the dependency chain. 
> > > 
> > > Changed the declaration order of |rpc_handler_| and |directive_handler_|
to
> > > reflect this change and added a comment to the rpc_handler's destructor.
> > 
> > Our classes are going to have to pass messages back and forth to each other.
> We
> > should just find a way to deal with callback cycles.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/704923002/diff/20001/components/copresence/ha...
> File components/copresence/handlers/directive_handler.h (right):
> 
>
https://codereview.chromium.org/704923002/diff/20001/components/copresence/ha...
> components/copresence/handlers/directive_handler.h:44: // TODO(rkc): Too many
> audio specific functions here, find a better way to
> On 2014/11/06 21:53:14, Charlie wrote:
> > On 2014/11/06 19:58:24, Rahul Chaturvedi wrote:
> > > On 2014/11/06 17:28:23, Charlie wrote:
> > > > CopresenceManagerImpl::CheckForAudio() probably belongs in the
> AudioManager.
> > > No
> > > > time to fix it in this CL though.
> > > 
> > > It doesn't solve the problem. If we put it in the audio manager, we'd need
> to
> > > plumb down a callback to report errors; so the net result will be the
same.
> We
> > > have too much coupling between the audio manager (a component several
levels
> > > down the chain) and the copresence manager.
> > 
> > I don't actually think the AudioManager should be buried like this. Objects
> that
> > are hidden inside others should be there because they're an implementation
> > detail, like the vectors we use to store queues. But it's turning out that
the
> > AudioManager is more than an implementation detail of directives. So it
> probably
> > belongs under CopresenceManager directly.
> 
> We can discuss this offline if you wish. I think the directive handler should
> abstract its individual discovery medium handlers - of course, this is not
> happening too well.
> 
>
https://codereview.chromium.org/704923002/diff/60001/components/copresence/te...
> File components/copresence/test/stub_whispernet_client.cc (right):
> 
>
https://codereview.chromium.org/704923002/diff/60001/components/copresence/te...
> components/copresence/test/stub_whispernet_client.cc:13:
> tokens_.push_back(AudioToken("abcdef", true));
> On 2014/11/06 21:53:14, Charlie wrote:
> > The test should pass these into a separate 2-argument constructor. It makes
> the
> > test easier to read.
> 
> This is a stub class. This is a test. I am not sure if changing this is truly
> going to effect the quality of the code.

Sure, I can fix it next week after M40 has been cut. LGTM.

[rkc, 2014-11-06 22:01:39]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-11-06 22:03:17]

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

[commit-bot: I haz the power, 2014-11-06 22:59:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-06 23:00:19]

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