arc: Revamp the ArcBridgeService interface

components/arc/metrics/arc_metrics_service.cc

Index: components/arc/metrics/arc_metrics_service.cc
diff --git a/components/arc/metrics/arc_metrics_service.cc b/components/arc/metrics/arc_metrics_service.cc
index 5ce5aa101ae122fde893bef49e0a3be45daae846..501e0b9b972d6d349041467cb1feb45875e3171f 100644
--- a/components/arc/metrics/arc_metrics_service.cc
+++ b/components/arc/metrics/arc_metrics_service.cc
@@ -4,6 +4,8 @@
 
 #include "components/arc/metrics/arc_metrics_service.h"
 
+#include <string>
+
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
@@ -17,19 +19,21 @@ const char kArcProcessNamePrefix[] = "org.chromium.arc.";
 const char kGmsProcessNamePrefix[] = "com.google.android.gms";
 const char kBootProgressEnableScreen[] = "boot_progress_enable_screen";
 
-} // namespace
+}  // namespace
 
 namespace arc {
 
 ArcMetricsService::ArcMetricsService(ArcBridgeService* bridge_service)
     : ArcService(bridge_service), binding_(this), weak_ptr_factory_(this) {
-  arc_bridge_service()->AddObserver(this);
+  arc_bridge_service()->metrics()->AddObserver(this);
+  arc_bridge_service()->process()->AddObserver(this);
   oom_kills_monitor_.Start();
 }
 
 ArcMetricsService::~ArcMetricsService() {
   DCHECK(CalledOnValidThread());
-  arc_bridge_service()->RemoveObserver(this);
+  arc_bridge_service()->process()->RemoveObserver(this);
+  arc_bridge_service()->metrics()->RemoveObserver(this);
 }
 
 bool ArcMetricsService::CalledOnValidThread() {
@@ -37,7 +41,8 @@ bool ArcMetricsService::CalledOnValidThread() {
   return thread_checker_.CalledOnValidThread();
 }
 
-void ArcMetricsService::OnMetricsInstanceReady() {
+void ArcMetricsService::OnInstanceReady(mojom::MetricsInstance*,
+                                        uint32_t version) {
   VLOG(2) << "Start metrics service.";
   // Retrieve ARC start time from session manager.
   chromeos::SessionManagerClient* session_manager_client =
@@ -47,39 +52,37 @@ void ArcMetricsService::OnMetricsInstanceReady() {
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void ArcMetricsService::OnMetricsInstanceClosed() {
+void ArcMetricsService::OnInstanceClosed(mojom::MetricsInstance*) {
   VLOG(2) << "Close metrics service.";
   DCHECK(CalledOnValidThread());
   if (binding_.is_bound())
     binding_.Unbind();
 }
 
-void ArcMetricsService::OnProcessInstanceReady() {
+void ArcMetricsService::OnInstanceReady(mojom::ProcessInstance*,
+                                        uint32_t version) {
   VLOG(2) << "Start updating process list.";
-  timer_.Start(
-      FROM_HERE,
-      base::TimeDelta::FromMinutes(kRequestProcessListPeriodInMinutes),
-      this,
-      &ArcMetricsService::RequestProcessList);
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromMinutes(kRequestProcessListPeriodInMinutes),
+               this, &ArcMetricsService::RequestProcessList);
 }
 
-void ArcMetricsService::OnProcessInstanceClosed() {
+void ArcMetricsService::OnInstanceClosed(mojom::ProcessInstance*) {
   VLOG(2) << "Stop updating process list.";
   timer_.Stop();
 }
 
 void ArcMetricsService::RequestProcessList() {
   mojom::ProcessInstance* process_instance =
-      arc_bridge_service()->process_instance();
+      arc_bridge_service()->process()->instance();
   if (!process_instance) {
     LOG(ERROR) << "No process instance found before RequestProcessList";
     return;
   }
 
   VLOG(2) << "RequestProcessList";
-  process_instance->RequestProcessList(
-      base::Bind(&ArcMetricsService::ParseProcessList,
-                 weak_ptr_factory_.GetWeakPtr()));
+  process_instance->RequestProcessList(base::Bind(
+      &ArcMetricsService::ParseProcessList, weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcMetricsService::ParseProcessList(
@@ -109,7 +112,8 @@ void ArcMetricsService::ParseProcessList(
 }
 
 void ArcMetricsService::OnArcStartTimeRetrieved(
-    bool success, base::TimeTicks arc_start_time) {
+    bool success,
+    base::TimeTicks arc_start_time) {
   DCHECK(CalledOnValidThread());
   if (!success) {
     LOG(ERROR) << "Failed to retrieve ARC start timeticks.";
@@ -122,7 +126,7 @@ void ArcMetricsService::OnArcStartTimeRetrieved(
   if (!binding_.is_bound()) {

[hidehiko, 2016/07/11 05:24:53]

(This is off topic so I'm ok to address this separately, but...),
Should this need to wait for MetricsInstance's initialization, too?
metrics()->instance() can be nullptr, here?

[Luis Héctor Chávez, 2016/07/11 17:13:34]

OnArcStartTimeRetrieved already waits for MetricInstance since it can only be
called from OnInstanceReady(MetricsInstance). Having said that, it can go away
between that function is called and this one, so just adding a null-check before
this should be ok.

     mojom::MetricsHostPtr host_ptr;
     binding_.Bind(mojo::GetProxy(&host_ptr));
-    arc_bridge_service()->metrics_instance()->Init(std::move(host_ptr));
+    arc_bridge_service()->metrics()->instance()->Init(std::move(host_ptr));
   }
   arc_start_time_ = arc_start_time;
   VLOG(2) << "ARC start @" << arc_start_time_;
@@ -134,26 +138,21 @@ void ArcMetricsService::ReportBootProgress(
   int64_t arc_start_time_in_ms =
       (arc_start_time_ - base::TimeTicks()).InMilliseconds();
   for (const auto& event : events) {
-    VLOG(2) << "Report boot progress event:"
-        << event->event << "@" << event->uptimeMillis;
+    VLOG(2) << "Report boot progress event:" << event->event << "@"
+            << event->uptimeMillis;
     std::string title = "Arc." + event->event.get();
     base::TimeDelta elapsed_time = base::TimeDelta::FromMilliseconds(
         event->uptimeMillis - arc_start_time_in_ms);
     // Note: This leaks memory, which is expected behavior.
-    base::HistogramBase* histogram =
-        base::Histogram::FactoryTimeGet(
-            title,
-            base::TimeDelta::FromMilliseconds(1),
-            base::TimeDelta::FromSeconds(30),
-            50,
-            base::HistogramBase::kUmaTargetedHistogramFlag);
+    base::HistogramBase* histogram = base::Histogram::FactoryTimeGet(
+        title, base::TimeDelta::FromMilliseconds(1),
+        base::TimeDelta::FromSeconds(30), 50,
+        base::HistogramBase::kUmaTargetedHistogramFlag);
     histogram->AddTime(elapsed_time);
     if (event->event.get().compare(kBootProgressEnableScreen) == 0)
-      UMA_HISTOGRAM_CUSTOM_TIMES("Arc.AndroidBootTime",
-                                 elapsed_time,
+      UMA_HISTOGRAM_CUSTOM_TIMES("Arc.AndroidBootTime", elapsed_time,
                                  base::TimeDelta::FromMilliseconds(1),
-                                 base::TimeDelta::FromSeconds(30),
-                                 50);
+                                 base::TimeDelta::FromSeconds(30), 50);
   }
 }