diff options
author | schenney <schenney@chromium.org> | 2015-04-17 12:43:23 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-04-17 19:43:48 +0000 |
commit | 7497d960f156a5c2381109efbda48a27a45f5996 (patch) | |
tree | 5700114b6f2fbcedd7e6c858ad3075415e9cc36e /components/copresence | |
parent | e7d1e3c20399ff27c609d6f231f8f58f18709665 (diff) | |
download | chromium_src-7497d960f156a5c2381109efbda48a27a45f5996.zip chromium_src-7497d960f156a5c2381109efbda48a27a45f5996.tar.gz chromium_src-7497d960f156a5c2381109efbda48a27a45f5996.tar.bz2 |
Revert of Revert of Readability review for ckehoe (patchset #1 id:1 of https://codereview.chromium.org/1093843003/)
Reason for revert:
Reverting the right revert.
Original issue's description:
> Revert of Readability review for ckehoe (patchset #8 id:160001 of https://codereview.chromium.org/941593002/)
>
> Reason for revert:
> Reverting speculatively due to failures on Mac 10.6 and 10.8 of the BrowserTest.WindowOpenClose test.
>
> See http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/475
>
> [47576:1799:0416/163155:ERROR:bluetooth_adapter_mac.mm(110)] Not implemented reached in virtual bool device::BluetoothAdapterMac::IsDiscoverable() const
>
> Original issue's description:
> > Readability review for ckehoe
> >
> > R=jgm@google.com, rkc@chromium.org
> >
> > Committed: https://chromium.googlesource.com/chromium/src/+/254c7ea0abc8bb7a4694e757404436aeea177ebf
>
> TBR=jgm@google.com,rkc@chromium.org,ckehoe@chromium.org
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
>
> Committed: https://crrev.com/fd46e3f1f668f8e82c0d0078660e75ca9ef810d9
> Cr-Commit-Position: refs/heads/master@{#325675}
TBR=jgm@google.com,rkc@chromium.org,ckehoe@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Review URL: https://codereview.chromium.org/1092133002
Cr-Commit-Position: refs/heads/master@{#325698}
Diffstat (limited to 'components/copresence')
9 files changed, 119 insertions, 115 deletions
diff --git a/components/copresence/copresence_manager_impl.cc b/components/copresence/copresence_manager_impl.cc index 8934ef4..25b8a5c 100644 --- a/components/copresence/copresence_manager_impl.cc +++ b/components/copresence/copresence_manager_impl.cc @@ -83,8 +83,8 @@ CopresenceManagerImpl::CopresenceManagerImpl(CopresenceDelegate* delegate) messages_callback)); rpc_handler_.reset(new RpcHandler(delegate, - state_.get(), directive_handler_.get(), + state_.get(), gcm_handler_.get(), messages_callback)); diff --git a/components/copresence/copresence_manager_impl.h b/components/copresence/copresence_manager_impl.h index bdb7ee1..0fc3280 100644 --- a/components/copresence/copresence_manager_impl.h +++ b/components/copresence/copresence_manager_impl.h @@ -85,12 +85,11 @@ class CopresenceManagerImpl : public CopresenceManager { bool init_failed_; - // This order is required because each object - // makes calls to those listed before it. + // The RpcHandler makes calls to the other objects here, so it must come last. scoped_ptr<CopresenceStateImpl> state_; - scoped_ptr<RpcHandler> rpc_handler_; scoped_ptr<DirectiveHandler> directive_handler_; scoped_ptr<GCMHandler> gcm_handler_; + scoped_ptr<RpcHandler> rpc_handler_; scoped_ptr<base::Timer> poll_timer_; scoped_ptr<base::Timer> audio_check_timer_; diff --git a/components/copresence/handlers/audio/audio_directive_handler_impl.cc b/components/copresence/handlers/audio/audio_directive_handler_impl.cc index d91b4d8..09a745b 100644 --- a/components/copresence/handlers/audio/audio_directive_handler_impl.cc +++ b/components/copresence/handlers/audio/audio_directive_handler_impl.cc @@ -211,12 +211,14 @@ void AudioDirectiveHandlerImpl::ProcessNextInstruction() { // TODO(crbug.com/436584): Instead of this, store the directives // in a single list, and prune them when expired. - std::vector<Directive> directives; - ConvertDirectives(transmits_lists_[AUDIBLE]->directives(), &directives); - ConvertDirectives(transmits_lists_[INAUDIBLE]->directives(), &directives); - ConvertDirectives(receives_lists_[AUDIBLE]->directives(), &directives); - ConvertDirectives(receives_lists_[INAUDIBLE]->directives(), &directives); - update_directives_callback_.Run(directives); + if (!update_directives_callback_.is_null()) { + std::vector<Directive> directives; + ConvertDirectives(transmits_lists_[AUDIBLE]->directives(), &directives); + ConvertDirectives(transmits_lists_[INAUDIBLE]->directives(), &directives); + ConvertDirectives(receives_lists_[AUDIBLE]->directives(), &directives); + ConvertDirectives(receives_lists_[INAUDIBLE]->directives(), &directives); + update_directives_callback_.Run(directives); + } } bool AudioDirectiveHandlerImpl::GetNextInstructionExpiry( diff --git a/components/copresence/handlers/directive_handler_impl.cc b/components/copresence/handlers/directive_handler_impl.cc index 0ee56d81..854a5c9 100644 --- a/components/copresence/handlers/directive_handler_impl.cc +++ b/components/copresence/handlers/directive_handler_impl.cc @@ -21,14 +21,16 @@ namespace copresence { // Public functions. +DirectiveHandlerImpl::DirectiveHandlerImpl() + : DirectiveHandlerImpl(make_scoped_ptr(new AudioDirectiveHandlerImpl( + DirectivesCallback()))) {} + DirectiveHandlerImpl::DirectiveHandlerImpl( const DirectivesCallback& update_directives_callback) - : DirectiveHandlerImpl(update_directives_callback, - make_scoped_ptr(new AudioDirectiveHandlerImpl( + : DirectiveHandlerImpl(make_scoped_ptr(new AudioDirectiveHandlerImpl( update_directives_callback))) {} DirectiveHandlerImpl::DirectiveHandlerImpl( - const DirectivesCallback& update_directives_callback, scoped_ptr<AudioDirectiveHandler> audio_handler) : audio_handler_(audio_handler.Pass()), is_started_(false) {} diff --git a/components/copresence/handlers/directive_handler_impl.h b/components/copresence/handlers/directive_handler_impl.h index 4712e4f..5191e1d 100644 --- a/components/copresence/handlers/directive_handler_impl.h +++ b/components/copresence/handlers/directive_handler_impl.h @@ -20,10 +20,10 @@ namespace copresence { // The directive handler manages transmit and receive directives. class DirectiveHandlerImpl final : public DirectiveHandler { public: + DirectiveHandlerImpl(); explicit DirectiveHandlerImpl( const DirectivesCallback& update_directives_callback); DirectiveHandlerImpl( - const DirectivesCallback& update_directives_callback, scoped_ptr<AudioDirectiveHandler> audio_handler); ~DirectiveHandlerImpl() override; diff --git a/components/copresence/handlers/directive_handler_unittest.cc b/components/copresence/handlers/directive_handler_unittest.cc index 97853cc..a18c775 100644 --- a/components/copresence/handlers/directive_handler_unittest.cc +++ b/components/copresence/handlers/directive_handler_unittest.cc @@ -26,8 +26,6 @@ const int64 kDefaultTtl = 600000; // 10 minutes namespace copresence { -void IgnoreDirectiveUpdates(const std::vector<Directive>& /* directives */) {} - const Directive CreateDirective(const std::string& publish_id, const std::string& subscribe_id, const std::string& token, @@ -105,7 +103,6 @@ class DirectiveHandlerTest : public testing::Test { : whispernet_client_(new audio_modem::StubWhispernetClient), audio_handler_(new FakeAudioDirectiveHandler), directive_handler_( - base::Bind(&IgnoreDirectiveUpdates), make_scoped_ptr<AudioDirectiveHandler>(audio_handler_)) {} protected: diff --git a/components/copresence/rpc/rpc_handler.cc b/components/copresence/rpc/rpc_handler.cc index c989531..22be6e2 100644 --- a/components/copresence/rpc/rpc_handler.cc +++ b/components/copresence/rpc/rpc_handler.cc @@ -1,4 +1,4 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. +// Copyright 2015 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -46,7 +46,7 @@ const char RpcHandler::kReportRequestRpcName[] = "report"; namespace { const int kTokenLoggingSuffix = 5; -const int kInvalidTokenExpiryTimeMs = 10 * 60 * 1000; // 10 minutes. +const int kInvalidTokenExpiryTimeMinutes = 10; const int kMaxInvalidTokens = 10000; const char kRegisterDeviceRpcName[] = "registerdevice"; const char kDefaultCopresenceServer[] = @@ -61,7 +61,6 @@ std::string ToUrlSafe(std::string token) { return token; } - // Logging // Checks for a copresence error. If there is one, logs it and returns true. @@ -104,12 +103,11 @@ const std::string LoggingStrForToken(const std::string& auth_token) { std::string token_suffix = auth_token.substr( auth_token.length() - kTokenLoggingSuffix, kTokenLoggingSuffix); - return base::StringPrintf("token ...%s", token_suffix.c_str()); + return "token ..." + token_suffix; } // Request construction -// TODO(ckehoe): Move these into a separate file? template <typename T> BroadcastScanConfiguration GetBroadcastScanConfig(const T& msg) { @@ -167,19 +165,19 @@ void AddTokenToRequest(const AudioToken& token, ReportRequest* request) { // Public functions. RpcHandler::RpcHandler(CopresenceDelegate* delegate, - CopresenceStateImpl* state, DirectiveHandler* directive_handler, + CopresenceStateImpl* state, GCMHandler* gcm_handler, const MessagesCallback& new_messages_callback, const PostCallback& server_post_callback) : delegate_(delegate), - state_(state), directive_handler_(directive_handler), + state_(state), gcm_handler_(gcm_handler), new_messages_callback_(new_messages_callback), server_post_callback_(server_post_callback), invalid_audio_token_cache_( - base::TimeDelta::FromMilliseconds(kInvalidTokenExpiryTimeMs), + base::TimeDelta::FromMinutes(kInvalidTokenExpiryTimeMinutes), kMaxInvalidTokens) { DCHECK(delegate_); DCHECK(directive_handler_); @@ -197,8 +195,7 @@ RpcHandler::RpcHandler(CopresenceDelegate* delegate, } RpcHandler::~RpcHandler() { - // Do not use |directive_handler_| or |gcm_handler_| here. - // They will already have been destructed. + // TODO(ckehoe): Cancel the GCM callback? for (HttpPost* post : pending_posts_) delete post; } @@ -258,15 +255,16 @@ void RpcHandler::SendReportRequest(scoped_ptr<ReportRequest> request, AddPlayingTokens(request.get()); - SendServerRequest(kReportRequestRpcName, - device_id, - app_id, - authenticated, - request.Pass(), - // On destruction, this request will be cancelled. - base::Bind(&RpcHandler::ReportResponseHandler, - base::Unretained(this), - status_callback)); + request->set_allocated_header(CreateRequestHeader(app_id, device_id)); + server_post_callback_.Run(delegate_->GetRequestContext(), + kReportRequestRpcName, + delegate_->GetAPIKey(app_id), + auth_token, + make_scoped_ptr<MessageLite>(request.release()), + // On destruction, this request will be cancelled. + base::Bind(&RpcHandler::ReportResponseHandler, + base::Unretained(this), + status_callback)); } void RpcHandler::ReportTokens(const std::vector<AudioToken>& tokens) { @@ -297,7 +295,7 @@ RpcHandler::PendingRequest::PendingRequest(scoped_ptr<ReportRequest> report, RpcHandler::PendingRequest::~PendingRequest() {} -void RpcHandler::RegisterDevice(bool authenticated) { +void RpcHandler::RegisterDevice(const bool authenticated) { DVLOG(2) << "Sending " << (authenticated ? "authenticated" : "anonymous") << " registration to server."; @@ -328,22 +326,25 @@ void RpcHandler::RegisterDevice(bool authenticated) { bool gcm_pending = authenticated && gcm_handler_ && gcm_id_.empty(); pending_registrations_.insert(authenticated); - SendServerRequest( - kRegisterDeviceRpcName, - // The device is empty on first registration. - // When re-registering to pass on the GCM ID, it will be present. - delegate_->GetDeviceId(authenticated), - std::string(), // app ID - authenticated, - request.Pass(), - base::Bind(&RpcHandler::RegisterResponseHandler, - // On destruction, this request will be cancelled. - base::Unretained(this), - authenticated, - gcm_pending)); + request->set_allocated_header(CreateRequestHeader( + // The device is empty on first registration. + // When re-registering to pass on the GCM ID, it will be present. + std::string(), delegate_->GetDeviceId(authenticated))); + if (authenticated) + DCHECK(!auth_token_.empty()); + server_post_callback_.Run(delegate_->GetRequestContext(), + kRegisterDeviceRpcName, + std::string(), + authenticated ? auth_token_ : std::string(), + make_scoped_ptr<MessageLite>(request.release()), + // On destruction, this request will be cancelled. + base::Bind(&RpcHandler::RegisterResponseHandler, + base::Unretained(this), + authenticated, + gcm_pending)); } -void RpcHandler::ProcessQueuedRequests(bool authenticated) { +void RpcHandler::ProcessQueuedRequests(const bool authenticated) { // Track requests that are not in this auth state. ScopedVector<PendingRequest> still_pending_requests; @@ -432,13 +433,13 @@ void RpcHandler::RegisterResponseHandler( int http_status_code, const std::string& response_data) { if (completed_post) { - int elements_erased = pending_posts_.erase(completed_post); - DCHECK_GT(elements_erased, 0); + size_t elements_erased = pending_posts_.erase(completed_post); + DCHECK_GT(elements_erased, 0u); delete completed_post; } - int registrations_completed = pending_registrations_.erase(authenticated); - DCHECK_GT(registrations_completed, 0); + size_t registrations_completed = pending_registrations_.erase(authenticated); + DCHECK_GT(registrations_completed, 0u); RegisterDeviceResponse response; const std::string token_str = @@ -469,8 +470,8 @@ void RpcHandler::ReportResponseHandler(const StatusCallback& status_callback, int http_status_code, const std::string& response_data) { if (completed_post) { - int elements_erased = pending_posts_.erase(completed_post); - DCHECK(elements_erased); + size_t elements_erased = pending_posts_.erase(completed_post); + DCHECK_GT(elements_erased, 0u); delete completed_post; } @@ -514,7 +515,8 @@ void RpcHandler::ReportResponseHandler(const StatusCallback& status_callback, directive_handler_->AddDirective(directive); for (const Token& token : update_response.token()) { - state_->UpdateTokenStatus(token.id(), token.status()); + if (state_) + state_->UpdateTokenStatus(token.id(), token.status()); switch (token.status()) { case VALID: // TODO(rkc/ckehoe): Store the token in a |valid_token_cache_| with a @@ -591,25 +593,6 @@ RequestHeader* RpcHandler::CreateRequestHeader( return header; } -template <class T> -void RpcHandler::SendServerRequest( - const std::string& rpc_name, - const std::string& device_id, - const std::string& app_id, - bool authenticated, - scoped_ptr<T> request, - const PostCleanupCallback& response_handler) { - request->set_allocated_header(CreateRequestHeader(app_id, device_id)); - if (authenticated) - DCHECK(!auth_token_.empty()); - server_post_callback_.Run(delegate_->GetRequestContext(), - rpc_name, - delegate_->GetAPIKey(app_id), - authenticated ? auth_token_ : std::string(), - make_scoped_ptr<MessageLite>(request.release()), - response_handler); -} - void RpcHandler::SendHttpPost(net::URLRequestContextGetter* url_context_getter, const std::string& rpc_name, const std::string& api_key, diff --git a/components/copresence/rpc/rpc_handler.h b/components/copresence/rpc/rpc_handler.h index 2e6f883..3731d96 100644 --- a/components/copresence/rpc/rpc_handler.h +++ b/components/copresence/rpc/rpc_handler.h @@ -1,4 +1,4 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. +// Copyright 2015 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -13,15 +13,12 @@ #include "base/callback_forward.h" #include "base/memory/scoped_ptr.h" #include "base/memory/scoped_vector.h" +#include "components/audio_modem/public/audio_modem_types.h" #include "components/copresence/proto/enums.pb.h" #include "components/copresence/public/copresence_constants.h" #include "components/copresence/public/copresence_delegate.h" #include "components/copresence/timed_map.h" -namespace audio_modem { -struct AudioToken; -} - namespace copresence { class CopresenceDelegate; @@ -33,12 +30,46 @@ class ReportRequest; class RequestHeader; class SubscribedMessage; -// This class currently handles all communication with the copresence server. -class RpcHandler { +// This class handles all communication with the copresence server. +// Clients provide a ReportRequest proto containing publishes, subscribes, +// and token observations they want to send to the server. The RpcHandler +// will fill in details like the RequestHeader and DeviceCapabilities, +// and dispatch the results of the server call to the appropriate parts +// of the system. +// +// To create an RpcHandler, clients will need to provide a few other classes +// that support its functionality. Notable among them is the CopresenceDelegate, +// an interface clients must implement to provide settings and functionality +// that may depend on the environment. See the definition in +// //components/copresence/public/copresence_delegate.h. +// +// Here is an example of creating and using an RpcHandler. +// The GCMHandler and CopresenceStateImpl are optional. +// +// MyDelegate delegate(...); +// copresence::DirectiveHandlerImpl directive_handler; +// +// RpcHandler handler(&delegate, +// &directive_handler, +// nullptr, +// nullptr, +// base::Bind(&HandleMessages)); +// +// scoped_ptr<ReportRequest> request(new ReportRequest); +// (Fill in ReportRequest.) +// +// handler.SendReportRequest(request.Pass(), +// "my_app_id", +// "", +// base::Bind(&HandleStatus)); +// +// The server will respond with directives, which get passed to the +// DirectiveHandlerImpl. +// +// Tokens from the audio modem should also be forwarded +// via ReportTokens() so that messages get delivered properly. +class RpcHandler final { public: - // A callback to indicate whether handler initialization succeeded. - using SuccessCallback = base::Callback<void(bool)>; - // An HttpPost::ResponseCallback along with an HttpPost object to be deleted. // Arguments: // HttpPost*: The handler should take ownership of (i.e. delete) this object. @@ -68,25 +99,29 @@ class RpcHandler { // Report rpc name to send to Apiary. static const char kReportRequestRpcName[]; - // Constructor. |delegate| and |directive_handler| - // are owned by the caller and must outlive the RpcHandler. - // |server_post_callback| should be set only by tests. + // Constructor. The CopresenceStateImpl and GCMHandler may be null. + // The first four parameters are owned by the caller and (if not null) + // must outlive the RpcHandler. RpcHandler(CopresenceDelegate* delegate, - CopresenceStateImpl* state, DirectiveHandler* directive_handler, + CopresenceStateImpl* state, GCMHandler* gcm_handler, const MessagesCallback& new_messages_callback, const PostCallback& server_post_callback = PostCallback()); - virtual ~RpcHandler(); + // Not copyable. + RpcHandler(const RpcHandler&) = delete; + void operator=(const RpcHandler&) = delete; + + ~RpcHandler(); - // Send a ReportRequest from a specific app, and get notified of completion. + // Sends a ReportRequest from a specific app, and get notified of completion. void SendReportRequest(scoped_ptr<ReportRequest> request, const std::string& app_id, const std::string& auth_token, const StatusCallback& callback); - // Report a set of tokens to the server for a given medium. + // Reports a set of tokens to the server for a given medium. // Uses all active auth tokens (if any). void ReportTokens(const std::vector<audio_modem::AudioToken>& tokens); @@ -114,10 +149,10 @@ class RpcHandler { // Device registration has completed. Send the requests that it was blocking. void ProcessQueuedRequests(bool authenticated); - // Send a ReportRequest from Chrome itself, i.e. no app id. + // Sends a ReportRequest from Chrome itself, i.e. no app id. void ReportOnAllDevices(scoped_ptr<ReportRequest> request); - // Store a GCM ID and send it to the server if needed. + // Stores a GCM ID and send it to the server if needed. void RegisterGcmId(const std::string& gcm_id); // Server call response handlers. @@ -131,11 +166,10 @@ class RpcHandler { int http_status_code, const std::string& response_data); - // If the request has any unpublish or unsubscribe operations, it removes - // them from our directive handlers. + // Removes unpublished or unsubscribed operations from the directive handlers. void ProcessRemovedOperations(const ReportRequest& request); - // Add all currently playing tokens to the update signals in this report + // Adds all currently playing tokens to the update signals in this report // request. This ensures that the server doesn't keep issueing new tokens to // us when we're already playing valid tokens. void AddPlayingTokens(ReportRequest* request); @@ -147,15 +181,6 @@ class RpcHandler { RequestHeader* CreateRequestHeader(const std::string& app_id, const std::string& device_id) const; - // Post a request to the server. The request should be in proto format. - template <class T> - void SendServerRequest(const std::string& rpc_name, - const std::string& device_id, - const std::string& app_id, - bool authenticated, - scoped_ptr<T> request, - const PostCleanupCallback& response_handler); - // Wrapper for the http post constructor. This is the default way // to contact the server, but it can be overridden for testing. void SendHttpPost(net::URLRequestContextGetter* url_context_getter, @@ -167,8 +192,8 @@ class RpcHandler { // These belong to the caller. CopresenceDelegate* const delegate_; - CopresenceStateImpl* state_; DirectiveHandler* const directive_handler_; + CopresenceStateImpl* state_; GCMHandler* const gcm_handler_; MessagesCallback new_messages_callback_; @@ -180,8 +205,6 @@ class RpcHandler { std::set<bool> pending_registrations_; std::string auth_token_; std::string gcm_id_; - - DISALLOW_COPY_AND_ASSIGN(RpcHandler); }; } // namespace copresence diff --git a/components/copresence/rpc/rpc_handler_unittest.cc b/components/copresence/rpc/rpc_handler_unittest.cc index e4ff730..3737eaf 100644 --- a/components/copresence/rpc/rpc_handler_unittest.cc +++ b/components/copresence/rpc/rpc_handler_unittest.cc @@ -1,4 +1,4 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. +// Copyright 2015 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -50,11 +50,10 @@ class RpcHandlerTest : public testing::Test, public CopresenceDelegate { : whispernet_client_(new audio_modem::StubWhispernetClient), // TODO(ckehoe): Use a FakeCopresenceState here // and test that it gets called correctly. - state_(new CopresenceStateImpl), rpc_handler_(this, - state_.get(), &directive_handler_, nullptr, + nullptr, base::Bind(&IgnoreMessages), base::Bind(&RpcHandlerTest::CaptureHttpPost, base::Unretained(this))), @@ -164,7 +163,6 @@ class RpcHandlerTest : public testing::Test, public CopresenceDelegate { scoped_ptr<WhispernetClient> whispernet_client_; FakeDirectiveHandler directive_handler_; - scoped_ptr<CopresenceStateImpl> state_; RpcHandler rpc_handler_; std::map<bool, std::string> device_id_by_auth_state_; |