Added ClientEvent proto and structure for storing events in the factory.

google_apis/gcm/engine/connection_event_tracker.cc

Index: google_apis/gcm/engine/connection_event_tracker.cc
diff --git a/google_apis/gcm/engine/connection_event_tracker.cc b/google_apis/gcm/engine/connection_event_tracker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..331de2e45d752daa1cbe34df733636c771c6d3fd
--- /dev/null
+++ b/google_apis/gcm/engine/connection_event_tracker.cc
@@ -0,0 +1,83 @@
+// Copyright 2016 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 "google_apis/gcm/engine/connection_event_tracker.h"
+
+#include "base/time/time.h"
+#include "google_apis/gcm/protocol/mcs.pb.h"

[Peter Beverloo, 2016/12/05 16:29:34]

nit: unused (already included in the header)

[harkness, 2016/12/06 12:53:35]

Done.

+
+namespace {
+
+// The maxiumum number of events which are stored before deleting old ones.
+// This mirrors the behaviour of the GMS Core connection tracking.
+constexpr size_t kMaxClientEvents = 30;
+
+}  // namespace
+
+namespace gcm {
+
+ConnectionEventTracker::ConnectionEventTracker() = default;
+
+ConnectionEventTracker::~ConnectionEventTracker() = default;
+
+void ConnectionEventTracker::StartConnectionAttempt() {
+  // TODO(harkness): Can we dcheck here that there is not an in progress

[Nicolas Zea, 2016/12/05 18:36:15]

I think you should be able to (the connection factory only has one open socket
at a time, via socket_handle_).

[harkness, 2016/12/06 12:53:35]

I'll experiment with that in a follow-up patch, and send you an e-mail with the
comments in connection_factory_impl.cc that make me wonder if it's valid.

+  // connection?
+  current_event_.set_time_connection_started_ms(base::Time::Now().ToJavaTime());
+  current_event_.set_network_type(
+      net::NetworkChangeNotifier::GetConnectionType());

[Peter Beverloo, 2016/12/05 16:29:34]

two thoughts:
  (1) maybe add a static_cast<int> for clarity? it looks like the implicit cast
is working fine, but it may be good to be explicit about it.
  (2) add a comment saying that the values should remain consistent since we
store them on the server. acts as a bit of a safe-guard in case someone comes by
and refactors the enum.

[harkness, 2016/12/06 12:53:35]

Done.

+}
+
+void ConnectionEventTracker::EndConnectionAttempt() {

[Peter Beverloo, 2016/12/05 16:29:34]

nit:
  DCHECK(current_event_.has_type());

(The type should *always* have been set at this point, right?)

[harkness, 2016/12/06 12:53:35]

It should have been set, however, the test code does a bunch of explicit
manipulation that doesn't respect the correct ordering of things. I fixed the
first test case where it was broken once I added the DCHECK, but there were
more. If you think it's worthwhile I can add the DCHECK and see about fixing the
test cases in a follow-up.

+  if (completed_events_.size() == kMaxClientEvents) {
+    // Don't let the completed events grow beyond the max.
+    completed_events_.pop_front();
+    // TODO(harkness): Keep track of deleted events.
+  }
+
+  // Current event is now completed, so add it to our list of completed events.
+  current_event_.set_time_connection_ended_ms(base::Time::Now().ToJavaTime());
+  completed_events_.push_back(current_event_);
+  current_event_.Clear();
+}
+
+void ConnectionEventTracker::ConnectionAttemptSucceeded() {
+  // Record the successful connection so information about it can be sent in the
+  // next login request. If there is a login failure, this will need to be
+  // updated to a failed connection.
+  current_event_.set_type(mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION);
+  current_event_.set_time_connection_established_ms(
+      base::Time::Now().ToJavaTime());
+
+  // A completed connection means that the old client event data has now been
+  // sent to GCM. Delete old data.
+  completed_events_.clear();
+}
+
+void ConnectionEventTracker::ConnectionLoginFailed() {
+  // A login failure would have originally been marked as a successful
+  // connection, so now that it failed, that needs to be updated.
+  DCHECK_EQ(current_event_.type(),
+            mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION);
+
+  current_event_.set_type(mcs_proto::ClientEvent::FAILED_CONNECTION);
+  current_event_.clear_time_connection_established_ms();
+  current_event_.set_error_code(net::ERR_CONNECTION_RESET);
+}
+
+void ConnectionEventTracker::ConnectionAttemptFailed(int error) {
+  DCHECK_NE(error, net::OK);
+
+  current_event_.set_type(mcs_proto::ClientEvent::FAILED_CONNECTION);
+  current_event_.set_error_code(error);
+}
+
+void ConnectionEventTracker::WriteToLoginRequest(
+    mcs_proto::LoginRequest* request) {
+  DCHECK(request);
+  for (const mcs_proto::ClientEvent& event : completed_events_)
+    request->add_client_event()->CopyFrom(event);
+}
+
+}  // namespace gcm