Ignore uninteresting events when recording the last errors to report

extensions/browser/api/cast_channel/logger.cc

Index: extensions/browser/api/cast_channel/logger.cc
diff --git a/extensions/browser/api/cast_channel/logger.cc b/extensions/browser/api/cast_channel/logger.cc
index 823b4c320877e84c3141879d8ce81d92e9bb8dd9..5ff850975f7b1beb914b44eb43ee21e829820b46 100644
--- a/extensions/browser/api/cast_channel/logger.cc
+++ b/extensions/browser/api/cast_channel/logger.cc
@@ -4,6 +4,8 @@
 
 #include "extensions/browser/api/cast_channel/logger.h"
 
+#include <string>
+
 #include "base/strings/string_util.h"
 #include "base/time/tick_clock.h"
 #include "extensions/browser/api/cast_channel/cast_auth_util.h"
@@ -98,6 +100,13 @@ scoped_ptr<char[]> Compress(const std::string& input, size_t* length) {
   return out.Pass();
 }
 
+// Returns true if |event| has any error fields set.
+bool IsErrorEvent(const SocketEvent& event) {
+  return (event.has_net_return_value() &&
+          event.net_return_value() < net::ERR_IO_PENDING) ||
+         event.has_challenge_reply_error_type() || event.has_nss_error_code();
+}
+
 }  // namespace
 
 Logger::AggregatedSocketEventLog::AggregatedSocketEventLog() {
@@ -290,16 +299,19 @@ AggregatedSocketEvent& Logger::LogSocketEvent(int channel_id,
 
   socket_events.push_back(socket_event);
 
-  it->second->last_errors.event_type = socket_event.type();
-  if (socket_event.has_net_return_value()) {
-    it->second->last_errors.net_return_value = socket_event.net_return_value();
-  }
-  if (socket_event.has_challenge_reply_error_type()) {
-    it->second->last_errors.challenge_reply_error_type =
-        socket_event.challenge_reply_error_type();
+  if (IsErrorEvent(socket_event)) {

[miu, 2014/10/11 02:16:44]

This is up to you, but IMO it hurts readability to have the logic in
IsErrorEvent() defined so far away from where it is relevant in the code.  My
suggestion:

auto& last_errors = it->second->last_errors;
if (socket_event.has_net_return_value() &&
    socket_event.net_return_value() < net::ERR_IO_PENDING)
  last_errors.net_return_value = socket_event.net_return_value();
if (socket_event.has_challenge_reply_error_type())
  last_errors.challenge_reply_error_type =
socket_event.challenge_reply_error_type();	
if (socket_event.has_nss_error_code())
  last_errors.nss_error_code = socket_event.nss_error_code();
if (last_errors.net_return_value != net::OK ||
    last_errors.challenge_reply_error_type != proto::CHALLENGE_REPLY_ERROR_NONE
||
    last_errors.nss_error_code != 0)
  last_errors.event_type = socket_event.type();

[mark a. foltz, 2014/10/13 20:32:56]

Not a fan of making the existing code block longer; instead factored out your
suggestion into its own function.

+    it->second->last_errors.event_type = socket_event.type();
+    if (socket_event.has_net_return_value()) {
+      it->second->last_errors.net_return_value =
+          socket_event.net_return_value();
+    }
+    if (socket_event.has_challenge_reply_error_type()) {
+      it->second->last_errors.challenge_reply_error_type =
+          socket_event.challenge_reply_error_type();
+    }
+    if (socket_event.has_nss_error_code())
+      it->second->last_errors.nss_error_code = socket_event.nss_error_code();
   }
-  if (socket_event.has_nss_error_code())
-    it->second->last_errors.nss_error_code = socket_event.nss_error_code();
 
   return it->second->aggregated_socket_event;
 }
@@ -356,5 +368,5 @@ LastErrors Logger::GetLastErrors(int channel_id) const {
 }
 
 }  // namespace cast_channel
-}  // namespace api
+}  // namespace core_api
 }  // namespace extensions