Adding CopresenceState

Adding CopresenceState, a central place to track the internal state of the copresence component. Classes can register to be notified of state changes (e.g. new directives or tokens). This provides the backend for the new debug UI.

BUG=420889
Committed: https://crrev.com/e8b0dbdbe00fd4cdfcc7db4fd4721ed74be4f928
Cr-Commit-Position: refs/heads/master@{#309315}

[Charlie, 2014-12-05 03:03:11]

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

[Charlie, 2014-12-05 03:03:12]

Still working on fixing the existing tests and adding a
copresence_state_unittest. But this is now successfully driving the debug UI,
and will be hooked up to a debug API in January. Let me know what you think.

[rkc, 2014-12-09 18:28:30]

Overall design looks good, I have a few nits and comments. I'll send those when
I do a detailed review later today.

[Charlie, 2014-12-16 20:37:37]

PTAL

[Charlie, 2014-12-18 17:09:36]

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

[Charlie, 2014-12-18 17:09:37]

reviewers += xiyuan in case rkc doesn't have time for a detailed review. Let's
get this checked in.

[xiyuan, 2014-12-18 18:10:52]

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
File components/copresence/copresence_state_impl.cc (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
components/copresence/copresence_state_impl.cc:93: UpdateToken(token,
&transmitted_tokens_[token.id]);
General question: when would a token be removed from |transmitted_tokens_| and
|received_tokens_|? It seems tokens are only added. Would this be a problem?

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
File components/copresence/copresence_state_impl.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
components/copresence/copresence_state_impl.h:64: std::set<CopresenceObserver*>
observers_;
Why not use ObserverList in base/observer_list.h?

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.h
(right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.h:48:
~AudioDirectiveHandlerImpl();
nit: override?

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

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:36: 
nit: nuke one empty line

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
File components/copresence/handlers/directive_handler_unittest.cc (left):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
components/copresence/handlers/directive_handler_unittest.cc:15: namespace {
nit: keep the anonymous namespace?

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
File components/copresence/public/copresence_observer.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
components/copresence/public/copresence_observer.h:10: #include <vector>
nit: The above 3 headers seems not used.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
components/copresence/public/copresence_observer.h:39:
DISALLOW_COPY_AND_ASSIGN(CopresenceObserver);
nit: usually we don't use DISALLOW_COPY_AND_ASSIGN and have default ctor for
interface,

e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/ash/shell_observer.h

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
File components/copresence/public/copresence_state.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
components/copresence/public/copresence_state.h:34: virtual const
std::vector<Directive>& active_directives() = 0;
nit: Can this and the two accessors below be const methods?

[rkc, 2014-12-18 18:12:10]

https://codereview.chromium.org/764673003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api_unittest.cc
(right):

https://codereview.chromium.org/764673003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api_unittest.cc:79:
state_(new CopresenceStateImpl) {}
Why aren't we using a stub here at the moment?

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
File components/copresence/copresence_state_impl.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
components/copresence/copresence_state_impl.h:17: // TODO(ckehoe): Write tests.
We should put in at least some basic unit tests, even if not extensive.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/to...
File components/copresence/tokens.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/to...
components/copresence/tokens.h:24: // It's an error to define these constructors
inline,
The AudioToken ctor seems to be fine being declared inline. What's the
difference with this struct?

[Charlie, 2014-12-19 03:53:34]

https://codereview.chromium.org/764673003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api_unittest.cc
(right):

https://codereview.chromium.org/764673003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api_unittest.cc:79:
state_(new CopresenceStateImpl) {}
On 2014/12/18 18:12:09, Rahul Chaturvedi wrote:
> Why aren't we using a stub here at the moment?

Because Andrew doesn't want me to spend a lot of time on the debug UI :-)

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
File components/copresence/copresence_state_impl.cc (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
components/copresence/copresence_state_impl.cc:93: UpdateToken(token,
&transmitted_tokens_[token.id]);
On 2014/12/18 18:10:51, xiyuan wrote:
> General question: when would a token be removed from |transmitted_tokens_| and
> |received_tokens_|? It seems tokens are only added. Would this be a problem?

Currently this doesn't happen. This is useful to a point, since past tokens are
still useful info. Added a TODO to limit the number that accumulate.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
File components/copresence/copresence_state_impl.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
components/copresence/copresence_state_impl.h:17: // TODO(ckehoe): Write tests.
On 2014/12/18 18:12:09, Rahul Chaturvedi wrote:
> We should put in at least some basic unit tests, even if not extensive.

Done.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/co...
components/copresence/copresence_state_impl.h:64: std::set<CopresenceObserver*>
observers_;
On 2014/12/18 18:10:52, xiyuan wrote:
> Why not use ObserverList in base/observer_list.h?

Thanks! Done.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler_impl.h
(right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_impl.h:48:
~AudioDirectiveHandlerImpl();
On 2014/12/18 18:10:52, xiyuan wrote:
> nit: override?

Done.

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

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler_unittest.cc:36: 
On 2014/12/18 18:10:52, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
File components/copresence/handlers/directive_handler_unittest.cc (left):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/ha...
components/copresence/handlers/directive_handler_unittest.cc:15: namespace {
On 2014/12/18 18:10:52, xiyuan wrote:
> nit: keep the anonymous namespace?

Done.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
File components/copresence/public/copresence_observer.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
components/copresence/public/copresence_observer.h:10: #include <vector>
On 2014/12/18 18:10:52, xiyuan wrote:
> nit: The above 3 headers seems not used.

Done.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
components/copresence/public/copresence_observer.h:39:
DISALLOW_COPY_AND_ASSIGN(CopresenceObserver);
On 2014/12/18 18:10:52, xiyuan wrote:
> nit: usually we don't use DISALLOW_COPY_AND_ASSIGN and have default ctor for
> interface,
> 
> e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/shell_observer.h

Done.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
File components/copresence/public/copresence_state.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/pu...
components/copresence/public/copresence_state.h:34: virtual const
std::vector<Directive>& active_directives() = 0;
On 2014/12/18 18:10:52, xiyuan wrote:
> nit: Can this and the two accessors below be const methods?

Done.

https://codereview.chromium.org/764673003/diff/60001/components/copresence/to...
File components/copresence/tokens.h (right):

https://codereview.chromium.org/764673003/diff/60001/components/copresence/to...
components/copresence/tokens.h:24: // It's an error to define these constructors
inline,
On 2014/12/18 18:12:09, Rahul Chaturvedi wrote:
> The AudioToken ctor seems to be fine being declared inline. What's the
> difference with this struct?

If you don't pass in every data member explicitly, their default constructors
get called instead. Which apparently counts as "complex".

[xiyuan, 2014-12-19 04:03:02]

lgtm

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
File components/copresence/copresence_state_impl.h (right):

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_impl.h:38: const override;
nit: wrap transimitted_tokens() as well? It feels a bit strange to have "const
override" by their own. Is this what "git cl format" generates?

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
File components/copresence/copresence_state_unittest.cc (right):

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_unittest.cc:34: }
nit: }  // namespace protobuf

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_unittest.cc:35: }
nit: }  // namespace google

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_unittest.cc:81:
directive_notifications_++;
nit: ++directive_notifications_;

[Charlie, 2014-12-19 05:25:39]

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
File components/copresence/copresence_state_impl.h (right):

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_impl.h:38: const override;
On 2014/12/19 04:03:01, xiyuan wrote:
> nit: wrap transimitted_tokens() as well? It feels a bit strange to have "const
> override" by their own. Is this what "git cl format" generates?

No I just made it up. Done.

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
File components/copresence/copresence_state_unittest.cc (right):

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_unittest.cc:34: }
On 2014/12/19 04:03:02, xiyuan wrote:
> nit: }  // namespace protobuf

Done.

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_unittest.cc:35: }
On 2014/12/19 04:03:02, xiyuan wrote:
> nit: }  // namespace google

Done.

https://codereview.chromium.org/764673003/diff/80001/components/copresence/co...
components/copresence/copresence_state_unittest.cc:81:
directive_notifications_++;
On 2014/12/19 04:03:01, xiyuan wrote:
> nit: ++directive_notifications_;

Style guide says this can be either before or after :-)

[Charlie, 2014-12-19 07:06:44]

https://codereview.chromium.org/764673003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api_unittest.cc
(right):

https://codereview.chromium.org/764673003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api_unittest.cc:79:
state_(new CopresenceStateImpl) {}
On 2014/12/19 03:53:33, Charlie wrote:
> On 2014/12/18 18:12:09, Rahul Chaturvedi wrote:
> > Why aren't we using a stub here at the moment?
> 
> Because Andrew doesn't want me to spend a lot of time on the debug UI :-)

Nevermind, I realized we actually don't need this at all.

[rkc, 2014-12-19 19:09:04]

lgtm

[Charlie, 2014-12-19 22:49:43]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-12-19 22:50:30]

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

[commit-bot: I haz the power, 2014-12-20 00:50:55]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-12-20 00:52:02]

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

[benwells, 2014-12-22 07:15:33]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/824593003/ by benwells@chromium.org.

The reason for reverting is: AudioDirectiveListTest.AddDirectiveMultiple started
failing consistently on the linux valgrind memory fyi bot after this change.

See
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...
for details.

I can't see any good reason why it would be failing in this way, on this bot,
after this change, but I don't have time to investigate deeply..