Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
637223011:
Redesign the copresence audio handlers. (Closed)
DescriptionRedesign the copresence audio handlers.
Since playback and recording were baked directly into the audio directive
handling code, it is really hard to implement features that control audio
playback independent of directive management - for example, carrier sense,
audio playback verification, etc.
This CL first, introduces a new class which abstracts the details of actual
playback and recording, then it refactors and redesigns a lot of the audio
code to make directive handling much simpler and makes the audible and
inaudible audio handling much cleaner. This also fixes the issue of us always
decoding both audible and inaudible tokens at the same time, despite the kind
of token we got.
Reviews requested,
derat, xiyuan - general
kalman - copresence_private.idl
R=derat@chromium.org, kalman@chromium.org, xiyuan@chromium.org
BUG=407621, 390388, 388499
Committed: https://crrev.com/a05223cb296cb5301f043d10867c027aa56ebff3
Cr-Commit-Position: refs/heads/master@{#300825}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 72
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 23
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #Patch Set 9 : #
Total comments: 47
Patch Set 10 : #
Total comments: 69
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : build fixes #Patch Set 15 : #Messages
Total messages: 59 (8 generated)
I'm just going to review the IDL. You should be an owner of the API implementations so I don't need to look at those. https://codereview.chromium.org/637223011/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/copresence_private.idl (right): https://codereview.chromium.org/637223011/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/copresence_private.idl:10: enum DecodeRequestType { audible, inaudible, both }; This will only ever be audible/inaudible, right? Because 'both' could easily become meaningless. I think a better model for this would be an object with boolean keys audible and inaudible, and not have an enum at all. It doesn't really feel like an enum. https://codereview.chromium.org/637223011/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/copresence_private.idl:53: static void onDecodeSamplesRequest(DecodeRequestType type, This isn't backwards compatible? Not a problem right at this moment given only a built-in extension uses it, but it's a bad habit to get into. Either - put the new argument at the end - make whole single argument a choice between an object (containing both type and audioSamples as keys) OR audioSamples, then expect callers to detect. The former is probably easier, doesn't scale very well obviously, but it's a private API anyway.
https://codereview.chromium.org/637223011/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/copresence_private.idl (right): https://codereview.chromium.org/637223011/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/copresence_private.idl:10: enum DecodeRequestType { audible, inaudible, both }; On 2014/10/17 20:17:57, kalman wrote: > This will only ever be audible/inaudible, right? Because 'both' could easily > become meaningless. I think a better model for this would be an object with > boolean keys audible and inaudible, and not have an enum at all. It doesn't > really feel like an enum. Done. https://codereview.chromium.org/637223011/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/copresence_private.idl:53: static void onDecodeSamplesRequest(DecodeRequestType type, On 2014/10/17 20:17:57, kalman wrote: > This isn't backwards compatible? Not a problem right at this moment given only a > built-in extension uses it, but it's a bad habit to get into. > > Either > - put the new argument at the end > - make whole single argument a choice between an object (containing both type > and audioSamples as keys) OR audioSamples, then expect callers to detect. > > The former is probably easier, doesn't scale very well obviously, but it's a > private API anyway. Done.
my comments are all on patch set 2 https://codereview.chromium.org/637223011/diff/20001/chrome/browser/copresenc... File chrome/browser/copresence/chrome_whispernet_client.cc (right): https://codereview.chromium.org/637223011/diff/20001/chrome/browser/copresenc... chrome/browser/copresence/chrome_whispernet_client.cc:90: switch (type) { you could get compile-time checking that all enum values are covered if you moved this into a helper function in an anon namespace: extensions::api::copresence_private::DecodeRequestType GetDecodeRequestTypeForAudioType(copresence::AudioType audio_type) { switch (audio_type) { case copresence::AUDIBLE: return extensions::api::copresence_private::DECODE_REQUEST_TYPE_AUDIBLE; case ... } NOTREACHED() << "Invalid audio type " << audio_type; } https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... File chrome/browser/resources/whispernet_proxy/js/init.js (right): https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... chrome/browser/resources/whispernet_proxy/js/init.js:53: * @param {string} type Type of decoding to perform. document the acceptable values (or at least that this is DecodeRequestType)? https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... File chrome/browser/resources/whispernet_proxy/js/wrapper.js (right): https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... chrome/browser/resources/whispernet_proxy/js/wrapper.js:147: * @param {string} type Type of decoding to perform. same here https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... File components/copresence/copresence_manager_impl.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... components/copresence/copresence_manager_impl.cc:67: // for (PendingRequest& request : pending_requests_queue_) { delete this comment https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:20: : audio_manager_(make_scoped_ptr<AudioManager>(NULL)) { don't think you need this; scoped_ptr's default c'tor initializes to NULL https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:94: if (transmits_list_[AUDIBLE].GetActiveDirective()) { nit: omit curly brackets here and below https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:131: // We cache now, since, calling Now() twice doesn't gaurantee that the time nit: guarantee https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:141: if (transmits_list_[INAUDIBLE].GetActiveDirective()) { how about moving this into a helper function so it doesn't need to be duplicated over and over? https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:26: const EncodeTokenCallback&) override{}; nit: space after 'override' and no trailing semicolon https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:27: nit: delete blank line https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:31: here too https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:35: here too https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:39: const std::string& CurrentAudioToken(AudioType type) const; nit: rename to GetCurrentAudioToken? https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:45: : playing_{false, false}, has this style been approved? https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:164: if (recording_[AUDIBLE] && recording_[INAUDIBLE]) overwriting the previous values looks a bit weird. maybe just do something like this? if (recording_[AUDIBLE]) decode_type = recording_[INAUDIBLE] ? BOTH : AUDIBLE; else if (recording_[INAUDIBLE]) decode_type = INAUDIBLE; https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:59: const std::string& GetToken(AudioType type) { return token_[type]; } nit: rename to get_token() since it's a simple inlined method https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:61: // For test. nit: don't need this comment; method names already have _for_testing in them https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:65: nit: i'd probably delete all the blank lines between these setters https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:113: // 0 = AUDIBLE, 1 = INAUDIBLE. please instead just document the name of the enum that's used to index into these. you should probably also add compile-time assertions that their values are less than 2. https://codereview.chromium.org/637223011/diff/20001/components/copresence/pr... File components/copresence/proto/enums.proto (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/pr... components/copresence/proto/enums.proto:84: AUDIBLE_AUDIO = 1; AUDIO_CONFIGURATION_AUDIBLE? for the purposes of avoiding conflicts, it's nice to have the type in the value name. https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... components/copresence/public/copresence_constants.h:30: // These constants are used from, everywhere. s/,// https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... File components/copresence/public/whispernet_client.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... components/copresence/public/whispernet_client.h:33: class WhispernetClient { why'd the virtual destructor for this interface get deleted?
ckehoe@chromium.org changed reviewers: + ckehoe@chromium.org
https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... File components/copresence/copresence_manager_impl.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... components/copresence/copresence_manager_impl.cc:67: // for (PendingRequest& request : pending_requests_queue_) { ? https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { We probably shouldn't be subclassing production code to create a mock for testing. This makes it unclear when the test uses the production behavior/state, and when it uses the mock behavior. The gMock guide suggests mocking against interfaces: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... See also: http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes So we would have an AudioManager interface, and the real implementation would live in AudioManagerImpl. This forces you to call out exactly the AudioManager behavior you're depending on, and would also allow you to remove the set_*_for_testing methods. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:49: class AudioDirectiveHandlerTest : public testing::Test { I know this is a pain, but we really should add a test with a fake timer to ensure that the directives are being timed out correctly. This seems not to be working in the current implementation. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:81: return directive_handler_->audio_manager_->is_playing_for_testing(type); These _for_testing methods, too, really only belong in the mock/stub. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:88: if (samples_cache_[type]->HasKey(token_[type]) && This looks like silent failure. Add a comment explaining why we sometimes call this when the encoded token isn't available yet, and how UpdateToken() will re-invoke it when encoding is done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:118: DCHECK_LT(type, BOTH); Why not DCHECK_NE? https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:164: if (recording_[AUDIBLE] && recording_[INAUDIBLE]) On 2014/10/17 22:26:00, Daniel Erat wrote: > overwriting the previous values looks a bit weird. maybe just do something like > this? > > if (recording_[AUDIBLE]) > decode_type = recording_[INAUDIBLE] ? BOTH : AUDIBLE; > else if (recording_[INAUDIBLE]) > decode_type = INAUDIBLE; Or: if (recording_[AUDIBLE] && recording_[INAUDIBLE]) decode_type = BOTH; else if (recording_[AUDIBLE]) decode_type = AUDIBLE; else if (recording_[INAUDIBLE]) decode_type = INAUDIBLE; https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:31: // The AudioManager class will record audio until told to stop. Provide some more detail. What is this class responsible for? What functionality does it add on top of the bare player and recorder? https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:48: virtual void Initialize(const DecodeSamplesCallback& decode_cb, In this new class, don't use the "new Foo(); foo->Initialize()" pattern. Instead make the constructor private and provide a static Create() method. See crbug/424735. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:61: // For test. The first two of these can be folded into the Create() method. The rest belong in MockAudioManager. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager_unittest.cc:130: // No 'audible' token yet, so player still shouldn't be playing. *audible*
https://codereview.chromium.org/637223011/diff/20001/chrome/browser/copresenc... File chrome/browser/copresence/chrome_whispernet_client.cc (right): https://codereview.chromium.org/637223011/diff/20001/chrome/browser/copresenc... chrome/browser/copresence/chrome_whispernet_client.cc:90: switch (type) { On 2014/10/17 22:25:59, Daniel Erat wrote: > you could get compile-time checking that all enum values are covered if you > moved this into a helper function in an anon namespace: > > extensions::api::copresence_private::DecodeRequestType > GetDecodeRequestTypeForAudioType(copresence::AudioType audio_type) { > switch (audio_type) { > case copresence::AUDIBLE: > return extensions::api::copresence_private::DECODE_REQUEST_TYPE_AUDIBLE; > case ... > } > NOTREACHED() << "Invalid audio type " << audio_type; > } Code removed with Patch 3. https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... File chrome/browser/resources/whispernet_proxy/js/init.js (right): https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... chrome/browser/resources/whispernet_proxy/js/init.js:53: * @param {string} type Type of decoding to perform. On 2014/10/17 22:25:59, Daniel Erat wrote: > document the acceptable values (or at least that this is DecodeRequestType)? Code changed. This is now an object. https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... File chrome/browser/resources/whispernet_proxy/js/wrapper.js (right): https://codereview.chromium.org/637223011/diff/20001/chrome/browser/resources... chrome/browser/resources/whispernet_proxy/js/wrapper.js:147: * @param {string} type Type of decoding to perform. On 2014/10/17 22:25:59, Daniel Erat wrote: > same here Ditto. https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... File components/copresence/copresence_manager_impl.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... components/copresence/copresence_manager_impl.cc:67: // for (PendingRequest& request : pending_requests_queue_) { On 2014/10/17 22:42:25, Charlie wrote: > ? Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... components/copresence/copresence_manager_impl.cc:67: // for (PendingRequest& request : pending_requests_queue_) { On 2014/10/17 22:25:59, Daniel Erat wrote: > delete this comment Whoops, leftover from debugging. Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:20: : audio_manager_(make_scoped_ptr<AudioManager>(NULL)) { On 2014/10/17 22:25:59, Daniel Erat wrote: > don't think you need this; scoped_ptr's default c'tor initializes to NULL Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:94: if (transmits_list_[AUDIBLE].GetActiveDirective()) { On 2014/10/17 22:25:59, Daniel Erat wrote: > nit: omit curly brackets here and below Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:131: // We cache now, since, calling Now() twice doesn't gaurantee that the time On 2014/10/17 22:25:59, Daniel Erat wrote: > nit: guarantee Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:141: if (transmits_list_[INAUDIBLE].GetActiveDirective()) { On 2014/10/17 22:25:59, Daniel Erat wrote: > how about moving this into a helper function so it doesn't need to be duplicated > over and over? Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { On 2014/10/17 22:42:25, Charlie wrote: > We probably shouldn't be subclassing production code to create a mock for > testing. This makes it unclear when the test uses the production behavior/state, > and when it uses the mock behavior. The gMock guide suggests mocking against > interfaces: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... > > See also: > > http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes > > So we would have an AudioManager interface, and the real implementation would > live in AudioManagerImpl. This forces you to call out exactly the AudioManager > behavior you're depending on, and would also allow you to remove the > set_*_for_testing methods. I agree that deriving a mock from a production class has pitfalls, but there are plenty of cases where deriving from a a concrete class is beneficial - particularly when you want to actually test the production code in the mocked class as well. We can discuss this at length offline if you wish. Leaving as is for now. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:26: const EncodeTokenCallback&) override{}; On 2014/10/17 22:25:59, Daniel Erat wrote: > nit: space after 'override' and no trailing semicolon Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:27: On 2014/10/17 22:25:59, Daniel Erat wrote: > nit: delete blank line Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:31: On 2014/10/17 22:25:59, Daniel Erat wrote: > here too Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:35: On 2014/10/17 22:25:59, Daniel Erat wrote: > here too Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:49: class AudioDirectiveHandlerTest : public testing::Test { On 2014/10/17 22:42:25, Charlie wrote: > I know this is a pain, but we really should add a test with a fake timer to > ensure that the directives are being timed out correctly. This seems not to be > working in the current implementation. This is actually tested in the directive handler; but you're right, there should be a test for that here too. Added a TODO to get add this test. It will take non-trivial changes, I'd prefer to do them in another CL. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:81: return directive_handler_->audio_manager_->is_playing_for_testing(type); On 2014/10/17 22:42:25, Charlie wrote: > These _for_testing methods, too, really only belong in the mock/stub. Acknowledged. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:39: const std::string& CurrentAudioToken(AudioType type) const; On 2014/10/17 22:26:00, Daniel Erat wrote: > nit: rename to GetCurrentAudioToken? Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:45: : playing_{false, false}, On 2014/10/17 22:26:00, Daniel Erat wrote: > has this style been approved? I thought this works with the C++03 standard onwards? I couldn't find this listed on http://chromium-cpp.appspot.com/ under approved, banned or tbd sections. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:88: if (samples_cache_[type]->HasKey(token_[type]) && On 2014/10/17 22:42:26, Charlie wrote: > This looks like silent failure. Add a comment explaining why we sometimes call > this when the encoded token isn't available yet, and how UpdateToken() will > re-invoke it when encoding is done. Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:118: DCHECK_LT(type, BOTH); On 2014/10/17 22:42:25, Charlie wrote: > Why not DCHECK_NE? We specifically want to check that the type is not greater than 1, so LT seems like the more natural check. Plus type could be UNKNOWN :) https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:164: if (recording_[AUDIBLE] && recording_[INAUDIBLE]) On 2014/10/17 22:26:00, Daniel Erat wrote: > overwriting the previous values looks a bit weird. maybe just do something like > this? > > if (recording_[AUDIBLE]) > decode_type = recording_[INAUDIBLE] ? BOTH : AUDIBLE; > else if (recording_[INAUDIBLE]) > decode_type = INAUDIBLE; Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:164: if (recording_[AUDIBLE] && recording_[INAUDIBLE]) On 2014/10/17 22:42:26, Charlie wrote: > On 2014/10/17 22:26:00, Daniel Erat wrote: > > overwriting the previous values looks a bit weird. maybe just do something > like > > this? > > > > if (recording_[AUDIBLE]) > > decode_type = recording_[INAUDIBLE] ? BOTH : AUDIBLE; > > else if (recording_[INAUDIBLE]) > > decode_type = INAUDIBLE; > > Or: > > if (recording_[AUDIBLE] && recording_[INAUDIBLE]) > decode_type = BOTH; > else if (recording_[AUDIBLE]) > decode_type = AUDIBLE; > else if (recording_[INAUDIBLE]) > decode_type = INAUDIBLE; > Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:31: // The AudioManager class will record audio until told to stop. On 2014/10/17 22:42:26, Charlie wrote: > Provide some more detail. What is this class responsible for? What functionality > does it add on top of the bare player and recorder? Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:48: virtual void Initialize(const DecodeSamplesCallback& decode_cb, On 2014/10/17 22:42:26, Charlie wrote: > In this new class, don't use the "new Foo(); foo->Initialize()" pattern. Instead > make the constructor private and provide a static Create() method. See > crbug/424735. I am not quite convinced that is the way to go. Again, let's discuss this offline. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:59: const std::string& GetToken(AudioType type) { return token_[type]; } On 2014/10/17 22:26:00, Daniel Erat wrote: > nit: rename to get_token() since it's a simple inlined method Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:61: // For test. On 2014/10/17 22:26:00, Daniel Erat wrote: > nit: don't need this comment; method names already have _for_testing in them Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:61: // For test. On 2014/10/17 22:42:26, Charlie wrote: > The first two of these can be folded into the Create() method. The rest belong > in MockAudioManager. Acknowledged. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:65: On 2014/10/17 22:26:00, Daniel Erat wrote: > nit: i'd probably delete all the blank lines between these setters Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.h:113: // 0 = AUDIBLE, 1 = INAUDIBLE. On 2014/10/17 22:26:00, Daniel Erat wrote: > please instead just document the name of the enum that's used to index into > these. you should probably also add compile-time assertions that their values > are less than 2. Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager_unittest.cc:130: // No 'audible' token yet, so player still shouldn't be playing. On 2014/10/17 22:42:26, Charlie wrote: > *audible* Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/pr... File components/copresence/proto/enums.proto (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/pr... components/copresence/proto/enums.proto:84: AUDIBLE_AUDIO = 1; On 2014/10/17 22:26:00, Daniel Erat wrote: > AUDIO_CONFIGURATION_AUDIBLE? for the purposes of avoiding conflicts, it's nice > to have the type in the value name. Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... components/copresence/public/copresence_constants.h:30: // These constants are used from, everywhere. On 2014/10/17 22:26:00, Daniel Erat wrote: > s/,// Done. https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... File components/copresence/public/whispernet_client.h (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/pu... components/copresence/public/whispernet_client.h:33: class WhispernetClient { On 2014/10/17 22:26:00, Daniel Erat wrote: > why'd the virtual destructor for this interface get deleted? Not sure when or why I removed it, but it shouldn't be. Re-added.
Let's VC about the design questions on Monday. There are bugs filed for them, and they're lower priority than other issues that this CL (hopefully) fixes. So LG from me. Dan? https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... File components/copresence/copresence_manager_impl.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... components/copresence/copresence_manager_impl.cc:67: // for (PendingRequest& request : pending_requests_queue_) { On 2014/10/18 00:21:54, Rahul Chaturvedi wrote: > On 2014/10/17 22:42:25, Charlie wrote: > > ? > > Done. Sorry, why can't we use the range-based version? https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:49: class AudioDirectiveHandlerTest : public testing::Test { On 2014/10/18 00:21:54, Rahul Chaturvedi wrote: > On 2014/10/17 22:42:25, Charlie wrote: > > I know this is a pain, but we really should add a test with a fake timer to > > ensure that the directives are being timed out correctly. This seems not to be > > working in the current implementation. > > This is actually tested in the directive handler; but you're right, there should > be a test for that here too. > Added a TODO to get add this test. It will take non-trivial changes, I'd prefer > to do them in another CL. > I don't follow. This *is* the test for the directive handler. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:118: DCHECK_LT(type, BOTH); On 2014/10/18 00:21:55, Rahul Chaturvedi wrote: > On 2014/10/17 22:42:25, Charlie wrote: > > Why not DCHECK_NE? > > We specifically want to check that the type is not greater than 1, so LT seems > like the more natural check. > Plus type could be UNKNOWN :) Ah, I'm used to the proto convention where UNKNOWN is always 0. Anyway, we don't want to depend on the numeric values of the enum. Do this instead: DCHECK(type == AUDIBLE || type == INAUDIBLE);
https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... File components/copresence/copresence_manager_impl.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/co... components/copresence/copresence_manager_impl.cc:67: // for (PendingRequest& request : pending_requests_queue_) { On 2014/10/18 00:44:11, Charlie wrote: > On 2014/10/18 00:21:54, Rahul Chaturvedi wrote: > > On 2014/10/17 22:42:25, Charlie wrote: > > > ? > > > > Done. > > Sorry, why can't we use the range-based version? Nothing, I had commented it out since I was debugging something and needed to see the index :) I put it back in place. https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:49: class AudioDirectiveHandlerTest : public testing::Test { On 2014/10/18 00:44:11, Charlie wrote: > On 2014/10/18 00:21:54, Rahul Chaturvedi wrote: > > On 2014/10/17 22:42:25, Charlie wrote: > > > I know this is a pain, but we really should add a test with a fake timer to > > > ensure that the directives are being timed out correctly. This seems not to > be > > > working in the current implementation. > > > > This is actually tested in the directive handler; but you're right, there > should > > be a test for that here too. > > Added a TODO to get add this test. It will take non-trivial changes, I'd > prefer > > to do them in another CL. > > > > I don't follow. This *is* the test for the directive handler. In the AudioDirectiveList tests, sorry. https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_manager.cc:118: DCHECK_LT(type, BOTH); On 2014/10/18 00:44:11, Charlie wrote: > On 2014/10/18 00:21:55, Rahul Chaturvedi wrote: > > On 2014/10/17 22:42:25, Charlie wrote: > > > Why not DCHECK_NE? > > > > We specifically want to check that the type is not greater than 1, so LT seems > > like the more natural check. > > Plus type could be UNKNOWN :) > > Ah, I'm used to the proto convention where UNKNOWN is always 0. Anyway, we don't > want to depend on the numeric values of the enum. Do this instead: > > DCHECK(type == AUDIBLE || type == INAUDIBLE); Done.
i didn't do a full pass this time, but i share charlie's concerns about test classes inheriting from the real implementations. please create pure abstract interfaces instead; it makes things much simpler and safer in the long run. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:19: base::Time GetClosestEventTime(AudioDirectiveList* list, nit: GetEarliestEventTime? https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:39: audio_manager_ = make_scoped_ptr(new AudioManager()); just do audio_manager_.reset(new AudioManager); https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:100: if (audio_event_timer_.IsRunning()) you can just call Stop() unconditionally without checking this first https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:136: base::TimeDelta AudioDirectiveHandler::NextInstructionExpiry() { nit: rename to GetNextInstructionExpiry() https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { i agree that you should create an pure virtual AudioManagerInterface instead of selectively overriding methods from the real implementation. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: virtual void Initialize(const DecodeSamplesCallback&, it's cleaner to keep Initialize methods in the implementation(s) rather than putting them in the interface class. different implementations usually need different things to be passed in at creation. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:28: set_playing_for_testing(type, true); it would also be cleaner if this stub class had its own member variables to track the playing/recording state instead of tagging along with the real implementation. if you want to be able to test some of the real AudioManager implementation, please move the code that actually communicates with audio hardware into a delegate class. then you can create a real implementation of the delegate for production and a stub implementation for tests.
https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:19: base::Time GetClosestEventTime(AudioDirectiveList* list, On 2014/10/18 21:21:18, Daniel Erat wrote: > nit: GetEarliestEventTime? Done. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:39: audio_manager_ = make_scoped_ptr(new AudioManager()); On 2014/10/18 21:21:17, Daniel Erat wrote: > just do audio_manager_.reset(new AudioManager); Done. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:100: if (audio_event_timer_.IsRunning()) On 2014/10/18 21:21:17, Daniel Erat wrote: > you can just call Stop() unconditionally without checking this first Done. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:136: base::TimeDelta AudioDirectiveHandler::NextInstructionExpiry() { On 2014/10/18 21:21:17, Daniel Erat wrote: > nit: rename to GetNextInstructionExpiry() Done. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { On 2014/10/18 21:21:18, Daniel Erat wrote: > i agree that you should create an pure virtual AudioManagerInterface instead of > selectively overriding methods from the real implementation. Though I agree that there are benefits to having a stub implementation inheriting from a pure interface, my concern about that pattern aren't with testing the original code (the AudioManager code will be tested separately in its own test anyway). The concern is managing two sets of code to have the same behavior. Currently the AudioManager behavior is simple, but if at a later date, it starts to get more complex (as it definitely will), we now have to update the stub to match the behavior of the original AudioManager. This now means we have two copies of code to maintain, which need to mimic the same behavior, i.e., to ensure that the stub does that, we really should have tests for the stub also. This doesn't sound very optimal. Overriding large parts of the implementation is also undesirable, so I've fixed this by using the original AudioManager class instead. I've changed the AudioPlayer and AudioRecorder classes instead to have a pure interface, and have their stubs inherit from that interface. This behavior duplication is acceptable since it is at the very lowest layer - these classes are mimicking what the Audio hardware would have done instead - something we intentionally want to duplicate. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: virtual void Initialize(const DecodeSamplesCallback&, On 2014/10/18 21:21:18, Daniel Erat wrote: > it's cleaner to keep Initialize methods in the implementation(s) rather than > putting them in the interface class. different implementations usually need > different things to be passed in at creation. I'm confused about how this would work, maybe I am not reading this right. Could you explain what you mean here a bit more? The way I am looking at it; we'll have AudioManagerBase, which will provide the start/stop play/record methods. Then there will be an AudioManagerImpl, which will inherit from AudioManagerBase, that will actually implement these methods, and will provide an additional Initialize method? Which class will AudioDirectiveHandler hold a pointer to? If AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really call Initialize(), since that method won't exist in the interface. If AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't accept any other classes that derive from AudioManagerBase as its audio manager. (since I am actually using the original AudioManager class itself, this point is moot here, but it still applies to the TestAudioPlayer/Recorder classes). https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:28: set_playing_for_testing(type, true); On 2014/10/18 21:21:18, Daniel Erat wrote: > it would also be cleaner if this stub class had its own member variables to > track the playing/recording state instead of tagging along with the real > implementation. > > if you want to be able to test some of the real AudioManager implementation, > please move the code that actually communicates with audio hardware into a > delegate class. then you can create a real implementation of the delegate for > production and a stub implementation for tests. The TestAudioPlayer/Recorder are technically delegates to the code that communicates with the audio hardware.
On 2014/10/20 16:45:38, Rahul Chaturvedi wrote: > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > File components/copresence/handlers/audio/audio_directive_handler.cc (right): > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler.cc:19: base::Time > GetClosestEventTime(AudioDirectiveList* list, > On 2014/10/18 21:21:18, Daniel Erat wrote: > > nit: GetEarliestEventTime? > > Done. > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler.cc:39: > audio_manager_ = make_scoped_ptr(new AudioManager()); > On 2014/10/18 21:21:17, Daniel Erat wrote: > > just do audio_manager_.reset(new AudioManager); > > Done. > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler.cc:100: if > (audio_event_timer_.IsRunning()) > On 2014/10/18 21:21:17, Daniel Erat wrote: > > you can just call Stop() unconditionally without checking this first > > Done. > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler.cc:136: > base::TimeDelta AudioDirectiveHandler::NextInstructionExpiry() { > On 2014/10/18 21:21:17, Daniel Erat wrote: > > nit: rename to GetNextInstructionExpiry() > > Done. > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > File components/copresence/handlers/audio/audio_directive_handler_unittest.cc > (right): > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: > class TestAudioManager : public AudioManager { > On 2014/10/18 21:21:18, Daniel Erat wrote: > > i agree that you should create an pure virtual AudioManagerInterface instead > of > > selectively overriding methods from the real implementation. > > Though I agree that there are benefits to having a stub implementation > inheriting from a pure interface, my concern about that pattern aren't with > testing the original code (the AudioManager code will be tested separately in > its own test anyway). The concern is managing two sets of code to have the same > behavior. Currently the AudioManager behavior is simple, but if at a later date, > it starts to get more complex (as it definitely will), we now have to update the > stub to match the behavior of the original AudioManager. This now means we have > two copies of code to maintain, which need to mimic the same behavior, i.e., to > ensure that the stub does that, we really should have tests for the stub also. > This doesn't sound very optimal. > > Overriding large parts of the implementation is also undesirable, so I've fixed > this by using the original AudioManager class instead. > > I've changed the AudioPlayer and AudioRecorder classes instead to have a pure > interface, and have their stubs inherit from that interface. This behavior > duplication is acceptable since it is at the very lowest layer - these classes > are mimicking what the Audio hardware would have done instead - something we > intentionally want to duplicate. > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: > virtual void Initialize(const DecodeSamplesCallback&, > On 2014/10/18 21:21:18, Daniel Erat wrote: > > it's cleaner to keep Initialize methods in the implementation(s) rather than > > putting them in the interface class. different implementations usually need > > different things to be passed in at creation. > > I'm confused about how this would work, maybe I am not reading this right. Could > you explain what you mean here a bit more? > > The way I am looking at it; we'll have AudioManagerBase, which will provide the > start/stop play/record methods. Then there will be an AudioManagerImpl, which > will inherit from AudioManagerBase, that will actually implement these methods, > and will provide an additional Initialize method? > > Which class will AudioDirectiveHandler hold a pointer to? If > AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really call > Initialize(), since that method won't exist in the interface. If > AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't accept > any other classes that derive from AudioManagerBase as its audio manager. > > (since I am actually using the original AudioManager class itself, this point is > moot here, but it still applies to the TestAudioPlayer/Recorder classes). > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler_unittest.cc:28: > set_playing_for_testing(type, true); > On 2014/10/18 21:21:18, Daniel Erat wrote: > > it would also be cleaner if this stub class had its own member variables to > > track the playing/recording state instead of tagging along with the real > > implementation. > > > > if you want to be able to test some of the real AudioManager implementation, > > please move the code that actually communicates with audio hardware into a > > delegate class. then you can create a real implementation of the delegate for > > production and a stub implementation for tests. > > The TestAudioPlayer/Recorder are technically delegates to the code that > communicates with the audio hardware. thanks for the explanation; i'll take another pass by this afternoon.
IDL lgtm https://codereview.chromium.org/637223011/diff/100001/chrome/common/extension... File chrome/common/extensions/api/copresence_private.idl (right): https://codereview.chromium.org/637223011/diff/100001/chrome/common/extension... chrome/common/extensions/api/copresence_private.idl:11: }; I'd probably call this DecodeSamplesParameters
https://codereview.chromium.org/637223011/diff/100001/chrome/common/extension... File chrome/common/extensions/api/copresence_private.idl (right): https://codereview.chromium.org/637223011/diff/100001/chrome/common/extension... chrome/common/extensions/api/copresence_private.idl:11: }; On 2014/10/20 17:18:11, kalman wrote: > I'd probably call this DecodeSamplesParameters Done.
https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:147: return next_event - now; I don't follow this. |next_event| should be less than |now|, right? What would this produce? https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:127: base::Bind(&AudioManager::OnTokenEncoded, base::Unretained(this))); base::Unretained makes me nervous. Could we use WeakPtr or something?
https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { On 2014/10/20 16:45:37, Rahul Chaturvedi wrote: > On 2014/10/18 21:21:18, Daniel Erat wrote: > > i agree that you should create an pure virtual AudioManagerInterface instead > of > > selectively overriding methods from the real implementation. > > Though I agree that there are benefits to having a stub implementation > inheriting from a pure interface, my concern about that pattern aren't with > testing the original code (the AudioManager code will be tested separately in > its own test anyway). The concern is managing two sets of code to have the same > behavior. Currently the AudioManager behavior is simple, but if at a later date, > it starts to get more complex (as it definitely will), we now have to update the > stub to match the behavior of the original AudioManager. This now means we have > two copies of code to maintain, which need to mimic the same behavior, i.e., to > ensure that the stub does that, we really should have tests for the stub also. > This doesn't sound very optimal. > > Overriding large parts of the implementation is also undesirable, so I've fixed > this by using the original AudioManager class instead. > > I've changed the AudioPlayer and AudioRecorder classes instead to have a pure > interface, and have their stubs inherit from that interface. This behavior > duplication is acceptable since it is at the very lowest layer - these classes > are mimicking what the Audio hardware would have done instead - something we > intentionally want to duplicate. It's a little hard to talk about this without going through all the specifics. But the general idea would be that if you're relying on the full behavior of the AudioManager, then it becomes more of an integration test than a unit test. The idea is exactly that you *shouldn't* have to duplicate AudioManager. You just need a simple stub to verify that this class interacts with it correctly. Yes, as the AudioManagerInterface grows, the stub will become slightly less simple. But it should not track the complexity of the real class. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: virtual void Initialize(const DecodeSamplesCallback&, On 2014/10/20 16:45:38, Rahul Chaturvedi wrote: > On 2014/10/18 21:21:18, Daniel Erat wrote: > > it's cleaner to keep Initialize methods in the implementation(s) rather than > > putting them in the interface class. different implementations usually need > > different things to be passed in at creation. > > I'm confused about how this would work, maybe I am not reading this right. Could > you explain what you mean here a bit more? > > The way I am looking at it; we'll have AudioManagerBase, which will provide the > start/stop play/record methods. Then there will be an AudioManagerImpl, which > will inherit from AudioManagerBase, that will actually implement these methods, > and will provide an additional Initialize method? > > Which class will AudioDirectiveHandler hold a pointer to? If > AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really call > Initialize(), since that method won't exist in the interface. If > AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't accept > any other classes that derive from AudioManagerBase as its audio manager. > > (since I am actually using the original AudioManager class itself, this point is > moot here, but it still applies to the TestAudioPlayer/Recorder classes). Let's call it AudioManager or AudioManagerInterface instead of AudioManagerBase. As for pointer types, one option would be: scoped_ptr<AudioManager> manager = AudioManagerImpl::Create(...); But I'm curious to see what Dan suggests.
https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { On 2014/10/20 20:06:54, Charlie wrote: > On 2014/10/20 16:45:37, Rahul Chaturvedi wrote: > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > i agree that you should create an pure virtual AudioManagerInterface instead > > of > > > selectively overriding methods from the real implementation. > > > > Though I agree that there are benefits to having a stub implementation > > inheriting from a pure interface, my concern about that pattern aren't with > > testing the original code (the AudioManager code will be tested separately in > > its own test anyway). The concern is managing two sets of code to have the > same > > behavior. Currently the AudioManager behavior is simple, but if at a later > date, > > it starts to get more complex (as it definitely will), we now have to update > the > > stub to match the behavior of the original AudioManager. This now means we > have > > two copies of code to maintain, which need to mimic the same behavior, i.e., > to > > ensure that the stub does that, we really should have tests for the stub also. > > This doesn't sound very optimal. > > > > Overriding large parts of the implementation is also undesirable, so I've > fixed > > this by using the original AudioManager class instead. > > > > I've changed the AudioPlayer and AudioRecorder classes instead to have a pure > > interface, and have their stubs inherit from that interface. This behavior > > duplication is acceptable since it is at the very lowest layer - these classes > > are mimicking what the Audio hardware would have done instead - something we > > intentionally want to duplicate. > > It's a little hard to talk about this without going through all the specifics. > But the general idea would be that if you're relying on the full behavior of the > AudioManager, then it becomes more of an integration test than a unit test. > > The idea is exactly that you *shouldn't* have to duplicate AudioManager. You > just need a simple stub to verify that this class interacts with it correctly. > Yes, as the AudioManagerInterface grows, the stub will become slightly less > simple. But it should not track the complexity of the real class. > Its complexity may not grow at the same rate as the implementation, but the complexity will grow. If it is possible to just use the real class instead, why not? https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: virtual void Initialize(const DecodeSamplesCallback&, On 2014/10/20 20:06:54, Charlie wrote: > On 2014/10/20 16:45:38, Rahul Chaturvedi wrote: > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > it's cleaner to keep Initialize methods in the implementation(s) rather than > > > putting them in the interface class. different implementations usually need > > > different things to be passed in at creation. > > > > I'm confused about how this would work, maybe I am not reading this right. > Could > > you explain what you mean here a bit more? > > > > The way I am looking at it; we'll have AudioManagerBase, which will provide > the > > start/stop play/record methods. Then there will be an AudioManagerImpl, which > > will inherit from AudioManagerBase, that will actually implement these > methods, > > and will provide an additional Initialize method? > > > > Which class will AudioDirectiveHandler hold a pointer to? If > > AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really > call > > Initialize(), since that method won't exist in the interface. If > > AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't > accept > > any other classes that derive from AudioManagerBase as its audio manager. > > > > (since I am actually using the original AudioManager class itself, this point > is > > moot here, but it still applies to the TestAudioPlayer/Recorder classes). > > Let's call it AudioManager or AudioManagerInterface instead of AudioManagerBase. > > As for pointer types, one option would be: > > scoped_ptr<AudioManager> manager = AudioManagerImpl::Create(...); > > But I'm curious to see what Dan suggests. > That doesn't fix the issue. The issue is that the container class needs to be able to initialize the derived class, without knowing that it is a derived class (which is what allows us to swap out the implementation with the stub). If the Intialize method or the ::Create method, does not exist in, or is different in, the Stub versus the Impl class, the container class needs to call a specific Impl version of the function. This means that the container class now will need to know which version of the AudioManagerInterface class it is using - the Impl? or the Stub? https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:147: return next_event - now; On 2014/10/20 19:26:46, xiyuan wrote: > I don't follow this. |next_event| should be less than |now|, right? What would > this produce? next_event will always be higher than base::Time::Now() since any directives that are less than now are first removed from the directive list when we call GetActiveDirective (audio_directive_list.cc:64). So the lowest value that |now| can have is base::Time::Now(). In the case that we don't have any directives at all, next_event will cascade down to the value of |now|, hence this will return exactly 0. I'll follow up this CL shortly with another that adds test to forward the time. That will require a change in //base so I don't want to add that to this CL. https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:127: base::Bind(&AudioManager::OnTokenEncoded, base::Unretained(this))); On 2014/10/20 19:26:46, xiyuan wrote: > base::Unretained makes me nervous. Could we use WeakPtr or something? This cancelable callback will get automatically canceled on destruction of this object (which will invalidate the internal weak_ptr that the cancelable callback uses), hence this callback will never be called after the destruction of this object. Since the cancelable callback already uses a weak_ptr internall, using a weak_ptr here would make using the cancelable callback redundant :)
https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > On 2014/10/20 20:06:54, Charlie wrote: > > On 2014/10/20 16:45:37, Rahul Chaturvedi wrote: > > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > > i agree that you should create an pure virtual AudioManagerInterface > instead > > > of > > > > selectively overriding methods from the real implementation. > > > > > > Though I agree that there are benefits to having a stub implementation > > > inheriting from a pure interface, my concern about that pattern aren't with > > > testing the original code (the AudioManager code will be tested separately > in > > > its own test anyway). The concern is managing two sets of code to have the > > same > > > behavior. Currently the AudioManager behavior is simple, but if at a later > > date, > > > it starts to get more complex (as it definitely will), we now have to update > > the > > > stub to match the behavior of the original AudioManager. This now means we > > have > > > two copies of code to maintain, which need to mimic the same behavior, i.e., > > to > > > ensure that the stub does that, we really should have tests for the stub > also. > > > This doesn't sound very optimal. > > > > > > Overriding large parts of the implementation is also undesirable, so I've > > fixed > > > this by using the original AudioManager class instead. > > > > > > I've changed the AudioPlayer and AudioRecorder classes instead to have a > pure > > > interface, and have their stubs inherit from that interface. This behavior > > > duplication is acceptable since it is at the very lowest layer - these > classes > > > are mimicking what the Audio hardware would have done instead - something we > > > intentionally want to duplicate. > > > > It's a little hard to talk about this without going through all the specifics. > > But the general idea would be that if you're relying on the full behavior of > the > > AudioManager, then it becomes more of an integration test than a unit test. > > > > The idea is exactly that you *shouldn't* have to duplicate AudioManager. You > > just need a simple stub to verify that this class interacts with it correctly. > > Yes, as the AudioManagerInterface grows, the stub will become slightly less > > simple. But it should not track the complexity of the real class. > > > > Its complexity may not grow at the same rate as the implementation, but the > complexity will grow. If it is possible to just use the real class instead, why > not? > > We talked about this already. See crbug/424781. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: virtual void Initialize(const DecodeSamplesCallback&, On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > On 2014/10/20 20:06:54, Charlie wrote: > > On 2014/10/20 16:45:38, Rahul Chaturvedi wrote: > > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > > it's cleaner to keep Initialize methods in the implementation(s) rather > than > > > > putting them in the interface class. different implementations usually > need > > > > different things to be passed in at creation. > > > > > > I'm confused about how this would work, maybe I am not reading this right. > > Could > > > you explain what you mean here a bit more? > > > > > > The way I am looking at it; we'll have AudioManagerBase, which will provide > > the > > > start/stop play/record methods. Then there will be an AudioManagerImpl, > which > > > will inherit from AudioManagerBase, that will actually implement these > > methods, > > > and will provide an additional Initialize method? > > > > > > Which class will AudioDirectiveHandler hold a pointer to? If > > > AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really > > call > > > Initialize(), since that method won't exist in the interface. If > > > AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't > > accept > > > any other classes that derive from AudioManagerBase as its audio manager. > > > > > > (since I am actually using the original AudioManager class itself, this > point > > is > > > moot here, but it still applies to the TestAudioPlayer/Recorder classes). > > > > Let's call it AudioManager or AudioManagerInterface instead of > AudioManagerBase. > > > > As for pointer types, one option would be: > > > > scoped_ptr<AudioManager> manager = AudioManagerImpl::Create(...); > > > > But I'm curious to see what Dan suggests. > > > > That doesn't fix the issue. The issue is that the container class needs to be > able to initialize the derived class, without knowing that it is a derived class > (which is what allows us to swap out the implementation with the stub). If the > Intialize method or the ::Create method, does not exist in, or is different in, > the Stub versus the Impl class, the container class needs to call a specific > Impl version of the function. This means that the container class now will need > to know which version of the AudioManagerInterface class it is using - the Impl? > or the Stub? By "container class", do you mean AudioDirectiveHandler? It's fine to have AudioDirectiveHandler initialize the Impl class by default. For testing, add another version of AudioDirectiveHandler::Initialize() that takes in a (stub) AudioManager. The default Initialize() can then delegate to that one and pass an AudioManagerImpl.
On 2014/10/20 20:33:05, Charlie wrote: > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > File components/copresence/handlers/audio/audio_directive_handler_unittest.cc > (right): > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: > class TestAudioManager : public AudioManager { > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 20:06:54, Charlie wrote: > > > On 2014/10/20 16:45:37, Rahul Chaturvedi wrote: > > > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > > > i agree that you should create an pure virtual AudioManagerInterface > > instead > > > > of > > > > > selectively overriding methods from the real implementation. > > > > > > > > Though I agree that there are benefits to having a stub implementation > > > > inheriting from a pure interface, my concern about that pattern aren't > with > > > > testing the original code (the AudioManager code will be tested separately > > in > > > > its own test anyway). The concern is managing two sets of code to have the > > > same > > > > behavior. Currently the AudioManager behavior is simple, but if at a later > > > date, > > > > it starts to get more complex (as it definitely will), we now have to > update > > > the > > > > stub to match the behavior of the original AudioManager. This now means we > > > have > > > > two copies of code to maintain, which need to mimic the same behavior, > i.e., > > > to > > > > ensure that the stub does that, we really should have tests for the stub > > also. > > > > This doesn't sound very optimal. > > > > > > > > Overriding large parts of the implementation is also undesirable, so I've > > > fixed > > > > this by using the original AudioManager class instead. > > > > > > > > I've changed the AudioPlayer and AudioRecorder classes instead to have a > > pure > > > > interface, and have their stubs inherit from that interface. This behavior > > > > duplication is acceptable since it is at the very lowest layer - these > > classes > > > > are mimicking what the Audio hardware would have done instead - something > we > > > > intentionally want to duplicate. > > > > > > It's a little hard to talk about this without going through all the > specifics. > > > But the general idea would be that if you're relying on the full behavior of > > the > > > AudioManager, then it becomes more of an integration test than a unit test. > > > > > > The idea is exactly that you *shouldn't* have to duplicate AudioManager. You > > > just need a simple stub to verify that this class interacts with it > correctly. > > > Yes, as the AudioManagerInterface grows, the stub will become slightly less > > > simple. But it should not track the complexity of the real class. > > > > > > > Its complexity may not grow at the same rate as the implementation, but the > > complexity will grow. If it is possible to just use the real class instead, > why > > not? > > > > > > We talked about this already. See crbug/424781. > > https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... > components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: > virtual void Initialize(const DecodeSamplesCallback&, > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 20:06:54, Charlie wrote: > > > On 2014/10/20 16:45:38, Rahul Chaturvedi wrote: > > > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > > > it's cleaner to keep Initialize methods in the implementation(s) rather > > than > > > > > putting them in the interface class. different implementations usually > > need > > > > > different things to be passed in at creation. > > > > > > > > I'm confused about how this would work, maybe I am not reading this right. > > > Could > > > > you explain what you mean here a bit more? > > > > > > > > The way I am looking at it; we'll have AudioManagerBase, which will > provide > > > the > > > > start/stop play/record methods. Then there will be an AudioManagerImpl, > > which > > > > will inherit from AudioManagerBase, that will actually implement these > > > methods, > > > > and will provide an additional Initialize method? > > > > > > > > Which class will AudioDirectiveHandler hold a pointer to? If > > > > AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really > > > call > > > > Initialize(), since that method won't exist in the interface. If > > > > AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't > > > accept > > > > any other classes that derive from AudioManagerBase as its audio manager. > > > > > > > > (since I am actually using the original AudioManager class itself, this > > point > > > is > > > > moot here, but it still applies to the TestAudioPlayer/Recorder classes). > > > > > > Let's call it AudioManager or AudioManagerInterface instead of > > AudioManagerBase. > > > > > > As for pointer types, one option would be: > > > > > > scoped_ptr<AudioManager> manager = AudioManagerImpl::Create(...); > > > > > > But I'm curious to see what Dan suggests. > > > > > > > That doesn't fix the issue. The issue is that the container class needs to be > > able to initialize the derived class, without knowing that it is a derived > class > > (which is what allows us to swap out the implementation with the stub). If the > > Intialize method or the ::Create method, does not exist in, or is different > in, > > the Stub versus the Impl class, the container class needs to call a specific > > Impl version of the function. This means that the container class now will > need > > to know which version of the AudioManagerInterface class it is using - the > Impl? > > or the Stub? > > By "container class", do you mean AudioDirectiveHandler? It's fine to have > AudioDirectiveHandler initialize the Impl class by default. For testing, add > another version of AudioDirectiveHandler::Initialize() that takes in a (stub) > AudioManager. The default Initialize() can then delegate to that one and pass an > AudioManagerImpl. i think that i'm on the same page as charlie here: AudioManager has a very simple set of public methods (particularly if all the _for_testing() stuff goes away). it seems like it would be easy to write a stub implementation of it and then use that in tests that need an AudioManager. to write tests for AudioManager, hand it stub/mock/recorder/whatever implementations of AudioPlayer and AudioRecorder. regarding initializing AudioManager, there are at least a few common approaches that i've seen: - pass an already-initialized instance of AudioManagerInterface into AudioDirectiveHandler - add AudioDirectiveHandler::set_audio_manager_for_testing(scoped_ptr<AudioManagerInterface>). if the AudioManager is already set when ADH::Init() is called, use it instead of creating a new one - add an AudioManagerFactoryInterface interface with real and fake implementations and pass a factory to AudioDirectiveHandler
https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:123: AudioRecorderImpl* recorder_; should this be AudioRecorder?
https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:147: return next_event - now; On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > On 2014/10/20 19:26:46, xiyuan wrote: > > I don't follow this. |next_event| should be less than |now|, right? What would > > this produce? > > next_event will always be higher than base::Time::Now() since any directives > that are less than now are first removed from the directive list when we call > GetActiveDirective (audio_directive_list.cc:64). So the lowest value that |now| > can have is base::Time::Now(). In the case that we don't have any directives at > all, next_event will cascade down to the value of |now|, hence this will return > exactly 0. > > I'll follow up this CL shortly with another that adds test to forward the time. > That will require a change in //base so I don't want to add that to this CL. I don't see how |next_event| could ever be different from |now|. If there is no pending directive, |next_event| is |now|. And if there is a pending directive and it would be higher than |now|, thus GetEarliestEventTime() would return |now| in this case. When would |next_event| not be |now|? https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:127: base::Bind(&AudioManager::OnTokenEncoded, base::Unretained(this))); On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > On 2014/10/20 19:26:46, xiyuan wrote: > > base::Unretained makes me nervous. Could we use WeakPtr or something? > > This cancelable callback will get automatically canceled on destruction of this > object (which will invalidate the internal weak_ptr that the cancelable callback > uses), hence this callback will never be called after the destruction of this > object. > > Since the cancelable callback already uses a weak_ptr internall, using a > weak_ptr here would make using the cancelable callback redundant :) Sorry if it is a dumb question. Why base::Bind() here creates a cancelable callback?
On 2014/10/20 21:29:29, xiyuan wrote: > https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... > File components/copresence/handlers/audio/audio_directive_handler.cc (right): > > https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... > components/copresence/handlers/audio/audio_directive_handler.cc:147: return > next_event - now; > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 19:26:46, xiyuan wrote: > > > I don't follow this. |next_event| should be less than |now|, right? What > would > > > this produce? > > > > next_event will always be higher than base::Time::Now() since any directives > > that are less than now are first removed from the directive list when we call > > GetActiveDirective (audio_directive_list.cc:64). So the lowest value that > |now| > > can have is base::Time::Now(). In the case that we don't have any directives > at > > all, next_event will cascade down to the value of |now|, hence this will > return > > exactly 0. > > > > I'll follow up this CL shortly with another that adds test to forward the > time. > > That will require a change in //base so I don't want to add that to this CL. > > I don't see how |next_event| could ever be different from |now|. If there is no > pending directive, |next_event| is |now|. And if there is a pending directive > and it would be higher than |now|, thus GetEarliestEventTime() would return > |now| in this case. When would |next_event| not be |now|? > > https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... > File components/copresence/mediums/audio/audio_manager.cc (right): > > https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... > components/copresence/mediums/audio/audio_manager.cc:127: > base::Bind(&AudioManager::OnTokenEncoded, base::Unretained(this))); > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 19:26:46, xiyuan wrote: > > > base::Unretained makes me nervous. Could we use WeakPtr or something? > > > > This cancelable callback will get automatically canceled on destruction of > this > > object (which will invalidate the internal weak_ptr that the cancelable > callback > > uses), hence this callback will never be called after the destruction of this > > object. > > > > Since the cancelable callback already uses a weak_ptr internall, using a > > weak_ptr here would make using the cancelable callback redundant :) > > Sorry if it is a dumb question. Why base::Bind() here creates a cancelable > callback? > i think that i'm on the same page as charlie here: > > AudioManager has a very simple set of public methods (particularly if all the > _for_testing() stuff goes away). it seems like it would be easy to write a stub > implementation of it and then use that in tests that need an AudioManager. There are two _for_testing() methods left; one to set the player, one to set the recorder. I am not sure if I understand how having a stub implementation is better than just using the real implementation directly with the audio hardware code stubbed out. If you feel strongly about this, I'll go ahead and change this around to have a common AudioManager and write the stub code. I agree that right now the burden to maintain an extra stub implementation is fairly trivial; I mostly don't understand why we'd take any burden at all when it isn't necessary. > regarding initializing AudioManager, there are at least a few common approaches > that i've seen: > - pass an already-initialized instance of AudioManagerInterface into > AudioDirectiveHandler > - add > AudioDirectiveHandler::set_audio_manager_for_testing(scoped_ptr<AudioManagerInterface>). > if the AudioManager is already set when ADH::Init() is called, use it instead of > creating a new one > - add an AudioManagerFactoryInterface interface with real and fake > implementations and pass a factory to AudioDirectiveHandler Isn't the second approach what we're already doing? The only difference seems to be that I am passing in the AudioManager itself, instead of creating a stub implementation and passing that in. Again, if you feel strongly that this is the wrong approach, I don't have a strong enough opinion about this to insist on any specific solution.
https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:19: class TestAudioManager : public AudioManager { On 2014/10/20 20:33:05, Charlie wrote: > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 20:06:54, Charlie wrote: > > > On 2014/10/20 16:45:37, Rahul Chaturvedi wrote: > > > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > > > i agree that you should create an pure virtual AudioManagerInterface > > instead > > > > of > > > > > selectively overriding methods from the real implementation. > > > > > > > > Though I agree that there are benefits to having a stub implementation > > > > inheriting from a pure interface, my concern about that pattern aren't > with > > > > testing the original code (the AudioManager code will be tested separately > > in > > > > its own test anyway). The concern is managing two sets of code to have the > > > same > > > > behavior. Currently the AudioManager behavior is simple, but if at a later > > > date, > > > > it starts to get more complex (as it definitely will), we now have to > update > > > the > > > > stub to match the behavior of the original AudioManager. This now means we > > > have > > > > two copies of code to maintain, which need to mimic the same behavior, > i.e., > > > to > > > > ensure that the stub does that, we really should have tests for the stub > > also. > > > > This doesn't sound very optimal. > > > > > > > > Overriding large parts of the implementation is also undesirable, so I've > > > fixed > > > > this by using the original AudioManager class instead. > > > > > > > > I've changed the AudioPlayer and AudioRecorder classes instead to have a > > pure > > > > interface, and have their stubs inherit from that interface. This behavior > > > > duplication is acceptable since it is at the very lowest layer - these > > classes > > > > are mimicking what the Audio hardware would have done instead - something > we > > > > intentionally want to duplicate. > > > > > > It's a little hard to talk about this without going through all the > specifics. > > > But the general idea would be that if you're relying on the full behavior of > > the > > > AudioManager, then it becomes more of an integration test than a unit test. > > > > > > The idea is exactly that you *shouldn't* have to duplicate AudioManager. You > > > just need a simple stub to verify that this class interacts with it > correctly. > > > Yes, as the AudioManagerInterface grows, the stub will become slightly less > > > simple. But it should not track the complexity of the real class. > > > > > > > Its complexity may not grow at the same rate as the implementation, but the > > complexity will grow. If it is possible to just use the real class instead, > why > > not? > > > > > > We talked about this already. See crbug/424781. I don't necessarily agree with everything said in that bug. https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: virtual void Initialize(const DecodeSamplesCallback&, On 2014/10/20 20:33:05, Charlie wrote: > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 20:06:54, Charlie wrote: > > > On 2014/10/20 16:45:38, Rahul Chaturvedi wrote: > > > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > > > it's cleaner to keep Initialize methods in the implementation(s) rather > > than > > > > > putting them in the interface class. different implementations usually > > need > > > > > different things to be passed in at creation. > > > > > > > > I'm confused about how this would work, maybe I am not reading this right. > > > Could > > > > you explain what you mean here a bit more? > > > > > > > > The way I am looking at it; we'll have AudioManagerBase, which will > provide > > > the > > > > start/stop play/record methods. Then there will be an AudioManagerImpl, > > which > > > > will inherit from AudioManagerBase, that will actually implement these > > > methods, > > > > and will provide an additional Initialize method? > > > > > > > > Which class will AudioDirectiveHandler hold a pointer to? If > > > > AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really > > > call > > > > Initialize(), since that method won't exist in the interface. If > > > > AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't > > > accept > > > > any other classes that derive from AudioManagerBase as its audio manager. > > > > > > > > (since I am actually using the original AudioManager class itself, this > > point > > > is > > > > moot here, but it still applies to the TestAudioPlayer/Recorder classes). > > > > > > Let's call it AudioManager or AudioManagerInterface instead of > > AudioManagerBase. > > > > > > As for pointer types, one option would be: > > > > > > scoped_ptr<AudioManager> manager = AudioManagerImpl::Create(...); > > > > > > But I'm curious to see what Dan suggests. > > > > > > > That doesn't fix the issue. The issue is that the container class needs to be > > able to initialize the derived class, without knowing that it is a derived > class > > (which is what allows us to swap out the implementation with the stub). If the > > Intialize method or the ::Create method, does not exist in, or is different > in, > > the Stub versus the Impl class, the container class needs to call a specific > > Impl version of the function. This means that the container class now will > need > > to know which version of the AudioManagerInterface class it is using - the > Impl? > > or the Stub? > > By "container class", do you mean AudioDirectiveHandler? It's fine to have > AudioDirectiveHandler initialize the Impl class by default. For testing, add > another version of AudioDirectiveHandler::Initialize() that takes in a (stub) > AudioManager. The default Initialize() can then delegate to that one and pass an > AudioManagerImpl. > What's the difference between this and just creating and setting the stub audio manager and then calling the original Initialize(). At this point, I am starting to wonder how this discussion is actually contributing to the quality of the code. https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:147: return next_event - now; On 2014/10/20 21:29:29, xiyuan wrote: > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 19:26:46, xiyuan wrote: > > > I don't follow this. |next_event| should be less than |now|, right? What > would > > > this produce? > > > > next_event will always be higher than base::Time::Now() since any directives > > that are less than now are first removed from the directive list when we call > > GetActiveDirective (audio_directive_list.cc:64). So the lowest value that > |now| > > can have is base::Time::Now(). In the case that we don't have any directives > at > > all, next_event will cascade down to the value of |now|, hence this will > return > > exactly 0. > > > > I'll follow up this CL shortly with another that adds test to forward the > time. > > That will require a change in //base so I don't want to add that to this CL. > > I don't see how |next_event| could ever be different from |now|. If there is no > pending directive, |next_event| is |now|. And if there is a pending directive > and it would be higher than |now|, thus GetEarliestEventTime() would return > |now| in this case. When would |next_event| not be |now|? Oh! I get what you are saying. Yes, this is extremely stupid and will never work. This needs to be base::Time::Max not base::Time::Now, since that will of course always be the lowest value. Fixed this up and re-wrote this function to not rely on a canary value to indicate that no new instructions were found. It returns a bool now instead. Thanks for catching this! I've made the timer changes needed for the timed test in this CL; the CL for the tests itself is here: https://codereview.chromium.org/670623002/ https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:127: base::Bind(&AudioManager::OnTokenEncoded, base::Unretained(this))); On 2014/10/20 21:29:29, xiyuan wrote: > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > On 2014/10/20 19:26:46, xiyuan wrote: > > > base::Unretained makes me nervous. Could we use WeakPtr or something? > > > > This cancelable callback will get automatically canceled on destruction of > this > > object (which will invalidate the internal weak_ptr that the cancelable > callback > > uses), hence this callback will never be called after the destruction of this > > object. > > > > Since the cancelable callback already uses a weak_ptr internall, using a > > weak_ptr here would make using the cancelable callback redundant :) > > Sorry if it is a dumb question. Why base::Bind() here creates a cancelable > callback? Really not my best day. This "should" be cancelable, I apparently forgot to put the code in for it. I thought more about this though and it seems that the correct solution is just to ensure that when we're done with the WhispernetClient and destructing all our managers/objects, we should simply null out any callbacks that we set. This will ensure that no objects within our hierarchy can be called by Whispernet post destruction. Added code to do that and added a comment here explaining why unretained is (now) safe to use here. Done. https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/120001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:123: AudioRecorderImpl* recorder_; On 2014/10/20 20:52:21, Daniel Erat wrote: > should this be AudioRecorder? Yep. Done.
if you are testing classes that needing an AudioManager, creating a pure interface and passing a stub (rather than a real AudioManager with stubbed players and recorders) is cleaner because it: a) avoids exercising a bunch of AudioManager code that you're not actually trying to test. b) avoids repetitive setup in the testing code, i.e. just instantiate an AudioManagerStub and tell it what to return instead of creating a real AudioManager or something that derives from it and trying to convince it to behave as is needed for the test. c) avoids polluting the real implementation with a bunch of testing-specific code that gets shipped in production builds.
https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/80001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:25: virtual void Initialize(const DecodeSamplesCallback&, On 2014/10/21 01:25:04, Rahul Chaturvedi wrote: > On 2014/10/20 20:33:05, Charlie wrote: > > On 2014/10/20 20:17:53, Rahul Chaturvedi wrote: > > > On 2014/10/20 20:06:54, Charlie wrote: > > > > On 2014/10/20 16:45:38, Rahul Chaturvedi wrote: > > > > > On 2014/10/18 21:21:18, Daniel Erat wrote: > > > > > > it's cleaner to keep Initialize methods in the implementation(s) > rather > > > than > > > > > > putting them in the interface class. different implementations usually > > > need > > > > > > different things to be passed in at creation. > > > > > > > > > > I'm confused about how this would work, maybe I am not reading this > right. > > > > Could > > > > > you explain what you mean here a bit more? > > > > > > > > > > The way I am looking at it; we'll have AudioManagerBase, which will > > provide > > > > the > > > > > start/stop play/record methods. Then there will be an AudioManagerImpl, > > > which > > > > > will inherit from AudioManagerBase, that will actually implement these > > > > methods, > > > > > and will provide an additional Initialize method? > > > > > > > > > > Which class will AudioDirectiveHandler hold a pointer to? If > > > > > AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't > really > > > > call > > > > > Initialize(), since that method won't exist in the interface. If > > > > > AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't > > > > accept > > > > > any other classes that derive from AudioManagerBase as its audio > manager. > > > > > > > > > > (since I am actually using the original AudioManager class itself, this > > > point > > > > is > > > > > moot here, but it still applies to the TestAudioPlayer/Recorder > classes). > > > > > > > > Let's call it AudioManager or AudioManagerInterface instead of > > > AudioManagerBase. > > > > > > > > As for pointer types, one option would be: > > > > > > > > scoped_ptr<AudioManager> manager = AudioManagerImpl::Create(...); > > > > > > > > But I'm curious to see what Dan suggests. > > > > > > > > > > That doesn't fix the issue. The issue is that the container class needs to > be > > > able to initialize the derived class, without knowing that it is a derived > > class > > > (which is what allows us to swap out the implementation with the stub). If > the > > > Intialize method or the ::Create method, does not exist in, or is different > > in, > > > the Stub versus the Impl class, the container class needs to call a specific > > > Impl version of the function. This means that the container class now will > > need > > > to know which version of the AudioManagerInterface class it is using - the > > Impl? > > > or the Stub? > > > > By "container class", do you mean AudioDirectiveHandler? It's fine to have > > AudioDirectiveHandler initialize the Impl class by default. For testing, add > > another version of AudioDirectiveHandler::Initialize() that takes in a (stub) > > AudioManager. The default Initialize() can then delegate to that one and pass > an > > AudioManagerImpl. > > > > What's the difference between this and just creating and setting the stub audio > manager and then calling the original Initialize(). At this point, I am starting > to wonder how this discussion is actually contributing to the quality of the > code. set_*_for_testing() methods are sub-optimal because, as Dan points out, test-specific code clutters the cc file and doesn't belong in the prod binary.
Changed AudioManager to be an interface, the stub and implementation inherit from it.
https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:106: DCHECK(audio_event_timer_); why is this DCHECK-ed? it's currently initialized in the c'tor and never appears to be reset. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:148: base::TimeTicks max = base::TimeTicks() + base::TimeDelta::Max(); using the max value here makes me nervous. why not start out with base::TimeTicks() and have GetEarliestEventTime() handle that value explicitly? note that the comment for Max() also says that you can't do this: // Returns the maximum time delta, which should be greater than any reasonable // time delta we might compare it to. Adding or subtracting the maximum time // delta to a time or another time delta has an undefined result. static TimeDelta Max(); https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:60: void set_audio_manager_for_testing(scoped_ptr<AudioManager> manager) { i still don't like Initialize() being part of the interface instead of the implementation, but if you do this you need to at least document here that this method requires an uninitialized manager to be passed in. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:78: scoped_ptr<base::Timer> audio_event_timer_; it's fine to #include things from base in headers and declare these directly instead of wrapping them in scoped_ptrs, fwiw https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:18: class TestAudioManager final : public AudioManager { nit: AudioManagerStub https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:35: const std::string& url_unsafe_token) override{}; nit: "override {}" https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:47: std::string token_; did you mean to set this in SetToken()? it's always empty right now. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:62: base::Bind(&AudioDirectiveHandlerTest::DecodeSamples, just pass a callback around an empty function in the anon namespace if you never intend to check that these are called? https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:106: TestAudioManager* manager_ptr_; nit: document that this is unowned https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:45: : playing_{false, false}, this is banned, at least at the moment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:139: } nit: add blank line https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:142: } nit: add blank line https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:36: SamplesCallback; nit: rename this to something like HandleTokenCallback so the name is less ambiguous? (at least, i'm guessing that the general flow is that EncodeTokenCallback generates a token and passes it to this callback.) https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:44: virtual void Initialize(const DecodeSamplesCallback& decode_cb, as discussed earlier, does this need to be part of the interface? https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:56: virtual const std::string& get_token(AudioType type) = 0; rename to GetToken() https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:61: virtual ~AudioManager(){}; nit: move this up after the typedefs, add a space between () and {}, and remove the trailing semicolon https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:44: virtual ~AudioRecorder(){}; nit: virtual ~AudioRecorder() {} https://codereview.chromium.org/637223011/diff/160001/components/copresence/p... File components/copresence/public/copresence_delegate.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/p... components/copresence/public/copresence_delegate.h:45: virtual ~CopresenceDelegate() {} nit: move this to the top https://codereview.chromium.org/637223011/diff/160001/components/copresence/t... File components/copresence/test/audio_test_support.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/t... components/copresence/test/audio_test_support.h:34: class TestAudioPlayer : public AudioPlayer { i think that names like AudioPlayerStub and AudioRecorderStub are more common for classes like these. they should probably live alongside the implementation in audio_player_stub/audio_recorder_stub classes, too. https://codereview.chromium.org/637223011/diff/160001/components/copresence/t... components/copresence/test/audio_test_support.h:52: class TestAudioRecorder : public AudioRecorderImpl { why's this deriving from AudioRecorderImpl instead of AudioRecorder? use 'final' there, maybe?
https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:68: class AudioManagerImpl final : public AudioManager { Does this need to be in the same .h file? Who references the impl class besides audio_manager.cc? URLFetcher, for example, has: url_fetcher.h url_fetcher_impl.h url_fetcher_impl.cc Same for AudioPlayerImpl and AudioRecorderImpl.
https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:106: DCHECK(audio_event_timer_); On 2014/10/21 19:43:21, Daniel Erat wrote: > why is this DCHECK-ed? it's currently initialized in the c'tor and never appears > to be reset. In the CL that adds the timed tests, this pointer can be changed. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:148: base::TimeTicks max = base::TimeTicks() + base::TimeDelta::Max(); On 2014/10/21 19:43:21, Daniel Erat wrote: > using the max value here makes me nervous. why not start out with > base::TimeTicks() and have GetEarliestEventTime() handle that value explicitly? > > note that the comment for Max() also says that you can't do this: > > // Returns the maximum time delta, which should be greater than any reasonable > // time delta we might compare it to. Adding or subtracting the maximum time > // delta to a time or another time delta has an undefined result. > static TimeDelta Max(); Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:60: void set_audio_manager_for_testing(scoped_ptr<AudioManager> manager) { On 2014/10/21 19:43:21, Daniel Erat wrote: > i still don't like Initialize() being part of the interface instead of the > implementation, but if you do this you need to at least document here that this > method requires an uninitialized manager to be passed in. Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:78: scoped_ptr<base::Timer> audio_event_timer_; On 2014/10/21 19:43:21, Daniel Erat wrote: > it's fine to #include things from base in headers and declare these directly > instead of wrapping them in scoped_ptrs, fwiw I am not using this as a pointer to avoid including base/time.h. Since I _am using this as a pointer, I am no longer including base/time.h The reason to make this a pointer to a base::Timer is to allow timed testing. See https://codereview.chromium.org/670623002/ https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:18: class TestAudioManager final : public AudioManager { On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: AudioManagerStub Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:35: const std::string& url_unsafe_token) override{}; On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: "override {}" Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:47: std::string token_; On 2014/10/21 19:43:22, Daniel Erat wrote: > did you mean to set this in SetToken()? it's always empty right now. This returns a reference pointer. It needs a real object to return. The value is not used anywhere in tests. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:62: base::Bind(&AudioDirectiveHandlerTest::DecodeSamples, On 2014/10/21 19:43:22, Daniel Erat wrote: > just pass a callback around an empty function in the anon namespace if you never > intend to check that these are called? Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:106: TestAudioManager* manager_ptr_; On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: document that this is unowned Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.cc (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:45: : playing_{false, false}, On 2014/10/21 19:43:22, Daniel Erat wrote: > this is banned, at least at the moment: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:139: } On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: add blank line Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.cc:142: } On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: add blank line Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:36: SamplesCallback; On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: rename this to something like HandleTokenCallback so the name is less > ambiguous? > > (at least, i'm guessing that the general flow is that EncodeTokenCallback > generates a token and passes it to this callback.) Nope; EncodeTokenCallback generates encoded samples, which are passed to this callback, hence it being called SamplesCallback. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:44: virtual void Initialize(const DecodeSamplesCallback& decode_cb, On 2014/10/21 19:43:22, Daniel Erat wrote: > as discussed earlier, does this need to be part of the interface? I changed my code around to remove Initialize from the interface and move it only to the Impl classes. This broke one of my tests, without it actually being a bug in my code at all. The code which did not call Initialize for the stub classes, made it so that the decode_callback was never set. The AudioManagerTest.Record test expects that when TriggerDecodeRequest() on the stub recorder is called, correct program behavior will try to decode samples. Since we never initialized the stub recorder (which makes this connection), the code path never gets called and the test fails. I can of course fix this by moving the passing of the decode callback to the constructor but then I'd be making a change in production code that I don't necessarily think is the optimal design, because of the insistence of not having an Initialize method for stub classes also. This made me realize that Initialize *does* need to be part of stub implementations. Since we have paper thin constructors and do all our construction in Initialize, completely bypassing Initialize doesn't sound like a good idea. For example, the AudioRecorder *does* need to be initialized and does need to be initialized by the AudioManager. This is a behavior that we want to specifically test. If we take out the Initialize methods, this code path is no longer being tested. Unless there is a really strong reason to remove Initialize, I'd prefer to keep it in. I don't see a particularly strong downside to letting a stub object initialize via the same interface method that the implementation object is. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:56: virtual const std::string& get_token(AudioType type) = 0; On 2014/10/21 19:43:22, Daniel Erat wrote: > rename to GetToken() Renamed back to GetToken. Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:61: virtual ~AudioManager(){}; On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: move this up after the typedefs, add a space between () and {}, and remove > the trailing semicolon Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:68: class AudioManagerImpl final : public AudioManager { On 2014/10/21 19:59:42, Charlie wrote: > Does this need to be in the same .h file? Who references the impl class besides > audio_manager.cc? > > URLFetcher, for example, has: > url_fetcher.h > url_fetcher_impl.h > url_fetcher_impl.cc > > Same for AudioPlayerImpl and AudioRecorderImpl. Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:44: virtual ~AudioRecorder(){}; On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: virtual ~AudioRecorder() {} Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/p... File components/copresence/public/copresence_delegate.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/p... components/copresence/public/copresence_delegate.h:45: virtual ~CopresenceDelegate() {} On 2014/10/21 19:43:22, Daniel Erat wrote: > nit: move this to the top Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/t... File components/copresence/test/audio_test_support.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/t... components/copresence/test/audio_test_support.h:34: class TestAudioPlayer : public AudioPlayer { On 2014/10/21 19:43:22, Daniel Erat wrote: > i think that names like AudioPlayerStub and AudioRecorderStub are more common > for classes like these. they should probably live alongside the implementation > in audio_player_stub/audio_recorder_stub classes, too. Done. https://codereview.chromium.org/637223011/diff/160001/components/copresence/t... components/copresence/test/audio_test_support.h:52: class TestAudioRecorder : public AudioRecorderImpl { On 2014/10/21 19:43:23, Daniel Erat wrote: > why's this deriving from AudioRecorderImpl instead of AudioRecorder? use 'final' > there, maybe? All derived classes marked final. Done.
https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:44: virtual void Initialize(const DecodeSamplesCallback& decode_cb, On 2014/10/22 00:06:20, Rahul Chaturvedi wrote: > On 2014/10/21 19:43:22, Daniel Erat wrote: > > as discussed earlier, does this need to be part of the interface? > > I changed my code around to remove Initialize from the interface and move it > only to the Impl classes. This broke one of my tests, without it actually being > a bug in my code at all. The code which did not call Initialize for the stub > classes, made it so that the decode_callback was never set. The > AudioManagerTest.Record test expects that when TriggerDecodeRequest() on the > stub recorder is called, correct program behavior will try to decode samples. > Since we never initialized the stub recorder (which makes this connection), the > code path never gets called and the test fails. > > I can of course fix this by moving the passing of the decode callback to the > constructor but then I'd be making a change in production code that I don't > necessarily think is the optimal design, because of the insistence of not having > an Initialize method for stub classes also. > > This made me realize that Initialize *does* need to be part of stub > implementations. Since we have paper thin constructors and do all our > construction in Initialize, completely bypassing Initialize doesn't sound like a > good idea. For example, the AudioRecorder *does* need to be initialized and does > need to be initialized by the AudioManager. This is a behavior that we want to > specifically test. If we take out the Initialize methods, this code path is no > longer being tested. > > Unless there is a really strong reason to remove Initialize, I'd prefer to keep > it in. I don't see a particularly strong downside to letting a stub object > initialize via the same interface method that the implementation object is. This is for another CL, but can you remind me why we have the Initialize() calls at all? Every time we call them, it seems follow this pattern: if (!foo_ptr_) foo_ptr_.reset(new Foo()); foo_ptr_->Initialize(...); I also cannot really see what's complex enough about Initialize() that it can't be done in a constructor. This may actually be a holdover from the test classes subclassing the prod impls: then we needed a way to set up the state differently for testing. But we're not doing that anymore. Summary: Add a TODO to merge Initialize() into the constructors?
https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:44: virtual void Initialize(const DecodeSamplesCallback& decode_cb, On 2014/10/22 00:28:06, Charlie wrote: > On 2014/10/22 00:06:20, Rahul Chaturvedi wrote: > > On 2014/10/21 19:43:22, Daniel Erat wrote: > > > as discussed earlier, does this need to be part of the interface? > > > > I changed my code around to remove Initialize from the interface and move it > > only to the Impl classes. This broke one of my tests, without it actually > being > > a bug in my code at all. The code which did not call Initialize for the stub > > classes, made it so that the decode_callback was never set. The > > AudioManagerTest.Record test expects that when TriggerDecodeRequest() on the > > stub recorder is called, correct program behavior will try to decode samples. > > Since we never initialized the stub recorder (which makes this connection), > the > > code path never gets called and the test fails. > > > > I can of course fix this by moving the passing of the decode callback to the > > constructor but then I'd be making a change in production code that I don't > > necessarily think is the optimal design, because of the insistence of not > having > > an Initialize method for stub classes also. > > > > This made me realize that Initialize *does* need to be part of stub > > implementations. Since we have paper thin constructors and do all our > > construction in Initialize, completely bypassing Initialize doesn't sound like > a > > good idea. For example, the AudioRecorder *does* need to be initialized and > does > > need to be initialized by the AudioManager. This is a behavior that we want to > > specifically test. If we take out the Initialize methods, this code path is no > > longer being tested. > > > > Unless there is a really strong reason to remove Initialize, I'd prefer to > keep > > it in. I don't see a particularly strong downside to letting a stub object > > initialize via the same interface method that the implementation object is. > > This is for another CL, but can you remind me why we have the Initialize() calls > at all? Every time we call them, it seems follow this pattern: > > if (!foo_ptr_) foo_ptr_.reset(new Foo()); > foo_ptr_->Initialize(...); > > I also cannot really see what's complex enough about Initialize() that it can't > be done in a constructor. This may actually be a holdover from the test classes > subclassing the prod impls: then we needed a way to set up the state differently > for testing. But we're not doing that anymore. > > Summary: Add a TODO to merge Initialize() into the constructors? We do this from tests all the time. We create the stub in the test, pass it to the class. The class then calls Initialize() (which ends up either initializing the real class or the stub).
https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:44: virtual void Initialize(const DecodeSamplesCallback& decode_cb, On 2014/10/22 00:47:08, Rahul Chaturvedi wrote: > On 2014/10/22 00:28:06, Charlie wrote: > > On 2014/10/22 00:06:20, Rahul Chaturvedi wrote: > > > On 2014/10/21 19:43:22, Daniel Erat wrote: > > > > as discussed earlier, does this need to be part of the interface? > > > > > > I changed my code around to remove Initialize from the interface and move it > > > only to the Impl classes. This broke one of my tests, without it actually > > being > > > a bug in my code at all. The code which did not call Initialize for the stub > > > classes, made it so that the decode_callback was never set. The > > > AudioManagerTest.Record test expects that when TriggerDecodeRequest() on the > > > stub recorder is called, correct program behavior will try to decode > samples. > > > Since we never initialized the stub recorder (which makes this connection), > > the > > > code path never gets called and the test fails. > > > > > > I can of course fix this by moving the passing of the decode callback to the > > > constructor but then I'd be making a change in production code that I don't > > > necessarily think is the optimal design, because of the insistence of not > > having > > > an Initialize method for stub classes also. > > > > > > This made me realize that Initialize *does* need to be part of stub > > > implementations. Since we have paper thin constructors and do all our > > > construction in Initialize, completely bypassing Initialize doesn't sound > like > > a > > > good idea. For example, the AudioRecorder *does* need to be initialized and > > does > > > need to be initialized by the AudioManager. This is a behavior that we want > to > > > specifically test. If we take out the Initialize methods, this code path is > no > > > longer being tested. > > > > > > Unless there is a really strong reason to remove Initialize, I'd prefer to > > keep > > > it in. I don't see a particularly strong downside to letting a stub object > > > initialize via the same interface method that the implementation object is. > > > > This is for another CL, but can you remind me why we have the Initialize() > calls > > at all? Every time we call them, it seems follow this pattern: > > > > if (!foo_ptr_) foo_ptr_.reset(new Foo()); > > foo_ptr_->Initialize(...); > > > > I also cannot really see what's complex enough about Initialize() that it > can't > > be done in a constructor. This may actually be a holdover from the test > classes > > subclassing the prod impls: then we needed a way to set up the state > differently > > for testing. But we're not doing that anymore. > > > > Summary: Add a TODO to merge Initialize() into the constructors? > > We do this from tests all the time. We create the stub in the test, pass it to > the class. The class then calls Initialize() (which ends up either initializing > the real class or the stub). I see. But if Initialize() is really just about setting callbacks, maybe we should have set_foo_callback() instead, and the class should be resilient to null callbacks. As I said, though, this is for another CL. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager_stub.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_stub.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. I think the typical pattern is that stub classes live in the *_unittest.cc file that uses them. Then you don't need *_stub.h either. Is this used in multiple places? Ditto for the other stub files.
https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:44: virtual void Initialize(const DecodeSamplesCallback& decode_cb, On 2014/10/22 01:03:29, Charlie wrote: > On 2014/10/22 00:47:08, Rahul Chaturvedi wrote: > > On 2014/10/22 00:28:06, Charlie wrote: > > > On 2014/10/22 00:06:20, Rahul Chaturvedi wrote: > > > > On 2014/10/21 19:43:22, Daniel Erat wrote: > > > > > as discussed earlier, does this need to be part of the interface? > > > > > > > > I changed my code around to remove Initialize from the interface and move > it > > > > only to the Impl classes. This broke one of my tests, without it actually > > > being > > > > a bug in my code at all. The code which did not call Initialize for the > stub > > > > classes, made it so that the decode_callback was never set. The > > > > AudioManagerTest.Record test expects that when TriggerDecodeRequest() on > the > > > > stub recorder is called, correct program behavior will try to decode > > samples. > > > > Since we never initialized the stub recorder (which makes this > connection), > > > the > > > > code path never gets called and the test fails. > > > > > > > > I can of course fix this by moving the passing of the decode callback to > the > > > > constructor but then I'd be making a change in production code that I > don't > > > > necessarily think is the optimal design, because of the insistence of not > > > having > > > > an Initialize method for stub classes also. > > > > > > > > This made me realize that Initialize *does* need to be part of stub > > > > implementations. Since we have paper thin constructors and do all our > > > > construction in Initialize, completely bypassing Initialize doesn't sound > > like > > > a > > > > good idea. For example, the AudioRecorder *does* need to be initialized > and > > > does > > > > need to be initialized by the AudioManager. This is a behavior that we > want > > to > > > > specifically test. If we take out the Initialize methods, this code path > is > > no > > > > longer being tested. > > > > > > > > Unless there is a really strong reason to remove Initialize, I'd prefer to > > > keep > > > > it in. I don't see a particularly strong downside to letting a stub object > > > > initialize via the same interface method that the implementation object > is. > > > > > > This is for another CL, but can you remind me why we have the Initialize() > > calls > > > at all? Every time we call them, it seems follow this pattern: > > > > > > if (!foo_ptr_) foo_ptr_.reset(new Foo()); > > > foo_ptr_->Initialize(...); > > > > > > I also cannot really see what's complex enough about Initialize() that it > > can't > > > be done in a constructor. This may actually be a holdover from the test > > classes > > > subclassing the prod impls: then we needed a way to set up the state > > differently > > > for testing. But we're not doing that anymore. > > > > > > Summary: Add a TODO to merge Initialize() into the constructors? > > > > We do this from tests all the time. We create the stub in the test, pass it to > > the class. The class then calls Initialize() (which ends up either > initializing > > the real class or the stub). > > I see. But if Initialize() is really just about setting callbacks, maybe we > should have set_foo_callback() instead, and the class should be resilient to > null callbacks. > > As I said, though, this is for another CL. Initialize in this case is about setting callbacks (and connecting callbacks). The function does exactly what the name indicates - it initializes the class. Whether this involves setting callbacks or connecting them or doing any other setup required for the class to function correctly, it will be done here. I I guess I am not seeing what exactly is wrong with this pattern?
lgtm with one more round of comments https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... File chrome/browser/copresence/chrome_whispernet_client.h (right): https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... chrome/browser/copresence/chrome_whispernet_client.h:41: virtual void Initialize(const SuccessCallback& init_callback) override; nit: i think that the latest is that 'virtual' is supposed to be omitted when 'override' is present https://codereview.chromium.org/637223011/diff/180001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence.g... components/copresence.gypi:37: 'copresence/mediums/audio/audio_manager_impl.cc', nit: fix alphabetization of all of these https://codereview.chromium.org/637223011/diff/180001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/B... components/copresence/BUILD.gn:21: "mediums/audio/audio_manager_impl.cc", here too https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:12: #include "base/time/time.h" don't need this here; it's already included by the header https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:49: if (!audio_manager_.get()) nit: just do "if (!audio_manager_)" https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:61: << " with medium= " << instruction.medium() nit: remove extra space after '=' to match the rest of this string https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:78: << " with medium= " << instruction.medium() same here https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:114: // Change audio_manager_ state for audible transmits. nit: s/audio_manager_/|audio_manager_|/ https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:140: // This timer will be destructed before us, hence it is safe to use nit: i don't think that this comment is necessary; passing base::Unretained(this) to a timer is a common pattern https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:153: base::TimeTicks& next_event = *next_event_ptr; nit: rename the argument to |expiry_out| and get rid of the reference? doesn't look like it'd wrap: *expiry_out = GetEarliestEventTime(&transmits_list_[INAUDIBLE], *expiry_out); https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:161: // If our next event is still at max, means we don't have any active this max comment is obsolete https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:162: // directives; or that they are all due to expire a bazzilion years from now. s/bazzilion/bazillion/ https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:165: if (next_event.is_null()) nit: "return !expiry_out->is_null();" https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:57: return audio_manager_->GetToken(type); don't inline this https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:70: bool GetNextInstructionExpiry(base::TimeTicks* next_event); nit: add a brief comment describing this method too. make sure to document the return value since it's non-obvious from the signature. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:23: const AudioManager::SamplesCallback& callback) { nit: omit the argument name for the callback too https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:26: struct AudioDirective final { nit: putting final on a struct seems unnecessary https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:32: base::TimeTicks end_time; while i pretty much always support the use of TimeTicks rather than Time, just want to make sure: one difference (at least on cros) is that TimeTicks won't advance while the system is suspended. probably doesn't matter here, but wanted to make sure you were aware of it. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:24: SamplesCallback; it seems confusing to have one callback named "DecodeSamplesCallback" and another that's just named "SamplesCallback" -- how about "TokenSamplesCallback", then? https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:46: virtual const std::string& GetToken(AudioType type) = 0; if the returned value is short, please just return std::string directly https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager_impl.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_impl.cc:97: DCHECK_GT(token_[type].size(), 0u); nit: DCHECK(!token_[type].empty()); https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager_stub.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_stub.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/10/22 01:03:29, Charlie wrote: > I think the typical pattern is that stub classes live in the *_unittest.cc file > that uses them. Then you don't need *_stub.h either. Is this used in multiple > places? > > Ditto for the other stub files. agreed that you can do this if these are only used in a single test file. if not, you can probably just inline the implementation in the .h file and get rid of this one (at least, i wouldn't care much about http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... for a stub file...) https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_unittest.cc:39: void DirectiveAdded() {} did you mean to delete this too? https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_unittest.cc:56: // These will be deleted by audio_manager_'s dtor calling finalize on them. nit: |audio_manager_| https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_player_impl.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_player_impl.h:32: virtual void Initialize() override; nit: remove 'virtual' for all of these too since you have 'override' https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_player_unittest.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_player_unittest.cc:133: AudioPlayerImpl* player_; nit: document ownership https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:36: virtual ~AudioRecorder(){}; nit: virtual ~AudioRecorder() {} https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... components/copresence/public/copresence_constants.h:38: UNKNOWN = 3, might've mentioned this already, but "UNKNOWN" seems like an overly-vague value to put at the top level of the copresence namespace. i think it'd be better to rename these to AUDIO_AUDIBLE, AUDIO_INAUDIBLE, AUDIO_BOTH, and AUDIO_UNKNOWN (or AUDIO_TYPE_*).
https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:44: virtual void Initialize(const DecodeSamplesCallback& decode_cb, On 2014/10/22 14:08:49, Rahul Chaturvedi wrote: > On 2014/10/22 01:03:29, Charlie wrote: > > On 2014/10/22 00:47:08, Rahul Chaturvedi wrote: > > > On 2014/10/22 00:28:06, Charlie wrote: > > > > On 2014/10/22 00:06:20, Rahul Chaturvedi wrote: > > > > > On 2014/10/21 19:43:22, Daniel Erat wrote: > > > > > > as discussed earlier, does this need to be part of the interface? > > > > > > > > > > I changed my code around to remove Initialize from the interface and > move > > it > > > > > only to the Impl classes. This broke one of my tests, without it > actually > > > > being > > > > > a bug in my code at all. The code which did not call Initialize for the > > stub > > > > > classes, made it so that the decode_callback was never set. The > > > > > AudioManagerTest.Record test expects that when TriggerDecodeRequest() on > > the > > > > > stub recorder is called, correct program behavior will try to decode > > > samples. > > > > > Since we never initialized the stub recorder (which makes this > > connection), > > > > the > > > > > code path never gets called and the test fails. > > > > > > > > > > I can of course fix this by moving the passing of the decode callback to > > the > > > > > constructor but then I'd be making a change in production code that I > > don't > > > > > necessarily think is the optimal design, because of the insistence of > not > > > > having > > > > > an Initialize method for stub classes also. > > > > > > > > > > This made me realize that Initialize *does* need to be part of stub > > > > > implementations. Since we have paper thin constructors and do all our > > > > > construction in Initialize, completely bypassing Initialize doesn't > sound > > > like > > > > a > > > > > good idea. For example, the AudioRecorder *does* need to be initialized > > and > > > > does > > > > > need to be initialized by the AudioManager. This is a behavior that we > > want > > > to > > > > > specifically test. If we take out the Initialize methods, this code path > > is > > > no > > > > > longer being tested. > > > > > > > > > > Unless there is a really strong reason to remove Initialize, I'd prefer > to > > > > keep > > > > > it in. I don't see a particularly strong downside to letting a stub > object > > > > > initialize via the same interface method that the implementation object > > is. > > > > > > > > This is for another CL, but can you remind me why we have the Initialize() > > > calls > > > > at all? Every time we call them, it seems follow this pattern: > > > > > > > > if (!foo_ptr_) foo_ptr_.reset(new Foo()); > > > > foo_ptr_->Initialize(...); > > > > > > > > I also cannot really see what's complex enough about Initialize() that it > > > can't > > > > be done in a constructor. This may actually be a holdover from the test > > > classes > > > > subclassing the prod impls: then we needed a way to set up the state > > > differently > > > > for testing. But we're not doing that anymore. > > > > > > > > Summary: Add a TODO to merge Initialize() into the constructors? > > > > > > We do this from tests all the time. We create the stub in the test, pass it > to > > > the class. The class then calls Initialize() (which ends up either > > initializing > > > the real class or the stub). > > > > I see. But if Initialize() is really just about setting callbacks, maybe we > > should have set_foo_callback() instead, and the class should be resilient to > > null callbacks. > > > > As I said, though, this is for another CL. > > Initialize in this case is about setting callbacks (and connecting callbacks). > The function does exactly what the name indicates - it initializes the class. > Whether this involves setting callbacks or connecting them or doing any other > setup required for the class to function correctly, it will be done here. I I > guess I am not seeing what exactly is wrong with this pattern? One of the principles of encapsulation is that objects should always be in a consistent state. This consistency is set up by the constructor and maintained by all the other member functions. Having the constructor half-initialize the object, and requiring a call to Initialize() before doing anything else, is really a hack that we should use if there's no other way. It's also a way to pretend there's no heavy lifting (e.g. initializing other objects) in the constructor. See: http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ Tracked at P3 in crbug/424735.
https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... File chrome/browser/copresence/chrome_whispernet_client.h (right): https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... chrome/browser/copresence/chrome_whispernet_client.h:41: virtual void Initialize(const SuccessCallback& init_callback) override; On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: i think that the latest is that 'virtual' is supposed to be omitted when > 'override' is present Ah they fixed the presubmit for this? I'll go ahead and change it everywhere. https://codereview.chromium.org/637223011/diff/180001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence.g... components/copresence.gypi:37: 'copresence/mediums/audio/audio_manager_impl.cc', On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: fix alphabetization of all of these Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/B... components/copresence/BUILD.gn:21: "mediums/audio/audio_manager_impl.cc", On 2014/10/22 16:34:34, Daniel Erat wrote: > here too Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:12: #include "base/time/time.h" On 2014/10/22 16:34:34, Daniel Erat wrote: > don't need this here; it's already included by the header Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:49: if (!audio_manager_.get()) On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: just do "if (!audio_manager_)" Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:61: << " with medium= " << instruction.medium() On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: remove extra space after '=' to match the rest of this string Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:78: << " with medium= " << instruction.medium() On 2014/10/22 16:34:34, Daniel Erat wrote: > same here Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:114: // Change audio_manager_ state for audible transmits. On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: s/audio_manager_/|audio_manager_|/ Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:140: // This timer will be destructed before us, hence it is safe to use On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: i don't think that this comment is necessary; passing > base::Unretained(this) to a timer is a common pattern Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:153: base::TimeTicks& next_event = *next_event_ptr; On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: rename the argument to |expiry_out| and get rid of the reference? doesn't > look like it'd wrap: > > *expiry_out = GetEarliestEventTime(&transmits_list_[INAUDIBLE], *expiry_out); Renaming to expiry does prevent wrapping on all lines. Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:161: // If our next event is still at max, means we don't have any active On 2014/10/22 16:34:34, Daniel Erat wrote: > this max comment is obsolete Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:162: // directives; or that they are all due to expire a bazzilion years from now. On 2014/10/22 16:34:34, Daniel Erat wrote: > s/bazzilion/bazillion/ I didn't know there was a widely accepted spelling for bazillion :) Outdated comment removed though. Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:165: if (next_event.is_null()) On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: "return !expiry_out->is_null();" Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:57: return audio_manager_->GetToken(type); On 2014/10/22 16:34:34, Daniel Erat wrote: > don't inline this Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:70: bool GetNextInstructionExpiry(base::TimeTicks* next_event); On 2014/10/22 16:34:34, Daniel Erat wrote: > nit: add a brief comment describing this method too. make sure to document the > return value since it's non-obvious from the signature. Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:23: const AudioManager::SamplesCallback& callback) { On 2014/10/22 16:34:35, Daniel Erat wrote: > nit: omit the argument name for the callback too Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:26: struct AudioDirective final { On 2014/10/22 16:34:35, Daniel Erat wrote: > nit: putting final on a struct seems unnecessary Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:32: base::TimeTicks end_time; On 2014/10/22 16:34:35, Daniel Erat wrote: > while i pretty much always support the use of TimeTicks rather than Time, just > want to make sure: one difference (at least on cros) is that TimeTicks won't > advance while the system is suspended. probably doesn't matter here, but wanted > to make sure you were aware of it. I read that in the documentation but don't think it effects this code. If NowTicks will always return the number of ticks since epoch, whether a system was suspended or not shoudln't make a difference, correct? https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:24: SamplesCallback; On 2014/10/22 16:34:35, Daniel Erat wrote: > it seems confusing to have one callback named "DecodeSamplesCallback" and > another that's just named "SamplesCallback" -- how about "TokenSamplesCallback", > then? Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:46: virtual const std::string& GetToken(AudioType type) = 0; On 2014/10/22 16:34:35, Daniel Erat wrote: > if the returned value is short, please just return std::string directly Any particular reason for not returning a const reference? it is a const, so the returned reference won't be changed (without an explicit const cast anyway at least). https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager_impl.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_impl.cc:97: DCHECK_GT(token_[type].size(), 0u); On 2014/10/22 16:34:35, Daniel Erat wrote: > nit: DCHECK(!token_[type].empty()); Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager_stub.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_stub.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/10/22 16:34:35, Daniel Erat wrote: > On 2014/10/22 01:03:29, Charlie wrote: > > I think the typical pattern is that stub classes live in the *_unittest.cc > file > > that uses them. Then you don't need *_stub.h either. Is this used in multiple > > places? > > > > Ditto for the other stub files. > > agreed that you can do this if these are only used in a single test file. if > not, you can probably just inline the implementation in the .h file and get rid > of this one (at least, i wouldn't care much about > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > for a stub file...) Sure. Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_stub.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/10/22 01:03:29, Charlie wrote: > I think the typical pattern is that stub classes live in the *_unittest.cc file > that uses them. Then you don't need *_stub.h either. Is this used in multiple > places? > > Ditto for the other stub files. Sure. Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_unittest.cc:39: void DirectiveAdded() {} On 2014/10/22 16:34:35, Daniel Erat wrote: > did you mean to delete this too? Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager_unittest.cc:56: // These will be deleted by audio_manager_'s dtor calling finalize on them. On 2014/10/22 16:34:35, Daniel Erat wrote: > nit: |audio_manager_| Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_player_impl.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_player_impl.h:32: virtual void Initialize() override; On 2014/10/22 16:34:35, Daniel Erat wrote: > nit: remove 'virtual' for all of these too since you have 'override' Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_player_unittest.cc (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_player_unittest.cc:133: AudioPlayerImpl* player_; On 2014/10/22 16:34:35, Daniel Erat wrote: > nit: document ownership Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:36: virtual ~AudioRecorder(){}; On 2014/10/22 16:34:35, Daniel Erat wrote: > nit: > > virtual ~AudioRecorder() {} git-cl-format is doing this automatically for some reason. I've fixed it for now, but this may get to this state again in some future check-in due to this. Done. https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... components/copresence/public/copresence_constants.h:38: UNKNOWN = 3, On 2014/10/22 16:34:35, Daniel Erat wrote: > might've mentioned this already, but "UNKNOWN" seems like an overly-vague value > to put at the top level of the copresence namespace. i think it'd be better to > rename these to AUDIO_AUDIBLE, AUDIO_INAUDIBLE, AUDIO_BOTH, and AUDIO_UNKNOWN > (or AUDIO_TYPE_*). So the enum name is the documentation that this is an audio type. If it isn't clear at places due to using the enum directly, we can just add AudioType::UNKNOWN or AudioType::AUDIBLE to achieve the same effect? I believe this is now legal in C++11?
On 2014/10/22 16:50:59, Charlie wrote: > https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... > File components/copresence/mediums/audio/audio_manager.h (right): > > https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... > components/copresence/mediums/audio/audio_manager.h:44: virtual void > Initialize(const DecodeSamplesCallback& decode_cb, > On 2014/10/22 14:08:49, Rahul Chaturvedi wrote: > > On 2014/10/22 01:03:29, Charlie wrote: > > > On 2014/10/22 00:47:08, Rahul Chaturvedi wrote: > > > > On 2014/10/22 00:28:06, Charlie wrote: > > > > > On 2014/10/22 00:06:20, Rahul Chaturvedi wrote: > > > > > > On 2014/10/21 19:43:22, Daniel Erat wrote: > > > > > > > as discussed earlier, does this need to be part of the interface? > > > > > > > > > > > > I changed my code around to remove Initialize from the interface and > > move > > > it > > > > > > only to the Impl classes. This broke one of my tests, without it > > actually > > > > > being > > > > > > a bug in my code at all. The code which did not call Initialize for > the > > > stub > > > > > > classes, made it so that the decode_callback was never set. The > > > > > > AudioManagerTest.Record test expects that when TriggerDecodeRequest() > on > > > the > > > > > > stub recorder is called, correct program behavior will try to decode > > > > samples. > > > > > > Since we never initialized the stub recorder (which makes this > > > connection), > > > > > the > > > > > > code path never gets called and the test fails. > > > > > > > > > > > > I can of course fix this by moving the passing of the decode callback > to > > > the > > > > > > constructor but then I'd be making a change in production code that I > > > don't > > > > > > necessarily think is the optimal design, because of the insistence of > > not > > > > > having > > > > > > an Initialize method for stub classes also. > > > > > > > > > > > > This made me realize that Initialize *does* need to be part of stub > > > > > > implementations. Since we have paper thin constructors and do all our > > > > > > construction in Initialize, completely bypassing Initialize doesn't > > sound > > > > like > > > > > a > > > > > > good idea. For example, the AudioRecorder *does* need to be > initialized > > > and > > > > > does > > > > > > need to be initialized by the AudioManager. This is a behavior that we > > > want > > > > to > > > > > > specifically test. If we take out the Initialize methods, this code > path > > > is > > > > no > > > > > > longer being tested. > > > > > > > > > > > > Unless there is a really strong reason to remove Initialize, I'd > prefer > > to > > > > > keep > > > > > > it in. I don't see a particularly strong downside to letting a stub > > object > > > > > > initialize via the same interface method that the implementation > object > > > is. > > > > > > > > > > This is for another CL, but can you remind me why we have the > Initialize() > > > > calls > > > > > at all? Every time we call them, it seems follow this pattern: > > > > > > > > > > if (!foo_ptr_) foo_ptr_.reset(new Foo()); > > > > > foo_ptr_->Initialize(...); > > > > > > > > > > I also cannot really see what's complex enough about Initialize() that > it > > > > can't > > > > > be done in a constructor. This may actually be a holdover from the test > > > > classes > > > > > subclassing the prod impls: then we needed a way to set up the state > > > > differently > > > > > for testing. But we're not doing that anymore. > > > > > > > > > > Summary: Add a TODO to merge Initialize() into the constructors? > > > > > > > > We do this from tests all the time. We create the stub in the test, pass > it > > to > > > > the class. The class then calls Initialize() (which ends up either > > > initializing > > > > the real class or the stub). > > > > > > I see. But if Initialize() is really just about setting callbacks, maybe we > > > should have set_foo_callback() instead, and the class should be resilient to > > > null callbacks. > > > > > > As I said, though, this is for another CL. > > > > Initialize in this case is about setting callbacks (and connecting callbacks). > > The function does exactly what the name indicates - it initializes the class. > > Whether this involves setting callbacks or connecting them or doing any other > > setup required for the class to function correctly, it will be done here. I I > > guess I am not seeing what exactly is wrong with this pattern? > > One of the principles of encapsulation is that objects should always be in a > consistent state. This consistency is set up by the constructor and maintained > by all the other member functions. Having the constructor half-initialize the > object, and requiring a call to Initialize() before doing anything else, is > really a hack that we should use if there's no other way. It's also a way to > pretend there's no heavy lifting (e.g. initializing other objects) in the > constructor. See: > > http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ > > Tracked at P3 in crbug/424735. Or, have the constructor not initialize the object at all and have the Initialize() be the 'real' constructor and do 'all' the heavy lifting. This does not break the principles of encapsulation. An object on which Initialize hasn't been called yet has not been 'constructed' yet. This is the pattern we're following here. I am not particularly against having a factory ::Create method, but that approach (as much as Scott Meyers may love it) has its own drawbacks too. I am not in favor of refactoring for the sake of refactoring. Refactoring should be solving a real problem. Once we *have* a problem, let's figure out the best solution for it then. Till then this pattern works fine.
https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... File chrome/browser/copresence/chrome_whispernet_client.h (right): https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... chrome/browser/copresence/chrome_whispernet_client.h:41: virtual void Initialize(const SuccessCallback& init_callback) override; On 2014/10/22 18:21:46, Rahul Chaturvedi wrote: > On 2014/10/22 16:34:34, Daniel Erat wrote: > > nit: i think that the latest is that 'virtual' is supposed to be omitted when > > 'override' is present > > Ah they fixed the presubmit for this? > I'll go ahead and change it everywhere. might want to double-check first; i'm just parrotting what i saw on another review. :-P https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:32: base::TimeTicks end_time; On 2014/10/22 18:21:47, Rahul Chaturvedi wrote: > On 2014/10/22 16:34:35, Daniel Erat wrote: > > while i pretty much always support the use of TimeTicks rather than Time, just > > want to make sure: one difference (at least on cros) is that TimeTicks won't > > advance while the system is suspended. probably doesn't matter here, but > wanted > > to make sure you were aware of it. > > I read that in the documentation but don't think it effects this code. If > NowTicks will always return the number of ticks since epoch, whether a system > was suspended or not shoudln't make a difference, correct? i'm not aware of anything that returns ticks since the epoch -- is TimeTicks documented as representing that somewhere? at least on linux, i'm pretty sure it's nanoseconds since boot minus any intervals where the system was suspended. the main concern with using TimeTicks here would just be that you could end up with stale directives if e.g. the system suspended for a long time. base::Time has its own problems, in that it'll get messed up if the user (or an NTP daemon) changes the system clock. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:46: virtual const std::string& GetToken(AudioType type) = 0; On 2014/10/22 18:21:47, Rahul Chaturvedi wrote: > On 2014/10/22 16:34:35, Daniel Erat wrote: > > if the returned value is short, please just return std::string directly > > Any particular reason for not returning a const reference? it is a const, so the > returned reference won't be changed (without an explicit const cast anyway at > least). in general, i find that it's better not to impose restrictions on implementations of an interface. for example, i usually avoid const methods in interfaces since it precludes future implementations from caching values internally. in this case, i think that returning a const reference meant that a stub implementation needed to have a weird empty string member that it never sent to anything. if the method isn't called over and over, it may be cleaner to just return a copy. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:36: virtual ~AudioRecorder(){}; On 2014/10/22 18:21:48, Rahul Chaturvedi wrote: > On 2014/10/22 16:34:35, Daniel Erat wrote: > > nit: > > > > virtual ~AudioRecorder() {} > > git-cl-format is doing this automatically for some reason. I've fixed it for > now, but this may get to this state again in some future check-in due to this. > Done. cool, thanks. does that run clang_format.py internally? i've been frustrated in the past by places where it seems to differ from the style that's used everywhere else in chrome. https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... components/copresence/public/copresence_constants.h:38: UNKNOWN = 3, On 2014/10/22 18:21:48, Rahul Chaturvedi wrote: > On 2014/10/22 16:34:35, Daniel Erat wrote: > > might've mentioned this already, but "UNKNOWN" seems like an overly-vague > value > > to put at the top level of the copresence namespace. i think it'd be better to > > rename these to AUDIO_AUDIBLE, AUDIO_INAUDIBLE, AUDIO_BOTH, and AUDIO_UNKNOWN > > (or AUDIO_TYPE_*). > > So the enum name is the documentation that this is an audio type. If it isn't > clear at places due to using the enum directly, we can just add > AudioType::UNKNOWN or AudioType::AUDIBLE to achieve the same effect? I believe > this is now legal in C++11? ah, i didn't know about the c++11 angle. my main concern was just that using UNKNOWN here would make it difficult to figure out the types of arguments passed by callers and prevent other enums in the namespace from using UNKNOWN.
On 2014/10/22 18:31:13, Rahul Chaturvedi wrote: > On 2014/10/22 16:50:59, Charlie wrote: > > > https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... > > File components/copresence/mediums/audio/audio_manager.h (right): > > > > > https://codereview.chromium.org/637223011/diff/160001/components/copresence/m... > > components/copresence/mediums/audio/audio_manager.h:44: virtual void > > Initialize(const DecodeSamplesCallback& decode_cb, > > On 2014/10/22 14:08:49, Rahul Chaturvedi wrote: > > > On 2014/10/22 01:03:29, Charlie wrote: > > > > On 2014/10/22 00:47:08, Rahul Chaturvedi wrote: > > > > > On 2014/10/22 00:28:06, Charlie wrote: > > > > > > On 2014/10/22 00:06:20, Rahul Chaturvedi wrote: > > > > > > > On 2014/10/21 19:43:22, Daniel Erat wrote: > > > > > > > > as discussed earlier, does this need to be part of the interface? > > > > > > > > > > > > > > I changed my code around to remove Initialize from the interface and > > > move > > > > it > > > > > > > only to the Impl classes. This broke one of my tests, without it > > > actually > > > > > > being > > > > > > > a bug in my code at all. The code which did not call Initialize for > > the > > > > stub > > > > > > > classes, made it so that the decode_callback was never set. The > > > > > > > AudioManagerTest.Record test expects that when > TriggerDecodeRequest() > > on > > > > the > > > > > > > stub recorder is called, correct program behavior will try to decode > > > > > samples. > > > > > > > Since we never initialized the stub recorder (which makes this > > > > connection), > > > > > > the > > > > > > > code path never gets called and the test fails. > > > > > > > > > > > > > > I can of course fix this by moving the passing of the decode > callback > > to > > > > the > > > > > > > constructor but then I'd be making a change in production code that > I > > > > don't > > > > > > > necessarily think is the optimal design, because of the insistence > of > > > not > > > > > > having > > > > > > > an Initialize method for stub classes also. > > > > > > > > > > > > > > This made me realize that Initialize *does* need to be part of stub > > > > > > > implementations. Since we have paper thin constructors and do all > our > > > > > > > construction in Initialize, completely bypassing Initialize doesn't > > > sound > > > > > like > > > > > > a > > > > > > > good idea. For example, the AudioRecorder *does* need to be > > initialized > > > > and > > > > > > does > > > > > > > need to be initialized by the AudioManager. This is a behavior that > we > > > > want > > > > > to > > > > > > > specifically test. If we take out the Initialize methods, this code > > path > > > > is > > > > > no > > > > > > > longer being tested. > > > > > > > > > > > > > > Unless there is a really strong reason to remove Initialize, I'd > > prefer > > > to > > > > > > keep > > > > > > > it in. I don't see a particularly strong downside to letting a stub > > > object > > > > > > > initialize via the same interface method that the implementation > > object > > > > is. > > > > > > > > > > > > This is for another CL, but can you remind me why we have the > > Initialize() > > > > > calls > > > > > > at all? Every time we call them, it seems follow this pattern: > > > > > > > > > > > > if (!foo_ptr_) foo_ptr_.reset(new Foo()); > > > > > > foo_ptr_->Initialize(...); > > > > > > > > > > > > I also cannot really see what's complex enough about Initialize() that > > it > > > > > can't > > > > > > be done in a constructor. This may actually be a holdover from the > test > > > > > classes > > > > > > subclassing the prod impls: then we needed a way to set up the state > > > > > differently > > > > > > for testing. But we're not doing that anymore. > > > > > > > > > > > > Summary: Add a TODO to merge Initialize() into the constructors? > > > > > > > > > > We do this from tests all the time. We create the stub in the test, pass > > it > > > to > > > > > the class. The class then calls Initialize() (which ends up either > > > > initializing > > > > > the real class or the stub). > > > > > > > > I see. But if Initialize() is really just about setting callbacks, maybe > we > > > > should have set_foo_callback() instead, and the class should be resilient > to > > > > null callbacks. > > > > > > > > As I said, though, this is for another CL. > > > > > > Initialize in this case is about setting callbacks (and connecting > callbacks). > > > The function does exactly what the name indicates - it initializes the > class. > > > Whether this involves setting callbacks or connecting them or doing any > other > > > setup required for the class to function correctly, it will be done here. I > I > > > guess I am not seeing what exactly is wrong with this pattern? > > > > One of the principles of encapsulation is that objects should always be in a > > consistent state. This consistency is set up by the constructor and maintained > > by all the other member functions. Having the constructor half-initialize the > > object, and requiring a call to Initialize() before doing anything else, is > > really a hack that we should use if there's no other way. It's also a way to > > pretend there's no heavy lifting (e.g. initializing other objects) in the > > constructor. See: > > > > http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ > > > > Tracked at P3 in crbug/424735. > > Or, have the constructor not initialize the object at all and have the > Initialize() be the 'real' constructor and do 'all' the heavy lifting. This does > not break the principles of encapsulation. An object on which Initialize hasn't > been called yet has not been 'constructed' yet. This is the pattern we're > following here. I am not particularly against having a factory ::Create method, > but that approach (as much as Scott Meyers may love it) has its own drawbacks > too. I am not in favor of refactoring for the sake of refactoring. Refactoring > should be solving a real problem. Once we *have* a problem, let's figure out the > best solution for it then. Till then this pattern works fine. I don't really see the point of having a constructor that doesn't actually construct the object. Having a private constructor and a Create() method is a self-documenting and self-enforcing way of making sure an object is *actually* constructed before it gets used. I think you'd actually agree that we have to call Initialize() before doing anything else with the object. So why give the option *not* to do this? "Fix the problem once we have it" sounds a little too much like "wait for there to be a fire instead of preventing one". All that being said, I'll repeat that this CL is solving other, more immediate problems. Let's get it checked in, and we can refactor when we have time. LGTM
https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... File chrome/browser/copresence/chrome_whispernet_client.h (right): https://codereview.chromium.org/637223011/diff/180001/chrome/browser/copresen... chrome/browser/copresence/chrome_whispernet_client.h:41: virtual void Initialize(const SuccessCallback& init_callback) override; On 2014/10/22 18:32:39, Daniel Erat wrote: > On 2014/10/22 18:21:46, Rahul Chaturvedi wrote: > > On 2014/10/22 16:34:34, Daniel Erat wrote: > > > nit: i think that the latest is that 'virtual' is supposed to be omitted > when > > > 'override' is present > > > > Ah they fixed the presubmit for this? > > I'll go ahead and change it everywhere. > > might want to double-check first; i'm just parrotting what i saw on another > review. :-P Seems to be; someone also did a massive refactor changing this across the code base. https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:32: base::TimeTicks end_time; On 2014/10/22 18:32:40, Daniel Erat wrote: > On 2014/10/22 18:21:47, Rahul Chaturvedi wrote: > > On 2014/10/22 16:34:35, Daniel Erat wrote: > > > while i pretty much always support the use of TimeTicks rather than Time, > just > > > want to make sure: one difference (at least on cros) is that TimeTicks won't > > > advance while the system is suspended. probably doesn't matter here, but > > wanted > > > to make sure you were aware of it. > > > > I read that in the documentation but don't think it effects this code. If > > NowTicks will always return the number of ticks since epoch, whether a system > > was suspended or not shoudln't make a difference, correct? > > i'm not aware of anything that returns ticks since the epoch -- is TimeTicks > documented as representing that somewhere? at least on linux, i'm pretty sure > it's nanoseconds since boot minus any intervals where the system was suspended. > > the main concern with using TimeTicks here would just be that you could end up > with stale directives if e.g. the system suspended for a long time. base::Time > has its own problems, in that it'll get messed up if the user (or an NTP daemon) > changes the system clock. So the posix implementation of Time::Now() returns time since Windows epoch. This is really strange though, the Windows implementation of Time::Now() returns TimeTicks::Now(), and I have no clue what mac is even doing. Considering that TimeTicks seem to be based on the high resolution timer and are guaranteed not to go backwards, I'll leave this as TimeTicks for now. Creating a bug for this and linked it here to investigate this further to figure out what time we really need to use; and what code needs to be written to compensate for system suspends. https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:46: virtual const std::string& GetToken(AudioType type) = 0; On 2014/10/22 18:32:40, Daniel Erat wrote: > On 2014/10/22 18:21:47, Rahul Chaturvedi wrote: > > On 2014/10/22 16:34:35, Daniel Erat wrote: > > > if the returned value is short, please just return std::string directly > > > > Any particular reason for not returning a const reference? it is a const, so > the > > returned reference won't be changed (without an explicit const cast anyway at > > least). > > in general, i find that it's better not to impose restrictions on > implementations of an interface. for example, i usually avoid const methods in > interfaces since it precludes future implementations from caching values > internally. in this case, i think that returning a const reference meant that a > stub implementation needed to have a weird empty string member that it never > sent to anything. if the method isn't called over and over, it may be cleaner to > just return a copy. So this code will be called 'fairly' often; considering that every time we upload a new token, we upload our currently playing tokens to - so this is something that will at least get called once every half a second - but that's a lifetime in terms of actual cycles. If we change this to return a new string though, the function that calls this (which also returns a reference) and the function that calls that, will all need to be changed, and now this will return a copy, which will return a copy, which will return a copy; which starts to add up. If the side-effect of this is a some weird code in a test (which will never actually get called, even in that test), then this should be fine? https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:36: virtual ~AudioRecorder(){}; On 2014/10/22 18:32:40, Daniel Erat wrote: > On 2014/10/22 18:21:48, Rahul Chaturvedi wrote: > > On 2014/10/22 16:34:35, Daniel Erat wrote: > > > nit: > > > > > > virtual ~AudioRecorder() {} > > > > git-cl-format is doing this automatically for some reason. I've fixed it for > > now, but this may get to this state again in some future check-in due to this. > > Done. > > cool, thanks. does that run clang_format.py internally? i've been frustrated in > the past by places where it seems to differ from the style that's used > everywhere else in chrome. Yep. Usually it is a significant convenience; unfortunately in some cases randomly it gives really weird results that need to be constantly fixed up. Luckily it only works on code you change, so once the code is checked in, hopefully it won't mess it up again. https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/p... components/copresence/public/copresence_constants.h:38: UNKNOWN = 3, On 2014/10/22 18:32:40, Daniel Erat wrote: > On 2014/10/22 18:21:48, Rahul Chaturvedi wrote: > > On 2014/10/22 16:34:35, Daniel Erat wrote: > > > might've mentioned this already, but "UNKNOWN" seems like an overly-vague > > value > > > to put at the top level of the copresence namespace. i think it'd be better > to > > > rename these to AUDIO_AUDIBLE, AUDIO_INAUDIBLE, AUDIO_BOTH, and > AUDIO_UNKNOWN > > > (or AUDIO_TYPE_*). > > > > So the enum name is the documentation that this is an audio type. If it isn't > > clear at places due to using the enum directly, we can just add > > AudioType::UNKNOWN or AudioType::AUDIBLE to achieve the same effect? I believe > > this is now legal in C++11? > > ah, i didn't know about the c++11 angle. my main concern was just that using > UNKNOWN here would make it difficult to figure out the types of arguments passed > by callers and prevent other enums in the namespace from using UNKNOWN. Ah yes, that would still be a problem even despite scoping. C++ won't let us declare two different values, despite the fact that people can scope enums (since unscoped enums are still allowed anyway). Okay so at least renaming UNKNOWN makes sense since it _is a common type that we may conflict. AUDIBLE/INAUDIBLE and BOTH are fairly unlikely to develop conflicts in this namespace.
LGTM Sorry, I forgot about this CL.
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637223011/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/72224) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637223011/260001
https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... File components/copresence/mediums/audio/audio_manager.h (right): https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... components/copresence/mediums/audio/audio_manager.h:46: virtual const std::string& GetToken(AudioType type) = 0; On 2014/10/22 19:28:46, Rahul Chaturvedi wrote: > On 2014/10/22 18:32:40, Daniel Erat wrote: > > On 2014/10/22 18:21:47, Rahul Chaturvedi wrote: > > > On 2014/10/22 16:34:35, Daniel Erat wrote: > > > > if the returned value is short, please just return std::string directly > > > > > > Any particular reason for not returning a const reference? it is a const, so > > the > > > returned reference won't be changed (without an explicit const cast anyway > at > > > least). > > > > in general, i find that it's better not to impose restrictions on > > implementations of an interface. for example, i usually avoid const methods in > > interfaces since it precludes future implementations from caching values > > internally. in this case, i think that returning a const reference meant that > a > > stub implementation needed to have a weird empty string member that it never > > sent to anything. if the method isn't called over and over, it may be cleaner > to > > just return a copy. > > So this code will be called 'fairly' often; considering that every time we > upload a new token, we upload our currently playing tokens to - so this is > something that will at least get called once every half a second - but that's a > lifetime in terms of actual cycles. If we change this to return a new string > though, the function that calls this (which also returns a reference) and the > function that calls that, will all need to be changed, and now this will return > a copy, which will return a copy, which will return a copy; which starts to add > up. > > If the side-effect of this is a some weird code in a test (which will never > actually get called, even in that test), then this should be fine? my larger point was that specifying a reference return value in the interface restricts what implementations can do. if a future implementation wants to generate a random token, or return a token that it fetches from some other class that doesn't return a const reference, suddenly it needs to find some way to hold on to the memory until the caller is done using it (how will it know when that's happened?). and this case seems particularly bad, since there's a SetToken() method. i can imagine implementations where calling that would invalidate previously-handed-out references. the current implementation avoids this by using an array of std::strings, but it could just as easily be using a vector. so i'll go further: only return references if the lifetime of the underlying memory is very clear (e.g. a read-only member that gets set in the c'tor of a non-derived class).
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2014/10/22 21:50:15, Daniel Erat wrote: > https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... > File components/copresence/mediums/audio/audio_manager.h (right): > > https://codereview.chromium.org/637223011/diff/180001/components/copresence/m... > components/copresence/mediums/audio/audio_manager.h:46: virtual const > std::string& GetToken(AudioType type) = 0; > On 2014/10/22 19:28:46, Rahul Chaturvedi wrote: > > On 2014/10/22 18:32:40, Daniel Erat wrote: > > > On 2014/10/22 18:21:47, Rahul Chaturvedi wrote: > > > > On 2014/10/22 16:34:35, Daniel Erat wrote: > > > > > if the returned value is short, please just return std::string directly > > > > > > > > Any particular reason for not returning a const reference? it is a const, > so > > > the > > > > returned reference won't be changed (without an explicit const cast anyway > > at > > > > least). > > > > > > in general, i find that it's better not to impose restrictions on > > > implementations of an interface. for example, i usually avoid const methods > in > > > interfaces since it precludes future implementations from caching values > > > internally. in this case, i think that returning a const reference meant > that > > a > > > stub implementation needed to have a weird empty string member that it never > > > sent to anything. if the method isn't called over and over, it may be > cleaner > > to > > > just return a copy. > > > > So this code will be called 'fairly' often; considering that every time we > > upload a new token, we upload our currently playing tokens to - so this is > > something that will at least get called once every half a second - but that's > a > > lifetime in terms of actual cycles. If we change this to return a new string > > though, the function that calls this (which also returns a reference) and the > > function that calls that, will all need to be changed, and now this will > return > > a copy, which will return a copy, which will return a copy; which starts to > add > > up. > > > > If the side-effect of this is a some weird code in a test (which will never > > actually get called, even in that test), then this should be fine? > > my larger point was that specifying a reference return value in the interface > restricts what implementations can do. if a future implementation wants to > generate a random token, or return a token that it fetches from some other class > that doesn't return a const reference, suddenly it needs to find some way to > hold on to the memory until the caller is done using it (how will it know when > that's happened?). > > and this case seems particularly bad, since there's a SetToken() method. i can > imagine implementations where calling that would invalidate > previously-handed-out references. the current implementation avoids this by > using an array of std::strings, but it could just as easily be using a vector. > > so i'll go further: only return references if the lifetime of the underlying > memory is very clear (e.g. a read-only member that gets set in the c'tor of a > non-derived class). That makes sense. Changed.
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637223011/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637223011/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/a05223cb296cb5301f043d10867c027aa56ebff3 Cr-Commit-Position: refs/heads/master@{#300825} |
