[Remoting Android] Refactor ClientTelemetryLogger

remoting/client/client_telemetry_logger.cc

 // 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 "remoting/client/client_telemetry_logger.h"
 
 #include "base/logging.h"
 #include "base/rand_util.h"
 #include "remoting/base/telemetry_log_writer.h"
 
 namespace {
 
 const char kSessionIdAlphabet[] =
     "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
 const int kSessionIdLength = 20;
-
 const int kMaxSessionIdAgeDays = 1;
 
 }  // namespace
 
 namespace remoting {
 
 struct ClientTelemetryLogger::HostInfo {
   const std::string host_version;
   const ChromotingEvent::Os host_os;
   const std::string host_os_version;
 };
 
-ClientTelemetryLogger::ClientTelemetryLogger(ChromotingEvent::Mode mode)
-    : mode_(mode) {}
+ClientTelemetryLogger::ClientTelemetryLogger(
+    base::WeakPtr<ChromotingEventLogWriter> log_writer,

[Sergey Ulanov, 2017/01/19 00:50:38]

ChromotingJniRuntime is guaranteed to outlive ChromotingJniInstance, so
log_writer will always outlive ClientTelemetryLogger. So do we really need to
use weak pointers here instead of raw pointers?

[Yuwei, 2017/01/19 23:00:00]

Done. You are right. It should be safe enough to use raw pointer.

+    ChromotingEvent::Mode mode)
+    : mode_(mode), log_writer_(log_writer) {}
 
 ClientTelemetryLogger::~ClientTelemetryLogger() {}
 
 void ClientTelemetryLogger::SetHostInfo(const std::string& host_version,
                                         ChromotingEvent::Os host_os,
                                         const std::string& host_os_version) {
   host_info_.reset(new HostInfo{host_version, host_os, host_os_version});
 }
 
-void ClientTelemetryLogger::Start(
-    std::unique_ptr<UrlRequestFactory> request_factory,
-    const std::string& telemetry_base_url) {
-  DCHECK(!log_writer_);
-  DCHECK(thread_checker_.CalledOnValidThread());
-  log_writer_.reset(
-      new TelemetryLogWriter(telemetry_base_url, std::move(request_factory)));
-}
-
 void ClientTelemetryLogger::LogSessionStateChange(
     ChromotingEvent::SessionState state,
     ChromotingEvent::ConnectionError error) {
-  DCHECK(log_writer_) << "Please call Start before posting logs.";
   DCHECK(thread_checker_.CalledOnValidThread());
-  ExpireSessionIdIfOutdated();
-
-  if (session_id_.empty()) {
-    GenerateSessionId();
-  }
+  RefreshSessionIdIfOutdated();
   if (session_start_time_.is_null()) {
     session_start_time_ = base::TimeTicks::Now();
   }
 
   ChromotingEvent event =
       ClientTelemetryLogger::MakeSessionStateChangeEvent(state, error);
   log_writer_->Log(event);

[Sergey Ulanov, 2017/01/19 00:50:38]

Since log_writer_ is a weak pointer now, you need to verify that it still exists
before using. Same everywhere else log_writer_ is used in this class.
Or alternatively make log_writer_ a raw pointer and require that it outlives
this class.

[Yuwei, 2017/01/19 23:00:00]

Just required that log_writer_ outlives the class.

 
   if (ChromotingEvent::IsEndOfSession(state)) {
     session_id_.clear();
     session_start_time_ = base::TimeTicks();
   }
 }
 
 void ClientTelemetryLogger::LogStatistics(
     protocol::PerformanceTracker* perf_tracker) {
-  DCHECK(log_writer_) << "Please call Start before posting logs.";
   DCHECK(thread_checker_.CalledOnValidThread());
-  ExpireSessionIdIfOutdated();
+  RefreshSessionIdIfOutdated();
 
   ChromotingEvent event = MakeStatsEvent(perf_tracker);
   log_writer_->Log(event);
 }
 
-void ClientTelemetryLogger::SetAuthToken(const std::string& token) {
-  DCHECK(log_writer_) << "Please call Start before setting the token.";
-  DCHECK(thread_checker_.CalledOnValidThread());
-  log_writer_->SetAuthToken(token);
-}
-
-void ClientTelemetryLogger::SetAuthClosure(const base::Closure& closure) {
-  DCHECK(log_writer_) << "Please call Start before setting the closure.";
-  DCHECK(thread_checker_.CalledOnValidThread());
-  log_writer_->SetAuthClosure(closure);
-}
-
 void ClientTelemetryLogger::SetSessionIdGenerationTimeForTest(
     base::TimeTicks gen_time) {
   session_id_generation_time_ = gen_time;
 }
 
-void ClientTelemetryLogger::StartForTest(
-    std::unique_ptr<ChromotingEventLogWriter> writer) {
-  DCHECK(!log_writer_);
-  log_writer_ = std::move(writer);
-}
-
 // static
 ChromotingEvent::SessionState ClientTelemetryLogger::TranslateState(
     protocol::ConnectionToHost::State state) {
   switch (state) {
     case protocol::ConnectionToHost::State::INITIALIZING:
       return ChromotingEvent::SessionState::INITIALIZING;
     case protocol::ConnectionToHost::State::CONNECTING:
       return ChromotingEvent::SessionState::CONNECTING;
     case protocol::ConnectionToHost::State::AUTHENTICATED:
       return ChromotingEvent::SessionState::AUTHENTICATED;
@@ skipping 68 matching lines @@
 
 void ClientTelemetryLogger::GenerateSessionId() {
   session_id_.resize(kSessionIdLength);
   for (int i = 0; i < kSessionIdLength; i++) {
     const int alphabet_size = arraysize(kSessionIdAlphabet) - 1;
     session_id_[i] = kSessionIdAlphabet[base::RandGenerator(alphabet_size)];
   }
   session_id_generation_time_ = base::TimeTicks::Now();
 }
 
-void ClientTelemetryLogger::ExpireSessionIdIfOutdated() {
+void ClientTelemetryLogger::RefreshSessionIdIfOutdated() {
   if (session_id_.empty()) {
+    GenerateSessionId();
     return;
   }
 
   base::TimeDelta max_age = base::TimeDelta::FromDays(kMaxSessionIdAgeDays);
   if (base::TimeTicks::Now() - session_id_generation_time_ > max_age) {
     // Log the old session ID.
     ChromotingEvent event = MakeSessionIdOldEvent();
     log_writer_->Log(event);
 
     // Generate a new session ID.
@@ skipping 52 matching lines @@
   return event;
 }
 
 ChromotingEvent ClientTelemetryLogger::MakeSessionIdNewEvent() {
   ChromotingEvent event(ChromotingEvent::Type::SESSION_ID_NEW);
   FillEventContext(&event);
   return event;
 }
 
 }  // namespace remoting