Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
419073002:
Add the copresence DirectiveHandler. (Closed)
|
Created:
6 years, 5 months ago by rkc Modified:
6 years, 4 months ago CC:
chromium-reviews, Charlie Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionThis is the preliminary introduction for the code for the chrome.copresence
API which enables the exchange short messages with nearby devices.
This is the audio handling code of the copresence core component. We'll be
open sourcing this code in phases, this is the first part. This CL includes
the code for the directive handler for copresence, and all related classes.
Including blundell@ for the OWNERs review to add this component.
R=blundell@chromium.org, derat@chromium.org, xiyuan@chromium.org
BUG=365493
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287900
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 162
Patch Set 3 : review comments #
Total comments: 4
Patch Set 4 : #Patch Set 5 : audio_test_support files #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : gn fix #
Total comments: 6
Patch Set 9 : #
Total comments: 71
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 14
Patch Set 13 : #Patch Set 14 : build fixes #Patch Set 15 : #Patch Set 16 : #Messages
Total messages: 51 (0 generated)
(are you planning to break this up into smaller pieces? it sounded like it earlier.)
I didn't look at the source files under //components/copresence. https://codereview.chromium.org/419073002/diff/1/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/419073002/diff/1/components/copresence.gypi#n... components/copresence.gypi:8: 'target_name': 'copresence_core', This target can just be named copresence. https://codereview.chromium.org/419073002/diff/1/components/copresence.gypi#n... components/copresence.gypi:16: 'copresence_proto', Are you including any of the generated pb.h headers in public headers of the copresence code? If so, you need to export_dependent_settings on copresence_proto to ensure that the proto headers are generated before the code that depends on this target is built.
@blundell, mostly need your review to verify that the component is being added correctly :) https://codereview.chromium.org/419073002/diff/1/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/419073002/diff/1/components/copresence.gypi#n... components/copresence.gypi:8: 'target_name': 'copresence_core', On 2014/07/25 07:33:57, blundell (OOO until August 6) wrote: > This target can just be named copresence. Done. https://codereview.chromium.org/419073002/diff/1/components/copresence.gypi#n... components/copresence.gypi:16: 'copresence_proto', On 2014/07/25 07:33:57, blundell (OOO until August 6) wrote: > Are you including any of the generated pb.h headers in public headers of the > copresence code? If so, you need to export_dependent_settings on > copresence_proto to ensure that the proto headers are generated before the code > that depends on this target is built. Done.
Adding dalecurtis@ for the audio code.
lgtm % method naming. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:185: base::RunLoop rl; This isn't allowed in production code. See base/run_loop.h. If this is test only code, rename these methods so that they have "ForTesting" in the name or ideally move the code to the test. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:253: if (media::AudioManager::Get()->GetTaskRunner()->BelongsToCurrentThread()) Ditto.
Mostly nits. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... File components/copresence/common/copresence_constants.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/copresence_constants.cc:10: const float kDefaultSampleRate = 48000.0; nit: 'f' for float literals to avoid compiler warning, e.g. 48000.0f https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/copresence_constants.cc:12: const float kDefaultCarrierFrequency = 18500.0; nit: 18500.0f https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... File components/copresence/common/timed_map.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:53: expiry_queue_.top().second >= base::Time::Now()) Should this be "<" to trim keys whose expire time is before now? https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:66: // The earlier end_time should be the 'higher' value. The comment needs to be updated. What is |end_time|? https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:76: const ValueType kEmptyValue; Can we make this a "static" member? Carrying one with each instance seems a bit wasteful. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:9: #include "base/message_loop/message_loop.h" nit: not used? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:24: base::Bind(&AudioDirectiveHandler::ExecuteNextTransmit, nit: #include "base/bind.h" https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:65: default: nit: If we don't use "default" here, the compiler would catch missing cases. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:77: if (transmit.get()) nit: get rid of ".get()" https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:83: if (receive.get()) nit: get rid of ".get()" https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:35: explicit AudioDirectiveHandler( nit: If we only want to expose it for testing, maybe we should put this ctor in private section and add a class factory method that ends with 'ForTesting'. This way presubmit script would catch misuse. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:43: virtual void PlayAudio( It seems that these are made "virtual" because we need to mock them in tests. Could you add a comment? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:45: base::TimeDelta duration); nit: const base::TimeDelta& ? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:46: virtual void RecordAudio(base::TimeDelta duration); nit: const base::TimeDelta& ? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:79: }; nit: private: DISALLOW_COPY_AND_ASSIGN(AudioDirectiveHandlerTest); https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:112: while (!list->empty() && list->top().end_time < base::Time::Now()) AudioDirectiveQueue is sorted by LatestFirstComparator, which should put the latest record (greatest end_time) at the beginning. If the greatest end_time is less than now, can we just clear the whole list? Am I missing anything? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:41: scoped_refptr<media::AudioBusRefCounted> samples; #include "base/memory/ref_counted.h" https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:66: base::TimeDelta ttl); nit: const base::TimeDelta& ? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:68: void AddReceiveDirective(const std::string& op_id, base::TimeDelta ttl); nit: const base::TimeDelta& ? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:8: #include <queue> nit: not used? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:31: virtual ~DirectiveHandler(); nit: No need to be "virtual" if the class is not intended to be inherited. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:24: } nit: insert an empty line before and add " // namespace" https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:23: const float kProcessIntervalMs = 500.0; // milliseconds. nit: 500.0f https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... File components/copresence/proto/codes.proto (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/codes.proto:1: syntax = "proto2"; nit: Do we have style guild for proto buf definition? The file looks a bit crowded. Let's add a few empty lines to break it up into sections.
here are some initial comments for the first few files. i'll find out soon if i'm called in for jury duty next week, which will determine how quickly i can go through the rest of the files. :-/ https://codereview.chromium.org/419073002/diff/20001/components/OWNERS File components/OWNERS (right): https://codereview.chromium.org/419073002/diff/20001/components/OWNERS#newcode33 components/OWNERS:33: per-file copresence.gypi=derat@chromium.org please move my name to the end of the list since i won't know much about this code compared to the other people :-P https://codereview.chromium.org/419073002/diff/20001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence.gy... components/copresence.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. just to double-check: do you need a BUILD.gn file as well, or will that be automatically generated? https://codereview.chromium.org/419073002/diff/20001/components/copresence/OW... File components/copresence/OWNERS (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/OW... components/copresence/OWNERS:1: derat@chromium.org please move me to the end here too (if you think i need to be in it at all) https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... File components/copresence/common/copresence_constants.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/copresence_constants.h:12: extern const int kDefaultRepetitions; nit: add a brief comment documenting what each of these is used for (or better, make them be public static members of the classes that they're most closely associated with if possible) https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... File components/copresence/common/timed_map.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:19: // key/value pair after their specified lifetime is over. nit: s/their/its/ https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:34: expiry_queue_.push(KeyTimeTuple(key, base::Time::Now() + lifetime_)); make this use a base::TickClock instead of calling base::Time::Now() so it'll be testable https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:40: ClearExpiredTokens(); mutating the map here and in GetValue() seems a bit strange. why not make these methods const and then reset |timer_| such that it'll fire when the oldest entry in the map is due to expire? https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:66: // The earlier end_time should be the 'higher' value. nit: s/end_time/end time/ (since i don't think this is referring to a variable name) https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:73: std::vector<KeyTimeTuple>, can you unwrap this? it should fit on the end of the previous line https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:82: ExpiryQueue expiry_queue_; add a comment documenting what this contains https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:47: const copresence::TokenInstruction& instruction, isn't all this code already in the copresence namespace? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:50: case copresence::TRANSMIT: please rename these enums to be more descriptive, e.g. TRANSMIT_INSTRUCTION, RECEIVE_INSTRUCTION, etc. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:65: default: On 2014/07/25 21:02:09, xiyuan wrote: > nit: If we don't use "default" here, the compiler would catch missing cases. yeah, but then the code won't be able to detect invalid values that don't correspond to valid enum values :-( https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:69: // ExecuteNextTransmit will be called by the directive_list_ when Add is done. s/the directive_list_/|directive_list_|/ https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:87: void AudioDirectiveHandler::PlayAudio( please reorder the methods in the .cc file to match the order in the .h file https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:35: explicit AudioDirectiveHandler( On 2014/07/25 21:02:09, xiyuan wrote: > nit: If we only want to expose it for testing, maybe we should put this ctor in > private section and add a class factory method that ends with 'ForTesting'. This > way presubmit script would catch misuse. what about just having a single c'tor and documenting that passing a null |decode_cb| will prevent |player_| and |recorder_| from being initialized? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:39: void AddInstruction(const copresence::TokenInstruction& instruction, document what this does https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:45: base::TimeDelta duration); On 2014/07/25 21:02:09, xiyuan wrote: > nit: const base::TimeDelta& ? (not really necessary; time structs are just int64s and there's a bunch of code that passes them by value) https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:52: base::Time Now(); i don't think you define this anywhere https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:55: void ExecuteNextTransmit(); add a comment documenting what these do https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:60: AudioPlayer* player_; document that these are owned by this class but destroyed on another thread. (actually, i would very strongly prefer that they were scoped_ptr or scoped_refptrs or something; haven't gotten to these classes yet to see if that's possible.)
https://codereview.chromium.org/419073002/diff/20001/components/OWNERS File components/OWNERS (right): https://codereview.chromium.org/419073002/diff/20001/components/OWNERS#newcode33 components/OWNERS:33: per-file copresence.gypi=derat@chromium.org On 2014/07/25 23:09:00, Daniel Erat wrote: > please move my name to the end of the list since i won't know much about this > code compared to the other people :-P I had sorted it alphabetically :) Changed the order to indicate preference of reviewer. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence.gy... components/copresence.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/07/25 23:09:00, Daniel Erat wrote: > just to double-check: do you need a BUILD.gn file as well, or will that be > automatically generated? I'll ping blundell/jochen to find out. https://codereview.chromium.org/419073002/diff/20001/components/copresence/OW... File components/copresence/OWNERS (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/OW... components/copresence/OWNERS:1: derat@chromium.org On 2014/07/25 23:09:00, Daniel Erat wrote: > please move me to the end here too (if you think i need to be in it at all) Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... File components/copresence/common/copresence_constants.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/copresence_constants.cc:10: const float kDefaultSampleRate = 48000.0; On 2014/07/25 21:02:08, xiyuan wrote: > nit: 'f' for float literals to avoid compiler warning, e.g. 48000.0f Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/copresence_constants.cc:12: const float kDefaultCarrierFrequency = 18500.0; On 2014/07/25 21:02:08, xiyuan wrote: > nit: 18500.0f Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... File components/copresence/common/copresence_constants.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/copresence_constants.h:12: extern const int kDefaultRepetitions; On 2014/07/25 23:09:00, Daniel Erat wrote: > nit: add a brief comment documenting what each of these is used for (or better, > make them be public static members of the classes that they're most closely > associated with if possible) We eventually want to make these values configurable, in which case I'll remove them from here. Added a TODO and detailed comments for each field. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... File components/copresence/common/timed_map.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:19: // key/value pair after their specified lifetime is over. On 2014/07/25 23:09:00, Daniel Erat wrote: > nit: s/their/its/ Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:34: expiry_queue_.push(KeyTimeTuple(key, base::Time::Now() + lifetime_)); On 2014/07/25 23:09:00, Daniel Erat wrote: > make this use a base::TickClock instead of calling base::Time::Now() so it'll be > testable Using TimeTicks versus TimeDelta and Time seems, awkward. Still, this seems to be the only way to make this testable so changing it. I'll discuss this more offline with you. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:40: ClearExpiredTokens(); On 2014/07/25 23:09:01, Daniel Erat wrote: > mutating the map here and in GetValue() seems a bit strange. why not make these > methods const and then reset |timer_| such that it'll fire when the oldest entry > in the map is due to expire? This method returns synchronously, so even if we reset the timer, it will only fire 'after' we return here. In which case, we'd have returned an entry that was already expired. Since whether an entry has expired or not is really only relevant when we're 'getting' a value, clearing expired entries during that process seemed natural. The timer is only useful to free memory after we don't need it (in the case of adding a massive number of entries, then not checking for them again well past the expiry time); we could completely get rid of it and be functionally equivalent, just use a bit more memory. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:53: expiry_queue_.top().second >= base::Time::Now()) On 2014/07/25 21:02:08, xiyuan wrote: > Should this be "<" to trim keys whose expire time is before now? This is a bug, fixed. Also added unit tests. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:66: // The earlier end_time should be the 'higher' value. On 2014/07/25 21:02:08, xiyuan wrote: > The comment needs to be updated. What is |end_time|? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:66: // The earlier end_time should be the 'higher' value. On 2014/07/25 23:09:00, Daniel Erat wrote: > nit: s/end_time/end time/ (since i don't think this is referring to a variable > name) Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:73: std::vector<KeyTimeTuple>, On 2014/07/25 23:09:01, Daniel Erat wrote: > can you unwrap this? it should fit on the end of the previous line Done, though git cl format will probably break this up again. https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:76: const ValueType kEmptyValue; On 2014/07/25 21:02:08, xiyuan wrote: > Can we make this a "static" member? Carrying one with each instance seems a bit > wasteful. Can't have global or static const objects in Chrome :-\ https://codereview.chromium.org/419073002/diff/20001/components/copresence/co... components/copresence/common/timed_map.h:82: ExpiryQueue expiry_queue_; On 2014/07/25 23:09:01, Daniel Erat wrote: > add a comment documenting what this contains Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:9: #include "base/message_loop/message_loop.h" On 2014/07/25 21:02:08, xiyuan wrote: > nit: not used? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:24: base::Bind(&AudioDirectiveHandler::ExecuteNextTransmit, On 2014/07/25 21:02:09, xiyuan wrote: > nit: #include "base/bind.h" Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:47: const copresence::TokenInstruction& instruction, On 2014/07/25 23:09:01, Daniel Erat wrote: > isn't all this code already in the copresence namespace? Yep, whoops. This is a vestige of the open sourcing process, they weren't in the same namespace then :) Fixed. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:50: case copresence::TRANSMIT: On 2014/07/25 23:09:01, Daniel Erat wrote: > please rename these enums to be more descriptive, e.g. TRANSMIT_INSTRUCTION, > RECEIVE_INSTRUCTION, etc. The enums are created by the protoc compiler. We pull the protos automatically from the Copresence code, so any changes I'd make to them locally will get wiped anytime we pull again. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:65: default: On 2014/07/25 21:02:09, xiyuan wrote: > nit: If we don't use "default" here, the compiler would catch missing cases. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:65: default: On 2014/07/25 23:09:01, Daniel Erat wrote: > On 2014/07/25 21:02:09, xiyuan wrote: > > nit: If we don't use "default" here, the compiler would catch missing cases. > > yeah, but then the code won't be able to detect invalid values that don't > correspond to valid enum values :-( So.. I'll keep this as is then :) https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:69: // ExecuteNextTransmit will be called by the directive_list_ when Add is done. On 2014/07/25 23:09:01, Daniel Erat wrote: > s/the directive_list_/|directive_list_|/ Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:77: if (transmit.get()) On 2014/07/25 21:02:09, xiyuan wrote: > nit: get rid of ".get()" Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:83: if (receive.get()) On 2014/07/25 21:02:08, xiyuan wrote: > nit: get rid of ".get()" Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.cc:87: void AudioDirectiveHandler::PlayAudio( On 2014/07/25 23:09:01, Daniel Erat wrote: > please reorder the methods in the .cc file to match the order in the .h file Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:35: explicit AudioDirectiveHandler( On 2014/07/25 21:02:09, xiyuan wrote: > nit: If we only want to expose it for testing, maybe we should put this ctor in > private section and add a class factory method that ends with 'ForTesting'. This > way presubmit script would catch misuse. Made this protected, so only the Mock that overrides the class can actually use this constructor. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:35: explicit AudioDirectiveHandler( On 2014/07/25 23:09:01, Daniel Erat wrote: > On 2014/07/25 21:02:09, xiyuan wrote: > > nit: If we only want to expose it for testing, maybe we should put this ctor > in > > private section and add a class factory method that ends with 'ForTesting'. > This > > way presubmit script would catch misuse. > > what about just having a single c'tor and documenting that passing a null > |decode_cb| will prevent |player_| and |recorder_| from being initialized? Keeping a separate constructor makes it explicit that this is being instantiated for testing. Plus moving it into protected: allows us to make sure that we aren't accidentally using the incorrect constructor by passing in a null value for decode_cb. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:39: void AddInstruction(const copresence::TokenInstruction& instruction, On 2014/07/25 23:09:01, Daniel Erat wrote: > document what this does Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:43: virtual void PlayAudio( On 2014/07/25 21:02:09, xiyuan wrote: > It seems that these are made "virtual" because we need to mock them in tests. > Could you add a comment? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:45: base::TimeDelta duration); On 2014/07/25 21:02:09, xiyuan wrote: > nit: const base::TimeDelta& ? time.h specifies that these can be efficiently passed by value. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:45: base::TimeDelta duration); On 2014/07/25 23:09:01, Daniel Erat wrote: > On 2014/07/25 21:02:09, xiyuan wrote: > > nit: const base::TimeDelta& ? > > (not really necessary; time structs are just int64s and there's a bunch of code > that passes them by value) Acknowledged. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:46: virtual void RecordAudio(base::TimeDelta duration); On 2014/07/25 21:02:09, xiyuan wrote: > nit: const base::TimeDelta& ? ditto. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:52: base::Time Now(); On 2014/07/25 23:09:01, Daniel Erat wrote: > i don't think you define this anywhere Leftover code, removed. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:55: void ExecuteNextTransmit(); On 2014/07/25 23:09:01, Daniel Erat wrote: > add a comment documenting what these do Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler.h:60: AudioPlayer* player_; On 2014/07/25 23:09:01, Daniel Erat wrote: > document that these are owned by this class but destroyed on another thread. > (actually, i would very strongly prefer that they were scoped_ptr or > scoped_refptrs or something; haven't gotten to these classes yet to see if > that's possible.) These objects delete themselves on the audio thread (after we call Finalize on them). We can't delete them from the UI thread. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:79: }; On 2014/07/25 21:02:09, xiyuan wrote: > nit: > private: > DISALLOW_COPY_AND_ASSIGN(AudioDirectiveHandlerTest); Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:112: while (!list->empty() && list->top().end_time < base::Time::Now()) On 2014/07/25 21:02:09, xiyuan wrote: > AudioDirectiveQueue is sorted by LatestFirstComparator, which should put the > latest record (greatest end_time) at the beginning. If the greatest end_time is > less than now, can we just clear the whole list? Am I missing anything? No, you're right. Added a comment explaining that if our top token has expired, then we're essentially out of valid tokens in that list. The code is still similar though since priority queue doesn't have a clear method. Added a while loop that empties out the list, which removes the redundant time check. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:41: scoped_refptr<media::AudioBusRefCounted> samples; On 2014/07/25 21:02:09, xiyuan wrote: > #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:66: base::TimeDelta ttl); On 2014/07/25 21:02:09, xiyuan wrote: > nit: const base::TimeDelta& ? Ditto. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:68: void AddReceiveDirective(const std::string& op_id, base::TimeDelta ttl); On 2014/07/25 21:02:09, xiyuan wrote: > nit: const base::TimeDelta& ? Ditto. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:8: #include <queue> On 2014/07/25 21:02:09, xiyuan wrote: > nit: not used? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:31: virtual ~DirectiveHandler(); On 2014/07/25 21:02:09, xiyuan wrote: > nit: No need to be "virtual" if the class is not intended to be inherited. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:24: } On 2014/07/25 21:02:10, xiyuan wrote: > nit: insert an empty line before and add " // namespace" Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:185: base::RunLoop rl; On 2014/07/25 20:41:29, DaleCurtis wrote: > This isn't allowed in production code. See base/run_loop.h. If this is test > only code, rename these methods so that they have "ForTesting" in the name or > ideally move the code to the test. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:23: const float kProcessIntervalMs = 500.0; // milliseconds. On 2014/07/25 21:02:10, xiyuan wrote: > nit: 500.0f Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:253: if (media::AudioManager::Get()->GetTaskRunner()->BelongsToCurrentThread()) On 2014/07/25 20:41:29, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... File components/copresence/proto/codes.proto (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/codes.proto:1: syntax = "proto2"; On 2014/07/25 21:02:10, xiyuan wrote: > nit: Do we have style guild for proto buf definition? The file looks a bit > crowded. Let's add a few empty lines to break it up into sections. These files are auto-generated using Google3 tools. Any style changes we make will be lost at the next revision.
here are the rest of my comments on patch set 2; i'll hold off on making another pass for now https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:30: base::TimeDelta)); add OVERRIDE on all of these https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:49: void DirectiveAdded() {} i don't see this called anywhere. what's it used for? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:60: const copresence::TokenInstruction CreateTransmitInstruction( why is the return value const? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:68: const copresence::TokenInstruction CreateReceiveInstruction() { why is the return value const? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:75: // our the audio directive handler since, since the directive list s/since, since/since/ https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:82: const base::TimeDelta small_ttl = base::TimeDelta::FromMilliseconds(0x1337); nit: kSmallTtl, kLargeTtl (or TTL; don't think chrome is consistent here) https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:17: std::string FromUrlSafe(std::string token) { const std::string& https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:36: : op_id(op_id), end_time(end_time) { nit: one per line if they don't all fit on the same line as the signature https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:50: : token(token), op_id(op_id), end_time(end_time), samples(samples) { same here https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:133: pending_transmit_tokens_[token].samples = samples; use find() to avoid doing the same lookup three times https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:35: const scoped_refptr<media::AudioBusRefCounted>& samples); are four different c'tors really needed? actually, i think that one of them isn't defined https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:46: // that have outlived their ttl. nit: s/ttl/TTL/ https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list_unittest.cc:22: base::Bind(&AudioDirectiveListTest::DirectiveAdded, the test doesn't verify that this was called. why not just pass a null callback here and drop DirectiveAdded? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list_unittest.cc:30: void EncodeToken(const std::string& token, this looks like it's a copy of the code from audio_directive_handler_unittest.cc, plus the call to PopulateSamples(). can you move this into some shared testing library? https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:39: // Audio: nit: delete unnecessary comment https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:41: AudioPlayer::~AudioPlayer() { please use the same order in the .cc file as in the .h file https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:51: base::Bind(&AudioPlayer::InitializeOnAudioThread, how about adding a separate Init() method that does this so you can have a set_output_stream_for_testing() method instead of needing nearly-identical c'tors? https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:118: is_playing_ = true; the header has a comment saying that all members apart from stream_ are protected by state_lock_, but you're accessing a bunch of them outside of the lock. which is it? please add comments describing thread-safety in this class and make all of the methods contain DCHECK()s that make it clear which thread should be running them. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.h:29: explicit AudioPlayer(media::AudioOutputStream* output_stream_for_testing); document ownership of the stream https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.h:66: media::AudioOutputStream* stream_; document ownership of this object https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.h:72: // Index to the frame in the samples that we need to play next. nit: add blanks line before this line and between frame_index_ and is_playing_ https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player_unittest.cc:52: total_frames += frames; does this mean that total_frames can exceed max_frame_count_? is that a problem? https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player_unittest.cc:105: ASSERT_EQ(0, differences); is it worthwhile logging the indices of the frames that differ? https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player_unittest.cc:119: void PopulateSamples(int random_seed, size_t size, float* samples) { this seems like it's duplicated across multiple files too; please move it to a shared file https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:25: void AudioBusToString(scoped_ptr<media::AudioBus> source, std::string* buffer) { std::string seems strange to use for passing audio data around. can this just be a std::vector<float> instead? https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:110: media::AudioParameters params = just do: media::AudioParameters params = params_for_testing_ ? *params_for_testing_ : media::AudioManager::Get()->GetInputStreamParameters(...); ? https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:192: double /* volume */) { DCHECK which thread this is running on https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.h:34: AudioRecorder(const DecodeSamplesCallback& decode_callback, could this also be removed in favor of e.g. set_input_stream_for_testing() setters and a separate Init() method? https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.h:74: bool IsRecording(); IsRecordingForTesting() or IsRecordingForTest() (chrome appears to use both styles, unfortunately) https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.h:83: // Past the initialization, only access the next variables on nit: s/Past the initialization/Outside of the constructor/? https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... File components/copresence/proto/codes.proto (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/codes.proto:4: enum Code { why isn't this in enums.proto? https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/codes.proto:5: OK = 0; are these all documented somewhere else? https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... File components/copresence/proto/data.proto (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/data.proto:6: message ClientVersion { please add comments to this file describing what the different messages are used for https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/data.proto:17: optional PushService service = 1 [deprecated = true]; why are already-deprecated fields being added?
https://codereview.chromium.org/419073002/diff/40001/components/copresence/co... File components/copresence/common/timed_map_unittest.cc (right): https://codereview.chromium.org/419073002/diff/40001/components/copresence/co... components/copresence/common/timed_map_unittest.cc:27: base::MessageLoop ml_; nit: Seems not used. If it is used, give it a better name, e.g. loop_, message_loop_ etc https://codereview.chromium.org/419073002/diff/40001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/40001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:115: while (!list->empty() && list->top().end_time < base::Time::Now()) { Can we do: if (!list->empty() && list->top().end_time < base::Time::Now()) { AudioDirectiveQueue empty; list->swap(empty); }
https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_handler_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:30: base::TimeDelta)); On 2014/07/28 21:18:16, Daniel Erat wrote: > add OVERRIDE on all of these Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:49: void DirectiveAdded() {} On 2014/07/28 21:18:17, Daniel Erat wrote: > i don't see this called anywhere. what's it used for? Leftover code. Removed. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:60: const copresence::TokenInstruction CreateTransmitInstruction( On 2014/07/28 21:18:16, Daniel Erat wrote: > why is the return value const? Doesn't need to be. I think I had made it constref first, then realized that I was going to be returning a local variable. Fixed. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:68: const copresence::TokenInstruction CreateReceiveInstruction() { On 2014/07/28 21:18:16, Daniel Erat wrote: > why is the return value const? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:75: // our the audio directive handler since, since the directive list On 2014/07/28 21:18:16, Daniel Erat wrote: > s/since, since/since/ Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_handler_unittest.cc:82: const base::TimeDelta small_ttl = base::TimeDelta::FromMilliseconds(0x1337); On 2014/07/28 21:18:16, Daniel Erat wrote: > nit: kSmallTtl, kLargeTtl (or TTL; don't think chrome is consistent here) Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:17: std::string FromUrlSafe(std::string token) { On 2014/07/28 21:18:17, Daniel Erat wrote: > const std::string& I need to change the string in the function so I'd have to end up making a copy of the string anyway; hence decided to make by value and reuse the parameter. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:36: : op_id(op_id), end_time(end_time) { On 2014/07/28 21:18:17, Daniel Erat wrote: > nit: one per line if they don't all fit on the same line as the signature BTW, git cl format keeps overriding this to put them back on the same line. Moved then down for now, but some future usage of git cl format may put them back again here. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:50: : token(token), op_id(op_id), end_time(end_time), samples(samples) { On 2014/07/28 21:18:17, Daniel Erat wrote: > same here Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:133: pending_transmit_tokens_[token].samples = samples; On 2014/07/28 21:18:17, Daniel Erat wrote: > use find() to avoid doing the same lookup three times Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:35: const scoped_refptr<media::AudioBusRefCounted>& samples); On 2014/07/28 21:18:17, Daniel Erat wrote: > are four different c'tors really needed? > > actually, i think that one of them isn't defined Each of them has their own purpose, added comments explaining which is used for where. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.h:46: // that have outlived their ttl. On 2014/07/28 21:18:17, Daniel Erat wrote: > nit: s/ttl/TTL/ Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list_unittest.cc:22: base::Bind(&AudioDirectiveListTest::DirectiveAdded, On 2014/07/28 21:18:17, Daniel Erat wrote: > the test doesn't verify that this was called. why not just pass a null callback > here and drop DirectiveAdded? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list_unittest.cc:30: void EncodeToken(const std::string& token, On 2014/07/28 21:18:17, Daniel Erat wrote: > this looks like it's a copy of the code from > audio_directive_handler_unittest.cc, plus the call to PopulateSamples(). can you > move this into some shared testing library? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:39: // Audio: On 2014/07/28 21:18:17, Daniel Erat wrote: > nit: delete unnecessary comment Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:41: AudioPlayer::~AudioPlayer() { On 2014/07/28 21:18:18, Daniel Erat wrote: > please use the same order in the .cc file as in the .h file Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:51: base::Bind(&AudioPlayer::InitializeOnAudioThread, On 2014/07/28 21:18:17, Daniel Erat wrote: > how about adding a separate Init() method that does this so you can have a > set_output_stream_for_testing() method instead of needing nearly-identical > c'tors? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.cc:118: is_playing_ = true; On 2014/07/28 21:18:17, Daniel Erat wrote: > the header has a comment saying that all members apart from stream_ are > protected by state_lock_, but you're accessing a bunch of them outside of the > lock. which is it? please add comments describing thread-safety in this class > and make all of the methods contain DCHECK()s that make it clear which thread > should be running them. All the methods that must run on certain threads have DCHECK's. The only two fields protected by the lock are samples_ and frame_index_ (I've moved the fields around now so the comment about what the lock protects is accurate). samples_ and frame_index_ are not accessed from anywhere the lock isn't. All the code in this class except the public members (which are only trampolines to the AudioThread) run on the same thread. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.h:29: explicit AudioPlayer(media::AudioOutputStream* output_stream_for_testing); On 2014/07/28 21:18:18, Daniel Erat wrote: > document ownership of the stream Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.h:66: media::AudioOutputStream* stream_; On 2014/07/28 21:18:18, Daniel Erat wrote: > document ownership of this object Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player.h:72: // Index to the frame in the samples that we need to play next. On 2014/07/28 21:18:18, Daniel Erat wrote: > nit: add blanks line before this line and between frame_index_ and is_playing_ Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_player_unittest.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player_unittest.cc:52: total_frames += frames; On 2014/07/28 21:18:18, Daniel Erat wrote: > does this mean that total_frames can exceed max_frame_count_? is that a problem? If total_frames equals the max frame count, we break out of the loop. We will never exceed the max_frame_count_ since max_frame_count_ is a multiple of default_frame_count_. We also know that frames will always be the same as default_frame_count_ since the player will constantly play, so at no point, should we get less frames (or more) than default_frame_count_. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player_unittest.cc:105: ASSERT_EQ(0, differences); On 2014/07/28 21:18:18, Daniel Erat wrote: > is it worthwhile logging the indices of the frames that differ? Not really. If we have any differences at all, this test is a fail. Even knowing the number of differences is mildly helpful. There really should be 0 differences otherwise something is very, very wrong. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_player_unittest.cc:119: void PopulateSamples(int random_seed, size_t size, float* samples) { On 2014/07/28 21:18:18, Daniel Erat wrote: > this seems like it's duplicated across multiple files too; please move it to a > shared file Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder.cc (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:25: void AudioBusToString(scoped_ptr<media::AudioBus> source, std::string* buffer) { On 2014/07/28 21:18:18, Daniel Erat wrote: > std::string seems strange to use for passing audio data around. can this just be > a std::vector<float> instead? We'd have to end up converting this to a string anyway, since that is the form we pass ArrayBuffer's to JS from an extension. The original code passed around a std::vector<float> which involved about 1-2 extra copies of the entire data. This is much more efficient. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:110: media::AudioParameters params = On 2014/07/28 21:18:18, Daniel Erat wrote: > just do: > > media::AudioParameters params = params_for_testing_ ? > *params_for_testing_ : > media::AudioManager::Get()->GetInputStreamParameters(...); > > ? Doing that for the stream already, missed this one. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.cc:192: double /* volume */) { On 2014/07/28 21:18:18, Daniel Erat wrote: > DCHECK which thread this is running on This isn't necessarily going to be called on the default AudioThread, but can be called from a special thread optimized for performance. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.h:34: AudioRecorder(const DecodeSamplesCallback& decode_callback, On 2014/07/28 21:18:19, Daniel Erat wrote: > could this also be removed in favor of e.g. set_input_stream_for_testing() > setters and a separate Init() method? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.h:74: bool IsRecording(); On 2014/07/28 21:18:18, Daniel Erat wrote: > IsRecordingForTesting() or IsRecordingForTest() (chrome appears to use both > styles, unfortunately) This is all gone; replaced this with FlushAudioLoopForTesting. Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/me... components/copresence/mediums/audio/audio_recorder.h:83: // Past the initialization, only access the next variables on On 2014/07/28 21:18:18, Daniel Erat wrote: > nit: s/Past the initialization/Outside of the constructor/? Done. https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... File components/copresence/proto/codes.proto (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/codes.proto:4: enum Code { On 2014/07/28 21:18:19, Daniel Erat wrote: > why isn't this in enums.proto? This isn't code we control, this is pulled from Google3 directly. I'll ask the server folks to look into it. https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/codes.proto:5: OK = 0; On 2014/07/28 21:18:19, Daniel Erat wrote: > are these all documented somewhere else? In the original protos on Google3. https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... File components/copresence/proto/data.proto (right): https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/data.proto:6: message ClientVersion { On 2014/07/28 21:18:19, Daniel Erat wrote: > please add comments to this file describing what the different messages are used > for Comments are stripped by the tool that creates these buffers. Again, I can ask the folks who wrote the tool to look into not removing them. https://codereview.chromium.org/419073002/diff/20001/components/copresence/pr... components/copresence/proto/data.proto:17: optional PushService service = 1 [deprecated = true]; On 2014/07/28 21:18:19, Daniel Erat wrote: > why are already-deprecated fields being added? Since we're still using some of the values. In the RPC code (which will come in another CL), we have a TODO to remove all the deprecated fields and change our client code to match. https://codereview.chromium.org/419073002/diff/40001/components/copresence/co... File components/copresence/common/timed_map_unittest.cc (right): https://codereview.chromium.org/419073002/diff/40001/components/copresence/co... components/copresence/common/timed_map_unittest.cc:27: base::MessageLoop ml_; On 2014/07/29 00:22:26, xiyuan wrote: > nit: Seems not used. If it is used, give it a better name, e.g. loop_, > message_loop_ etc It needs to exist so we have a message loop that the timer can use; otherwise it'll crash. Renamed and comment added. https://codereview.chromium.org/419073002/diff/40001/components/copresence/ha... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/40001/components/copresence/ha... components/copresence/handlers/audio/audio_directive_list.cc:115: while (!list->empty() && list->top().end_time < base::Time::Now()) { On 2014/07/29 00:22:26, xiyuan wrote: > Can we do: > > if (!list->empty() && list->top().end_time < base::Time::Now()) { > AudioDirectiveQueue empty; > list->swap(empty); > } Done.
Adding jochen@ for owners review for components since blundell is OOO.
LGTM
not lgtm this needs a tracking bug. also, please split up the CL into reviewable chunks
Jochen, I'll get a tracking bug attached to this CL ASAP. It is 3am here and I need to check with the PMs in the morning before removing the restrict-view-label from the launch bug. Since all I really need from you is the review for adding this component, is it alright that I send you a separate CL that only adds this component's GYPs and the OWNERs files? The review of the total code will be very extensive with large amounts of code being added every day. Having a reviewer for _all_ the code for this component in a timezone that requires a day of turnaround will be infeasible. I can assure you that any code added to this component will definitely undergo thorough review by Chromium engineers here in MTV before it gets committed.
On 2014/07/29 at 09:57:03, rkc wrote: > Jochen, I'll get a tracking bug attached to this CL ASAP. It is 3am here and I need to check with the PMs in the morning before removing the restrict-view-label from the launch bug. Launch bugs are usually restrict-view-google, so that's fine > > Since all I really need from you is the review for adding this component, is it alright that I send you a separate CL that only adds this component's GYPs and the OWNERs files? no. That's not how this process works. > The review of the total code will be very extensive with large amounts of code being added every day. Having a reviewer for _all_ the code for this component in a timezone that requires a day of turnaround will be infeasible. I can assure you that any code added to this component will definitely undergo thorough review by Chromium engineers here in MTV before it gets committed. It wouldn't require turnaround if you'd split the CL up into functional units and have somebody review them before I can do the owners review. Here you're adding hundreds of lines of effectively dead code. I don't see how anybody could review that the API is correct or that the code otherwise does what it's supposed to do as is.
On 2014/07/29 11:31:40, jochen wrote: > On 2014/07/29 at 09:57:03, rkc wrote: > > Launch bugs are usually restrict-view-google, so that's fine Done, launch bug is now in the CL description. > > It wouldn't require turnaround if you'd split the CL up into functional units > and have somebody review them before I can do the owners review. I believe we may be defining functional units differently. By my definition, this is a complete and functional unit that is being exercised via tests. If this isn't a functional unit, I am not sure how breaking this up into smaller CLs will be functional units? :-\ It seems that you are saying that you want the code to be reviewed before you do the OWNERs review for the componentization. So this code _is reviewed by reviewers that were comfortable with the size of the CL. Since other reviewers have seen this code and either lgtm'ed it or in the process of reviewing it, would it be possible for you to do only a review of the code related to putting this code in src/components? > > Here you're adding hundreds of lines of effectively dead code. I don't see how > anybody could review that the API is correct or that the code otherwise does > what it's supposed to do as is. The reviewers have access to the full source in src-internal, design docs and I have personally spoken to them about the code, so they have more context than any other engineers (hence why I plan on using the same reviewers for all the code). Even if that weren't the case, this code by itself is one cohesive unit that, though not being used from anywhere in Chrome currently, is functional and tested. The plan for open sourcing this code is, .) Find reviewers comfortable with reviewing all the CLs as they go out, so the reviewers have context on the full code base. .) Send out the CLs in manageable units (I verified with the reviewers that they were comfortable with the size, and offered to split it up further if they thought the size to be unmanageable). .) Land lower level components first, then land the final API and the API test code that ties everything together and makes Copresence functional in Chrome. This CL is probably also one of the larger ones since it does land a lot of common code that the rest of the code depends on. I expect future CLs in src/components/copresence to be smaller. At this point, I really need someone to look at the code that adds the component; the GYP files, the organization, the OWNERs files, and can tell me if that code is correct. Being the only other OWNER of src/components and blundell@ currently OOO, I have turned to you for that review.
On 2014/07/29 at 17:03:43, rkc wrote: > On 2014/07/29 11:31:40, jochen wrote: > > On 2014/07/29 at 09:57:03, rkc wrote: > > > > Launch bugs are usually restrict-view-google, so that's fine > Done, launch bug is now in the CL description. thank you. I'm sorry if my comments about finding local reviewers were confusing. I meant to offer help with dealing with timezone differences. > The plan for open sourcing this code is, > .) Find reviewers comfortable with reviewing all the CLs as they go out, so the reviewers have context on the full code base. You also need to find the "right" reviewers, meaning that they're owners of the parts of the code you touch. > .) Send out the CLs in manageable units (I verified with the reviewers that they were comfortable with the size, and offered to split it up further if they thought the size to be unmanageable). I think the size is unmanageable. > .) Land lower level components first, then land the final API and the API test code that ties everything together and makes Copresence functional in Chrome. > > This CL is probably also one of the larger ones since it does land a lot of common code that the rest of the code depends on. I expect future CLs in src/components/copresence to be smaller. At this point, I really need someone to look at the code that adds the component; the GYP files, the organization, the OWNERs files, and can tell me if that code is correct. Being the only other OWNER of src/components and blundell@ currently OOO, I have turned to you for that review. I'm glad to help there. My feedback is that it's not possible to tell whether it's correct or not without seeing the APIs being used. I'd recommend to split up your code along those lines (the code that implements the APIs together with the code that uses the APIs as opposed to splitting it up by directory).
once you rebase this CL on top of https://codereview.chromium.org/426093003/ I think we're good to go.
please let me know when i should make another pass through this.
Uploaded the rebased patch. Tracking off https://codereview.chromium.org/426093003/ now.
lgtm for overall structure assuming it compiles on all platforms. didn't review the handler/mediums bits https://codereview.chromium.org/419073002/diff/130001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/419073002/diff/130001/components/copresence/B... components/copresence/BUILD.gn:5: static_library("copresence") { isn't this missing the additional deps on content and media? https://codereview.chromium.org/419073002/diff/130001/components/copresence/c... File components/copresence/copresence_constants.cc (right): https://codereview.chromium.org/419073002/diff/130001/components/copresence/c... components/copresence/copresence_constants.cc:9: const int kDefaultRepetitions = 5; does this actually work on windows? https://codereview.chromium.org/419073002/diff/130001/components/copresence/t... File components/copresence/timed_map.h (right): https://codereview.chromium.org/419073002/diff/130001/components/copresence/t... components/copresence/timed_map.h:33: ~TimedMap() {} kinda surprised clang doesn't complain about non-trivial inline dtor?
Dan, I think you should be fine with resuming the review. https://codereview.chromium.org/419073002/diff/130001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/419073002/diff/130001/components/copresence/B... components/copresence/BUILD.gn:5: static_library("copresence") { On 2014/07/31 20:38:48, jochen wrote: > isn't this missing the additional deps on content and media? I am still learning GN :) I'll make sure this is done correctly in the next patch I upload. https://codereview.chromium.org/419073002/diff/130001/components/copresence/c... File components/copresence/copresence_constants.cc (right): https://codereview.chromium.org/419073002/diff/130001/components/copresence/c... components/copresence/copresence_constants.cc:9: const int kDefaultRepetitions = 5; On 2014/07/31 20:38:48, jochen wrote: > does this actually work on windows? Haven't tried it on a windows machine, but will kick off a try bot to check. https://codereview.chromium.org/419073002/diff/130001/components/copresence/t... File components/copresence/timed_map.h (right): https://codereview.chromium.org/419073002/diff/130001/components/copresence/t... components/copresence/timed_map.h:33: ~TimedMap() {} On 2014/07/31 20:38:48, jochen wrote: > kinda surprised clang doesn't complain about non-trivial inline dtor? I followed this pattern from base/containers/mru_cache.h; it seems to be a common pattern used among templated classes, since they need to exist completely in the header. Clang doesn't complain.
https://codereview.chromium.org/419073002/diff/150001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/419073002/diff/150001/components/components_t... components/components_tests.gyp:81: 'copresence/timed_map_unittest.cc', fix alphabetization https://codereview.chromium.org/419073002/diff/150001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence.g... components/copresence.gypi:24: 'copresence/timed_map.h', fix alphabetization https://codereview.chromium.org/419073002/diff/150001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/B... components/copresence/BUILD.gn:9: "timed_map.h", fix alphabetization https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:78: player_->Play(samples); should you be checking player_ and recorder_ for null-ness in these methods, as you do in the d'tor? https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:5: #ifndef COMPONENTS_COPRESENCE_HANDLERS_AUDIO_DIRECTIVE_HANDLER_ update header guard to match path https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:13: #include "base/memory/scoped_ptr.h" i don't think you use this https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:27: class AudioRecorder; you don't need to forward-declare this; you're already including the header https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:44: const AudioDirectiveList::EncodeTokenCallback& encode_cb); add an Init() method here too and set_player_for_testing() and set_recorder_for_testing() so you don't need a protected c'tor? https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:74: #endif // COMPONENTS_COPRESENCE_HANDLERS_AUDIO_DIRECTIVE_HANDLER_ update path https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:114: scoped_ptr<AudioDirective> AudioDirectiveList::GetNextFromList( this doesn't look like it needs to be a member function; just move it to the anon namespace at the top of the file https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:129: return make_scoped_ptr(new AudioDirective(list->top())); where do directives get removed from the list? https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:140: // Copy the samples into its corresponding directive object and move s/its/their/ https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:143: // the samples vector. re TODO: aren't you using a scoped_refptr for the samples vector? https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:41: virtual ~AudioDirective(); why virtual? https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list_unittest.cc:32: // This order is important. We want the message loop to get created before i don't think you need this comment in all of your tests. having the message loop outlive the things that use it seems pretty obvious https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/directive_handler.h:20: namespace copresence { squash this namespace with the previous one https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_player.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player.h:67: void set_output_stream_for_testing( nit: make this public; the for_testing should prevent it from being called from random places https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player.h:75: media::AudioOutputStream* output_stream_for_testing_; why not scoped_ptr? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_player_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player_unittest.cc:76: if (!media::AudioManager::Get()) under which circumstances is this created already? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player_unittest.cc:134: base::MessageLoop message_loop_; create message_loop_ first? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:41: const copresence::AudioRecorder::DecodeSamplesCallback& callback) { nit: nest the anon namespace under the copresence namespace so you don't need the qualifier here https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:121: stream_ = input_stream_for_testing_ ? input_stream_for_testing_ : stream_ = uh, what's up with the "stream_ =" at the end of this line? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:128: stream_->Close(); do you actually need to call Close() here after Open() fails? if so, can this method just call StopAndCloseOnAudioThread()? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:161: if (is_recording_) why do you check is_recording_ here but not in StopOnAudioThread()? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:162: stream_->Stop(); can this method just call StopOnAudioThread() and then close the stream and set it to null? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:189: media::AudioBus::Create(kDefaultChannels, converter_->ChunkSize()); nit: add blank line after this one so this isn't a huge block of text https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:196: // NULL, which will break the conversion loop. i don't understand this comment. is it saying that some other thread will run ProvideInput() and set temp_conversion_buffer_ to NULL while this loop is running? that doesn't seem safe. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:213: int remaining_source_frames = converted_source->frames() - frames_to_copy; is converted_source->frames() guaranteed to be <= total_buffer_frames_? if not, don't you need to check that you're not going to overflow buffer_ here? it seems like it would be wise to use std::min() to ensure that this doesn't happen anyway. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:217: buffer_.get()); it seems like you'll never process the last chunk of samples that's left over from the final call to OnData(). is that a problem? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:101: media::AudioInputStream* input_stream_for_testing_; why not scoped_ptr? https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder_unittest.cc:146: if (received_samples_.size() == expected_samples_size) { shouldn't you assert that this is true? https://codereview.chromium.org/419073002/diff/150001/components/copresence/p... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/p... components/copresence/public/copresence_constants.h:12: // Audio constants. Currently used from the AudioPlayer/AudioRecorder. does this really need to be in the public/ subdirectory? this comment indicates that these are internal constants. https://codereview.chromium.org/419073002/diff/150001/components/copresence/p... components/copresence/public/copresence_constants.h:15: extern const int kDefaultRepetitions; add a blank line between this one and the next one, and similarly between following variables https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... File components/copresence/test/audio_test_support.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... components/copresence/test/audio_test_support.cc:28: PopulateSamples(kFixedRandomSeed, samples, bus->channel(ch)); it seems strange that one of the methods in this file takes a passed-in seed while the others use a hardcoded seed https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... File components/copresence/timed_map_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... components/copresence/timed_map_unittest.cc:22: class TimedMapTest : public testing::Test { please add tests that test the "timed" part of this class. the reason for my suggestion to use TickClock was so that you can have tests that provide their own implementation so that the timing-related part of this class can be tested; hardcoding DefaultTickClock into the implementation doesn't accomplish anything. https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... components/copresence/timed_map_unittest.cc:35: Map map(base::TimeDelta::FromSeconds(0x7331), 3); why are you passing the seconds as hex?
https://codereview.chromium.org/419073002/diff/150001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/419073002/diff/150001/components/components_t... components/components_tests.gyp:81: 'copresence/timed_map_unittest.cc', On 2014/07/31 22:31:14, Daniel Erat wrote: > fix alphabetization Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence.g... components/copresence.gypi:24: 'copresence/timed_map.h', On 2014/07/31 22:31:14, Daniel Erat wrote: > fix alphabetization Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/B... components/copresence/BUILD.gn:9: "timed_map.h", On 2014/07/31 22:31:14, Daniel Erat wrote: > fix alphabetization Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.cc:78: player_->Play(samples); On 2014/07/31 22:31:14, Daniel Erat wrote: > should you be checking player_ and recorder_ for null-ness in these methods, as > you do in the d'tor? Nope. In tests (the only time we have the player_ and recorder_ null), these methods get mocked out hence are never executed. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:5: #ifndef COMPONENTS_COPRESENCE_HANDLERS_AUDIO_DIRECTIVE_HANDLER_ On 2014/07/31 22:31:14, Daniel Erat wrote: > update header guard to match path Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:13: #include "base/memory/scoped_ptr.h" On 2014/07/31 22:31:15, Daniel Erat wrote: > i don't think you use this Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:27: class AudioRecorder; On 2014/07/31 22:31:14, Daniel Erat wrote: > you don't need to forward-declare this; you're already including the header Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:44: const AudioDirectiveList::EncodeTokenCallback& encode_cb); On 2014/07/31 22:31:15, Daniel Erat wrote: > add an Init() method here too and set_player_for_testing() and > set_recorder_for_testing() so you don't need a protected c'tor? Added the init method. Don't need the set methods since we just want to prevent creation of the recorder/player for a test; they don't need to actually be set to anything. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:114: scoped_ptr<AudioDirective> AudioDirectiveList::GetNextFromList( On 2014/07/31 22:31:15, Daniel Erat wrote: > this doesn't look like it needs to be a member function; just move it to the > anon namespace at the top of the file Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:129: return make_scoped_ptr(new AudioDirective(list->top())); On 2014/07/31 22:31:15, Daniel Erat wrote: > where do directives get removed from the list? We don't really remove directives individually. If we keep getting new tokens, the old tokens stay in the list. Once we run out of tokens to play, the list gets cleared, removing all the previous tokens. The only reason we even keep older tokens around is the case when a user 'unsubscribes/unpublishes' a token, a token with a later expiry may be removed, but a token that is still has an unexpired TTL may still be available. So for example, this order of operations happens, User publishes with id P1, token T1, TTL=5m (we're now playing T1) User publishes with id P2, token T2, TTL=10m (we're now playing T2) User publishes with id P3, token T3, TTL=1m (we're still playing T2) User unpublishes with id P2 (we're now playing T1) User unpublishes with id P1 (we're now playing T3) If no unpublishes happen, once T2 expires, we'll clear the entire list, clearing all tokens ending before T2 (which will be T1 and T3). We are currently not removing the tokens from the list but this is a high priority to fix (being tracked at crbug.com/384616). https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:140: // Copy the samples into its corresponding directive object and move On 2014/07/31 22:31:15, Daniel Erat wrote: > s/its/their/ Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.cc:143: // the samples vector. On 2014/07/31 22:31:15, Daniel Erat wrote: > re TODO: aren't you using a scoped_refptr for the samples vector? The TODO is DONE :) Removed. Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:41: virtual ~AudioDirective(); On 2014/07/31 22:31:15, Daniel Erat wrote: > why virtual? No need to, changed. Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list_unittest.cc:32: // This order is important. We want the message loop to get created before On 2014/07/31 22:31:15, Daniel Erat wrote: > i don't think you need this comment in all of your tests. having the message > loop outlive the things that use it seems pretty obvious Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/h... components/copresence/handlers/directive_handler.h:20: namespace copresence { On 2014/07/31 22:31:15, Daniel Erat wrote: > squash this namespace with the previous one Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_player.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player.h:67: void set_output_stream_for_testing( On 2014/07/31 22:31:15, Daniel Erat wrote: > nit: make this public; the for_testing should prevent it from being called from > random places Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player.h:75: media::AudioOutputStream* output_stream_for_testing_; On 2014/07/31 22:31:15, Daniel Erat wrote: > why not scoped_ptr? I wanted to make sure that this got deleted only on the audio thread, but our finalize takes care of that. Made it scoped. Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_player_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player_unittest.cc:76: if (!media::AudioManager::Get()) On 2014/07/31 22:31:15, Daniel Erat wrote: > under which circumstances is this created already? This is a global manager, so once we've created it from one test, it's already created for the rest of the tests. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_player_unittest.cc:134: base::MessageLoop message_loop_; On 2014/07/31 22:31:15, Daniel Erat wrote: > create message_loop_ first? I don't understand. This loop will get created before anything else in the class that will use it (since AudioPlayer is going to only be created when we run tests). https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:41: const copresence::AudioRecorder::DecodeSamplesCallback& callback) { On 2014/07/31 22:31:16, Daniel Erat wrote: > nit: nest the anon namespace under the copresence namespace so you don't need > the qualifier here Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:121: stream_ = input_stream_for_testing_ ? input_stream_for_testing_ : stream_ = On 2014/07/31 22:31:16, Daniel Erat wrote: > uh, what's up with the "stream_ =" at the end of this line? Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:128: stream_->Close(); On 2014/07/31 22:31:16, Daniel Erat wrote: > do you actually need to call Close() here after Open() fails? if so, can this > method just call StopAndCloseOnAudioThread()? If the stream open fails, we should close the stream (at least that seems to be the way the current audio code is recommended. dalecurtis@ reviewed this while we were still closed source and recommended this method). StopAndClose tries to stop the stream, which isn't started yet. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:161: if (is_recording_) On 2014/07/31 22:31:16, Daniel Erat wrote: > why do you check is_recording_ here but not in StopOnAudioThread()? StopOnAudioThread is called when a user explicitly wants to stop a stream, so we make the assumption that the user _has started recording before then. StopAndClose can get called even when the user hasn't started recording (in case he creates the recorder and just destructs it). I added a check in Stop also, just in case the user doesn't invoke the method correctly. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:162: stream_->Stop(); On 2014/07/31 22:31:16, Daniel Erat wrote: > can this method just call StopOnAudioThread() and then close the stream and set > it to null? Sure. Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:189: media::AudioBus::Create(kDefaultChannels, converter_->ChunkSize()); On 2014/07/31 22:31:16, Daniel Erat wrote: > nit: add blank line after this one so this isn't a huge block of text Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:196: // NULL, which will break the conversion loop. On 2014/07/31 22:31:16, Daniel Erat wrote: > i don't understand this comment. is it saying that some other thread will run > ProvideInput() and set temp_conversion_buffer_ to NULL while this loop is > running? that doesn't seem safe. No, this is all single threaded. ->Convert is what will call ProvideInput (on the same thread), which will set temp_conversion_buffer_ to NULL once its done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:213: int remaining_source_frames = converted_source->frames() - frames_to_copy; On 2014/07/31 22:31:16, Daniel Erat wrote: > is converted_source->frames() guaranteed to be <= total_buffer_frames_? if not, > don't you need to check that you're not going to overflow buffer_ here? it seems > like it would be wise to use std::min() to ensure that this doesn't happen > anyway. total_buffer_frames_ is orders of magnitude higher than converted_source->frames() can get to. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.cc:217: buffer_.get()); On 2014/07/31 22:31:16, Daniel Erat wrote: > it seems like you'll never process the last chunk of samples that's left over > from the final call to OnData(). is that a problem? Whatever samples are leftover after the last ProcessSamples call will be picked up by the next OnData call and processed with the next. The only case that won't happen is if the user stops recording, in which case he doesn't care about processing any more samples. Depending on when the user stops recording, he will lose between 0 to 499ms of samples anyway (unless we decide to send out another ProcessSamples on the user stopping recording - there are pros and cons to this, we decided on letting the last half a second go). https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder.h:101: media::AudioInputStream* input_stream_for_testing_; On 2014/07/31 22:31:16, Daniel Erat wrote: > why not scoped_ptr? Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... File components/copresence/mediums/audio/audio_recorder_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/m... components/copresence/mediums/audio/audio_recorder_unittest.cc:146: if (received_samples_.size() == expected_samples_size) { On 2014/07/31 22:31:16, Daniel Erat wrote: > shouldn't you assert that this is true? This is an accumulator method and this is its termination condition. When this is true means that we've gotten enough samples, verify our samples and quit the run loop. https://codereview.chromium.org/419073002/diff/150001/components/copresence/p... File components/copresence/public/copresence_constants.h (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/p... components/copresence/public/copresence_constants.h:12: // Audio constants. Currently used from the AudioPlayer/AudioRecorder. On 2014/07/31 22:31:16, Daniel Erat wrote: > does this really need to be in the public/ subdirectory? this comment indicates > that these are internal constants. They are shared by the whispernet implementation (see https://codereview.chromium.org/438513002/) https://codereview.chromium.org/419073002/diff/150001/components/copresence/p... components/copresence/public/copresence_constants.h:15: extern const int kDefaultRepetitions; On 2014/07/31 22:31:16, Daniel Erat wrote: > add a blank line between this one and the next one, and similarly between > following variables Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... File components/copresence/test/audio_test_support.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... components/copresence/test/audio_test_support.cc:28: PopulateSamples(kFixedRandomSeed, samples, bus->channel(ch)); On 2014/07/31 22:31:16, Daniel Erat wrote: > it seems strange that one of the methods in this file takes a passed-in seed > while the others use a hardcoded seed Changed to allow passed in seeds for all three. Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... File components/copresence/timed_map_unittest.cc (right): https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... components/copresence/timed_map_unittest.cc:22: class TimedMapTest : public testing::Test { On 2014/07/31 22:31:16, Daniel Erat wrote: > please add tests that test the "timed" part of this class. the reason for my > suggestion to use TickClock was so that you can have tests that provide their > own implementation so that the timing-related part of this class can be tested; > hardcoding DefaultTickClock into the implementation doesn't accomplish anything. Done. https://codereview.chromium.org/419073002/diff/150001/components/copresence/t... components/copresence/timed_map_unittest.cc:35: Map map(base::TimeDelta::FromSeconds(0x7331), 3); On 2014/07/31 22:31:16, Daniel Erat wrote: > why are you passing the seconds as hex? Just wanted a random large value, didn't think it would matter what form I provided the value in :) Changed to decimal for easier readability. Done.
sorry, i didn't get to this tonight. i'll try to do it tomorrow morning if the builders aren't busted again.
https://codereview.chromium.org/419073002/diff/210001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/B... components/copresence/BUILD.gn:30: "//content", nit: fix sorting https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:38: // Adds in instruction to our handler. The instruction will execute and be nit: s/in/an/ https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:61: AudioPlayer* player_; nit: add a comment describing ownership of these bare pointers https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:31: AudioDirective(const std::string& op_id, base::Time end_time); the style guide says "Use overloaded functions (including constructors) only if a reader looking at a call site can get a good idea of what is happening without having to first figure out exactly which overload is being called." i don't think that that's the case here; it's confusing how the first string argument passed to this c'tor is op_id while the first string argument passed to the next two c'tors is the token, with the second argument containing op_id. i think i'd be okay with dropping this c'tor and making its callers instead invoke the next c'tor with an empty token, though. https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:114: // Cache that holds 10k encoded samples. After reaching its limit, the cache nit: if there's a chance that the size will change later, don't hardcode it in this comment since it'll probably get out of date https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... File components/copresence/mediums/audio/audio_player.cc (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... components/copresence/mediums/audio/audio_player.cc:108: is_playing_ = true; you have an unprotected write to is_playing_ here (and in a bunch of other audio thread methods), but OnMoreData() reads is_playing_ and is documented as potentially being called from any thread. should is_playing_ be protected by the lock as well? https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... File components/copresence/mediums/audio/audio_player.h (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... components/copresence/mediums/audio/audio_player.h:33: void Play(const scoped_refptr<media::AudioBusRefCounted>& samples); nit: add a blank line after this line
https://codereview.chromium.org/419073002/diff/210001/components/copresence/B... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/B... components/copresence/BUILD.gn:30: "//content", On 2014/08/05 16:38:51, Daniel Erat wrote: > nit: fix sorting Done. https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_handler.h (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:38: // Adds in instruction to our handler. The instruction will execute and be On 2014/08/05 16:38:51, Daniel Erat wrote: > nit: s/in/an/ Done. https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_handler.h:61: AudioPlayer* player_; On 2014/08/05 16:38:51, Daniel Erat wrote: > nit: add a comment describing ownership of these bare pointers Done. https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... File components/copresence/handlers/audio/audio_directive_list.h (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:31: AudioDirective(const std::string& op_id, base::Time end_time); On 2014/08/05 16:38:51, Daniel Erat wrote: > the style guide says "Use overloaded functions (including constructors) only if > a reader looking at a call site can get a good idea of what is happening without > having to first figure out exactly which overload is being called." > > i don't think that that's the case here; it's confusing how the first string > argument passed to this c'tor is op_id while the first string argument passed to > the next two c'tors is the token, with the second argument containing op_id. i > think i'd be okay with dropping this c'tor and making its callers instead invoke > the next c'tor with an empty token, though. Makes sense. Done. https://codereview.chromium.org/419073002/diff/210001/components/copresence/h... components/copresence/handlers/audio/audio_directive_list.h:114: // Cache that holds 10k encoded samples. After reaching its limit, the cache On 2014/08/05 16:38:51, Daniel Erat wrote: > nit: if there's a chance that the size will change later, don't hardcode it in > this comment since it'll probably get out of date Done. https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... File components/copresence/mediums/audio/audio_player.cc (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... components/copresence/mediums/audio/audio_player.cc:108: is_playing_ = true; On 2014/08/05 16:38:52, Daniel Erat wrote: > you have an unprotected write to is_playing_ here (and in a bunch of other audio > thread methods), but OnMoreData() reads is_playing_ and is documented as > potentially being called from any thread. should is_playing_ be protected by the > lock as well? This lock is in place to prevent simultaneous read/write to the actual audio data and the frame pointer into it. OnMoreData doesn't actually need to even check is_playing_ except for the DCHECK. The DCHECK isn't super useful as the audio code is supposed to ensure that OnMoreData isn't called after we stop the stream. Removing the DCHECK removes this concern completely. Done. https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... File components/copresence/mediums/audio/audio_player.h (right): https://codereview.chromium.org/419073002/diff/210001/components/copresence/m... components/copresence/mediums/audio/audio_player.h:33: void Play(const scoped_refptr<media::AudioBusRefCounted>& samples); On 2014/08/05 16:38:52, Daniel Erat wrote: > nit: add a blank line after this line Done.
Uploaded patch for realz this time.
lgtm
The CQ bit was checked by rkc@chromium.org
The CQ bit was unchecked by rkc@chromium.org
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/419073002/230001
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/419073002/250001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/419073002/250001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
On 2014/08/06 16:55:23, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_chromium_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) It looks like you have a real problem with the audio recorder tests on win.
On 2014/08/06 17:32:30, blundell wrote: > On 2014/08/06 16:55:23, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > win_chromium_rel on tryserver.chromium.win > > > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) > > It looks like you have a real problem with the audio recorder tests on win. Spoke to dalecurtis@ about it. This is because the tests are forcing the sample rate on the recorder, which works on other platforms but not on Windows. The solution is to re-write the recorder tests to record at sample rates provided by the OS, which is not trivial. Since this can only happen in test code (in production code, we always use the OS provided audio parameters), I am disabling the test on Windows for now. I'll fix the test to use the OS sample rates in a later iteration.
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/419073002/270001
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/419073002/290001 |
