DBus MediaAnalyticsClient and media_perception pb.

chromeos/dbus/fake_media_analytics_client.cc

+// Copyright 2017 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 "chromeos/dbus/fake_media_analytics_client.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/threading/thread_task_runner_handle.h"
+
+namespace chromeos {
+
+FakeMediaAnalyticsClient::FakeMediaAnalyticsClient()
+    : weak_ptr_factory_(this) {}
+
+FakeMediaAnalyticsClient::~FakeMediaAnalyticsClient() {}
+
+// Override these initializations to do nothing.

[tbarzic, 2017/05/09 20:28:40]

nit: no comment needed

[Luke Sorenson, 2017/05/09 22:21:24]

Done.

+void FakeMediaAnalyticsClient::Init(dbus::Bus* bus) {}
+
+void FakeMediaAnalyticsClient::SetMediaPerceptionSignalHandler(
+    const MediaPerceptionSignalHandler& handler) {
+  media_perception_signal_handler_ = handler;
+}
+
+void FakeMediaAnalyticsClient::ClearMediaPerceptionSignalHandler() {
+  media_perception_signal_handler_.Reset();
+}
+
+void FakeMediaAnalyticsClient::GetState(const StateCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&FakeMediaAnalyticsClient::OnState,
+                            weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void FakeMediaAnalyticsClient::SetState(const mri::State& state,
+                                        const StateCallback& callback) {
+  if (state.has_status()) {

[tbarzic, 2017/05/09 20:28:40]

Status should be set here, right?

[Luke Sorenson, 2017/05/09 22:21:25]

Done.

[tbarzic, 2017/05/10 03:26:24]

sorry for not being precise enough...
I think you should DCHECK state.has_status()

[Luke Sorenson, 2017/05/10 16:38:22]

Done.

+    DCHECK(state.status() == mri::State::SUSPENDED ||
+           state.status() == mri::State::RUNNING)
+        << "Trying set state to something other than RUNNING or SUSPENDED.";
+    current_state_ = state;
+  }
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&FakeMediaAnalyticsClient::OnState,
+                            weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void FakeMediaAnalyticsClient::OnState(const StateCallback& callback) {
+  callback.Run(true, current_state_);
+  // Also fire a fake MediaPerception detection signal.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&FakeMediaAnalyticsClient::OnMediaPerception,

[tbarzic, 2017/05/09 20:28:40]

This seems unintuitive.. if you need the fired in tests, I'd add an explicit
method to the fake for it

[Luke Sorenson, 2017/05/09 22:21:24]

Done.

+                            weak_ptr_factory_.GetWeakPtr()));
+}
+
+void FakeMediaAnalyticsClient::GetDiagnostics(
+    const DiagnosticsCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&FakeMediaAnalyticsClient::OnGetDiagnostics,
+                            weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void FakeMediaAnalyticsClient::OnGetDiagnostics(
+    const DiagnosticsCallback& callback) {
+  mri::Diagnostics diagnostics;
+  diagnostics.add_perception_sample()->mutable_frame_perception()->set_frame_id(

[tbarzic, 2017/05/09 20:28:41]

should frames returned here ahve unique ids?

[Luke Sorenson, 2017/05/09 22:21:24]

Only returning one fake PerceptionSample here, so there is only one Frame.
Trusting DiagnosticsProtoToIdl unittest to verify that having multiple
perception samples gets converted correctly.

[tbarzic, 2017/05/10 03:26:24]

OK, though, my point was that this will always return the same frame id.
I'd consider changing thay (e.g to have the frame set to an increasing index).
Also, I'd make the returned frame value a little more representative (or maybe
always return an empty value here).

Ideally, in tests, there would be a way for a test to set the values to be
returned here - that would be less fragile than having them rely on arbitrary
values set here (and more flexible). Note that I'd be fine with tests overriding
this implementations for their means, I'd just like to avoid having a test rely
on values set here by default.

[Luke Sorenson, 2017/05/10 16:38:22]

Done.

+      1);
+  callback.Run(true, diagnostics);
+}
+
+void FakeMediaAnalyticsClient::OnMediaPerception() {
+  if (media_perception_signal_handler_.is_null()) {
+    return;
+  }
+  mri::MediaPerception media_perception;
+  media_perception.set_timestamp(1);
+  media_perception_signal_handler_.Run(media_perception);
+}
+
+}  // namespace chromeos