Ignore uninteresting events when recording the last errors to report

extensions/browser/api/cast_channel/logger.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/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"
 #include "extensions/browser/api/cast_channel/logger_util.h"
 #include "net/base/net_errors.h"
 #include "third_party/zlib/zlib.h"
 
 namespace extensions {
 namespace core_api {
 namespace cast_channel {
@@ skipping 74 matching lines @@
 
   result = deflateEnd(&stream);
   DCHECK(result == Z_OK || result == Z_DATA_ERROR);
 
   if (success)
     *length = out_size - stream.avail_out;
 
   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() {
 }
 
 Logger::AggregatedSocketEventLog::~AggregatedSocketEventLog() {
 }
 
 Logger::Logger(scoped_ptr<base::TickClock> clock,
                base::TimeTicks unix_epoch_time_ticks)
@@ skipping 172 matching lines @@
   }
 
   std::deque<proto::SocketEvent>& socket_events = it->second->socket_events;
   if (socket_events.size() >= kMaxEventsPerSocket) {
     socket_events.pop_front();
     log_.set_num_evicted_socket_events(log_.num_evicted_socket_events() + 1);
   }
 
   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 (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_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();
 
   return it->second->aggregated_socket_event;
 }
 
 scoped_ptr<char[]> Logger::GetLogs(size_t* length) const {
   *length = 0;
 
   Log log;
   // Copy "global" values from |log_|. Don't use |log_| directly since this
   // function is const.
@@ skipping 36 matching lines @@
   AggregatedSocketEventLogMap::const_iterator it =
       aggregated_socket_events_.find(channel_id);
   if (it != aggregated_socket_events_.end()) {
     return it->second->last_errors;
   } else {
     return LastErrors();
   }
 }
 
 }  // namespace cast_channel
-}  // namespace api
+}  // namespace core_api
 }  // namespace extensions