Readability review for ckehoe

R=jgm@google.com, rkc@chromium.org

Review URL: https://codereview.chromium.org/941593002

Cr-Commit-Position: refs/heads/master@{#325504}

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));
 

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_;

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(

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) {}

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;
 

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:

components/copresence/rpc/rpc_handler.cc

-// 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,

components/copresence/rpc/rpc_handler.h

-// 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

components/copresence/rpc/rpc_handler_unittest.cc

-// 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_;