cast channel error cleanup

extensions/browser/api/cast_channel/cast_channel_api.cc

 // Copyright 2014 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.
 
 #include "extensions/browser/api/cast_channel/cast_channel_api.h"
 
 #include <limits>
 #include <string>
 
 #include "base/json/json_writer.h"
@@ skipping 155 matching lines @@
   VLOG(1) << "Sending message " << ParamToString(message_info)
           << " to channel " << ParamToString(channel_info);
   scoped_ptr<Event> event(new Event(OnMessage::kEventName, results.Pass()));
   extensions::EventRouter::Get(browser_context_)
       ->DispatchEventToExtension(socket->owner_extension_id(), event.Pass());
 }
 
 CastChannelAPI::~CastChannelAPI() {}
 
 CastChannelAsyncApiFunction::CastChannelAsyncApiFunction()
-  : manager_(NULL), error_(cast_channel::CHANNEL_ERROR_NONE) { }
+    : manager_(NULL), channel_error_(cast_channel::CHANNEL_ERROR_NONE) {
+}
 
 CastChannelAsyncApiFunction::~CastChannelAsyncApiFunction() { }
 
 bool CastChannelAsyncApiFunction::PrePrepare() {
   manager_ = ApiResourceManager<CastSocket>::Get(browser_context());
   return true;
 }
 
 bool CastChannelAsyncApiFunction::Respond() {
-  return error_ == cast_channel::CHANNEL_ERROR_NONE;
+  return channel_error_ == cast_channel::CHANNEL_ERROR_NONE;

[mark a. foltz, 2014/10/31 22:52:35]

It looks like we only use channel_error_ to indicate whether the function was
successful or not, and don't use the value (after putting it in the ChannelInfo
result).

IMO it would be cleaner to replace it with an error_ boolean that gets set by
SetResultFromError().

Maybe this then becomes:
return error_ || !GetError().empty()

[vadimgo, 2014/10/31 23:32:39]

Yes, it is cleaner with a bool. The logic became

  return !is_channel_error_ && GetError().empty();

 }
 
 CastSocket* CastChannelAsyncApiFunction::GetSocketOrCompleteWithError(
     int channel_id) {
   CastSocket* socket = GetSocket(channel_id);
   if (!socket) {
     SetResultFromError(channel_id,
                        cast_channel::CHANNEL_ERROR_INVALID_CHANNEL_ID);
     AsyncWorkCompleted();
   }
@@ skipping 12 matching lines @@
 void CastChannelAsyncApiFunction::RemoveSocket(int channel_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(manager_);
   manager_->Remove(extension_->id(), channel_id);
 }
 
 void CastChannelAsyncApiFunction::SetResultFromSocket(
     const CastSocket& socket) {
   ChannelInfo channel_info;
   FillChannelInfo(socket, &channel_info);
-  error_ = socket.error_state();
+  channel_error_ = socket.error_state();
   SetResultFromChannelInfo(channel_info);
 }
 
-void CastChannelAsyncApiFunction::SetResultFromError(int channel_id,
-                                                     ChannelError error) {
+void CastChannelAsyncApiFunction::SetResultFromError(
+    int channel_id,
+    ChannelError channel_error) {
   ChannelInfo channel_info;
   channel_info.channel_id = channel_id;
   channel_info.url = "";
   channel_info.ready_state = cast_channel::READY_STATE_CLOSED;
-  channel_info.error_state = error;
+  channel_info.error_state = channel_error;
   channel_info.connect_info.ip_address = "";
   channel_info.connect_info.port = 0;
   channel_info.connect_info.auth = cast_channel::CHANNEL_AUTH_TYPE_SSL;
   SetResultFromChannelInfo(channel_info);
-  error_ = error;
+  channel_error_ = channel_error;
+}
+
+void CastChannelAsyncApiFunction::SetChannelError(const std::string& error,

[mark a. foltz, 2014/10/31 22:52:35]

With the change I mentioned this may no longer be necessary.

[vadimgo, 2014/10/31 23:32:39]

Done.

+                                                  ChannelError channel_error) {
+  SetError(error);
+  channel_error_ = channel_error;
 }
 
 CastSocket* CastChannelAsyncApiFunction::GetSocket(int channel_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(manager_);
   return manager_->Get(extension_->id(), channel_id);
 }
 
 void CastChannelAsyncApiFunction::SetResultFromChannelInfo(
     const ChannelInfo& channel_info) {
@@ skipping 69 matching lines @@
   EXTENSION_FUNCTION_VALIDATE(params_.get());
   // The connect_info parameter may be a string URL like cast:// or casts:// or
   // a ConnectInfo object.
   std::string cast_url;
   switch (params_->connect_info->GetType()) {
     case base::Value::TYPE_STRING:
       CHECK(params_->connect_info->GetAsString(&cast_url));
       connect_info_.reset(new ConnectInfo);
       if (!ParseChannelUrl(GURL(cast_url), connect_info_.get())) {
         connect_info_.reset();
-        SetError("Invalid connect_info (invalid Cast URL " + cast_url + ")");
+        SetChannelError("Invalid connect_info (invalid Cast URL " + cast_url +
+                        ")");
       }
       break;
     case base::Value::TYPE_DICTIONARY:
       connect_info_ = ConnectInfo::FromValue(*(params_->connect_info));
       if (!connect_info_.get()) {
-        SetError("connect_info.auth is required");
+        SetChannelError("connect_info.auth is required");
       }
       break;
     default:
-      SetError("Invalid connect_info (unknown type)");
+      SetChannelError("Invalid connect_info (unknown type)");
       break;
   }
   if (!connect_info_.get()) {
     return false;
   }
   if (!IsValidConnectInfoPort(*connect_info_)) {
-    SetError("Invalid connect_info (invalid port)");
+    SetChannelError("Invalid connect_info (invalid port)");
   } else if (!IsValidConnectInfoAuth(*connect_info_)) {
-    SetError("Invalid connect_info (invalid auth)");
+    SetChannelError("Invalid connect_info (invalid auth)");
   } else if (!IsValidConnectInfoIpAddress(*connect_info_)) {
-    SetError("Invalid connect_info (invalid IP address)");
+    SetChannelError("Invalid connect_info (invalid IP address)");
   }
   if (!GetError().empty()) {
     return false;
   }
   channel_auth_ = connect_info_->auth;
   ip_endpoint_.reset(ParseConnectInfo(*connect_info_));
   return true;
 }
 
 void CastChannelOpenFunction::AsyncWorkStart() {
@@ skipping 32 matching lines @@
 }
 
 CastChannelSendFunction::CastChannelSendFunction() { }
 
 CastChannelSendFunction::~CastChannelSendFunction() { }
 
 bool CastChannelSendFunction::Prepare() {
   params_ = Send::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params_.get());
   if (params_->message.namespace_.empty()) {
-    SetError("message_info.namespace_ is required");
+    SetChannelError("message_info.namespace_ is required");
     return false;
   }
   if (params_->message.source_id.empty()) {
-    SetError("message_info.source_id is required");
+    SetChannelError("message_info.source_id is required");
     return false;
   }
   if (params_->message.destination_id.empty()) {
-    SetError("message_info.destination_id is required");
+    SetChannelError("message_info.destination_id is required");
     return false;
   }
   switch (params_->message.data->GetType()) {
     case base::Value::TYPE_STRING:
     case base::Value::TYPE_BINARY:
       break;
     default:
-      SetError("Invalid type of message_info.data");
+      SetChannelError("Invalid type of message_info.data");
       return false;
   }
   return true;
 }
 
 void CastChannelSendFunction::AsyncWorkStart() {
   CastSocket* socket = GetSocket(params_->channel.channel_id);
   if (!socket) {
     SetResultFromError(params_->channel.channel_id,
                        cast_channel::CHANNEL_ERROR_INVALID_CHANNEL_ID);
@@ skipping 71 matching lines @@
 }
 
 void CastChannelGetLogsFunction::AsyncWorkStart() {
   DCHECK(api_);
 
   size_t length = 0;
   scoped_ptr<char[]> out = api_->GetLogger()->GetLogs(&length);
   if (out.get()) {
     SetResult(new base::BinaryValue(out.Pass(), length));
   } else {
-    SetError("Unable to get logs.");
+    SetChannelError("Unable to get logs.");
   }
 
   api_->GetLogger()->Reset();
 
   AsyncWorkCompleted();
 }
 
 CastChannelSetAuthorityKeysFunction::CastChannelSetAuthorityKeysFunction() {
 }
 
 CastChannelSetAuthorityKeysFunction::~CastChannelSetAuthorityKeysFunction() {
 }
 
 bool CastChannelSetAuthorityKeysFunction::Prepare() {
   params_ = cast_channel::SetAuthorityKeys::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params_.get());
   return true;
 }
 
 void CastChannelSetAuthorityKeysFunction::AsyncWorkStart() {
   std::string& keys = params_->keys;
   std::string& signature = params_->signature;
   if (signature.empty() || keys.empty() ||
       !cast_channel::SetTrustedCertificateAuthorities(keys, signature)) {
-    SetError("Unable to set authority keys.");
+    SetChannelError("Unable to set authority keys.");
   }
 
   AsyncWorkCompleted();
 }
 
 }  // namespace extensions