MediaPerceptionPrivate API impl and testing.

extensions/browser/api/media_perception_private/media_perception_api_manager.cc

Index: extensions/browser/api/media_perception_private/media_perception_api_manager.cc
diff --git a/extensions/browser/api/media_perception_private/media_perception_api_manager.cc b/extensions/browser/api/media_perception_private/media_perception_api_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..32b8b5d6f44271093f01a5f582d58763e8c23e84
--- /dev/null
+++ b/extensions/browser/api/media_perception_private/media_perception_api_manager.cc
@@ -0,0 +1,170 @@
+// 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 "extensions/browser/api/media_perception_private/media_perception_api_manager.h"
+
+#include "base/lazy_instance.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/media_analytics_client.h"
+#include "chromeos/dbus/upstart_client.h"
+#include "extensions/browser/api/media_perception_private/conversion_utils.h"
+#include "extensions/browser/event_router.h"
+#include "extensions/browser/extension_function.h"
+
+namespace media_perception = extensions::api::media_perception_private;
+
+namespace extensions {
+
+// static
+MediaPerceptionAPIManager* MediaPerceptionAPIManager::Get(
+    content::BrowserContext* context) {
+  return GetFactoryInstance()->Get(context);
+}
+
+static base::LazyInstance<
+    BrowserContextKeyedAPIFactory<MediaPerceptionAPIManager>>::Leaky g_factory =
+    LAZY_INSTANCE_INITIALIZER;
+
+// static
+BrowserContextKeyedAPIFactory<MediaPerceptionAPIManager>*
+MediaPerceptionAPIManager::GetFactoryInstance() {
+  return g_factory.Pointer();
+}
+
+MediaPerceptionAPIManager::MediaPerceptionAPIManager(
+    content::BrowserContext* context)
+    : browser_context_(context),
+      analytics_process_running_(false),
+      weak_ptr_factory_(this) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->SetMediaPerceptionSignalHandler(
+      base::Bind(&MediaPerceptionAPIManager::MediaPerceptionSignalHandler,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+MediaPerceptionAPIManager::~MediaPerceptionAPIManager() {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->ClearMediaPerceptionSignalHandler();
+  // Stop the separate media analytics process.
+  chromeos::UpstartClient* upstart_client =
+      chromeos::DBusThreadManager::Get()->GetUpstartClient();
+  upstart_client->StopMediaAnalytics();
+}
+
+void MediaPerceptionAPIManager::GetState(const APIStateCallback& callback) {
+  // Return uninitialized state if the media analytics process isn't running.
+  if (!analytics_process_running_) {
+    media_perception::State state_unitialized;

[tbarzic, 2017/05/11 00:38:27]

can you add a test for this?

[Luke Sorenson, 2017/05/11 23:57:59]

Done.

+    state_unitialized.status = media_perception::STATUS_UNINITIALIZED;
+    callback.Run(true, std::move(state_unitialized));
+    return;
+  }
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->GetState(base::Bind(&MediaPerceptionAPIManager::StateCallback,
+                                   weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void MediaPerceptionAPIManager::SetState(const media_perception::State& state,
+                                         const APIStateCallback& callback) {
+  mri::State state_proto = StateIdlToProto(state);
+  DCHECK(state_proto.status() == mri::State::RUNNING ||
+         state_proto.status() == mri::State::SUSPENDED)
+      << "Cannot set state to something other than RUNNING or SUSPENDED.";
+  if (!analytics_process_running_) {

[tbarzic, 2017/05/11 00:38:27]

if (analytics_process_running_) {
  SetStateInternal();
  return;
}

if (state_proto.status() == mri::State::RUNNIN) {
  // request start process
  return;
}

// error callback

[Luke Sorenson, 2017/05/11 23:57:59]

Done.

+    if (state_proto.status() == mri::State::RUNNING) {
+      chromeos::UpstartClient* dbus_client =
+          chromeos::DBusThreadManager::Get()->GetUpstartClient();
+      dbus_client->StartMediaAnalytics(

[tbarzic, 2017/05/11 00:38:27]

what if SetState is called before this returns?
e.g.

SetState(RUNNING)
  call UpstartClient
SetState(RUNNING)

we should probably not make a duplicate StartMediaAnalytics call here

[Luke Sorenson, 2017/05/11 23:57:59]

As the client of the API, we can be sure to wait for the callback before calling
SetState(RUNNING) a second time. However, I do see the value in hardening the
API. What do you recommend for ensuring we dont make a duplicate call. A mutex
locked member variable in MediaPerceptionAPIManager?

[tbarzic, 2017/05/12 01:00:21]

nope, let's avoid mutex variables.

you could change analytics_process_running_ to an enum state:
IDLE -> STARTING -> STARTED

when you get a request when this state is STARTING either raise an error,
or set the target state to the new one, and cache the callback for the state.

e.g.
if (upstart_state_ == IDLE) {
  if (state != RUNNING) {
    // respond unitialized
    return;
  }
  state_after_upstart_ = state;
  upstart_callback_ = {callback};
  // invoke start up media perception
} else if (upstart_state_ == STARTING) {
  state_after_upstart = state;
  upstart_callbacks_.push_back(callback);
} else {
  SetStateInternal(callback, state_proto);
}


void UpstartCallback(bool success) {
  if (success) {
    SetState( // callback that will run all callback in upstart_callbacks_)
    return;
  }
  else 
   // run all callbacks in upstart_callbacks_ with an error state
}

[Luke Sorenson, 2017/05/12 17:07:30]

Done.

+          base::Bind(&MediaPerceptionAPIManager::UpstartCallback,
+                     weak_ptr_factory_.GetWeakPtr(), callback, state_proto));
+    } else {
+      media_perception::State state;
+      state.status = media_perception::STATUS_UNINITIALIZED;
+      callback.Run(false, std::move(state));
+    }
+  } else {
+    SetStateInternal(callback, state_proto);
+  }
+}
+
+void MediaPerceptionAPIManager::SetStateInternal(
+    const APIStateCallback& callback,
+    const mri::State& state) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->SetState(state,
+                        base::Bind(&MediaPerceptionAPIManager::StateCallback,
+                                   weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void MediaPerceptionAPIManager::GetDiagnostics(
+    const APIGetDiagnosticsCallback& callback) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->GetDiagnostics(
+      base::Bind(&MediaPerceptionAPIManager::GetDiagnosticsCallback,
+                 weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void MediaPerceptionAPIManager::UpstartCallback(
+    const APIStateCallback& callback,
+    const mri::State& state,
+    bool succeeded) {
+  if (!succeeded) {
+    analytics_process_running_ = false;

[tbarzic, 2017/05/11 00:38:27]

do you need this?

[Luke Sorenson, 2017/05/11 23:57:59]

Done.

+    LOG(ERROR) << "Failed to start media analytics process via Upstart.";
+    media_perception::State state;
+    state.status = media_perception::STATUS_UNINITIALIZED;
+    callback.Run(false, std::move(state));
+    return;
+  }
+  analytics_process_running_ = true;
+  SetStateInternal(callback, state);
+}
+
+void MediaPerceptionAPIManager::StateCallback(const APIStateCallback& callback,
+                                              bool succeeded,
+                                              const mri::State& state_proto) {
+  media_perception::State state;
+  if (!succeeded) {
+    state.status = media_perception::STATUS_TIMEOUT;
+    callback.Run(false, std::move(state));

[tbarzic, 2017/05/11 00:38:27]

instead of passing error code via state, can you change |success| to an enum
with error codes.

Or just return state (with not success parameter) and forward it to the app

[Luke Sorenson, 2017/05/11 23:57:59]

The success parameter allows me to easily determine if I need to call
Respond(Error()). Would be strange to have another set of success enums in
addition to the failure enums in State.Status, especially because we want the
analytics process to eventually support providing errors back over the State
proto.

[tbarzic, 2017/05/12 01:00:21]

well you already have two success enums - only one happens to be {TRUE, FALSE}.
why not expand this to report a better error to the app.
returning a bool for success, and then stuffing the error code into the |state|
so the calling code
can fetch it from there is confusing (and conflates dbus service state with
failure to request service state to be set).

Error reported by analytics process is different - I do not think you should
raise
extension function error in that case (as you already don't), since setState
method succeeded,
and it returned actual service state.

[Luke Sorenson, 2017/05/12 17:07:30]

Done.

+    return;
+  }
+  callback.Run(true, StateProtoToIdl(state_proto));
+}
+
+void MediaPerceptionAPIManager::GetDiagnosticsCallback(
+    const APIGetDiagnosticsCallback& callback,
+    bool succeeded,
+    const mri::Diagnostics& diagnostics_proto) {
+  if (!succeeded) {
+    callback.Run(false, media_perception::Diagnostics());
+    return;
+  }
+  callback.Run(true, DiagnosticsProtoToIdl(diagnostics_proto));
+}
+
+void MediaPerceptionAPIManager::MediaPerceptionSignalHandler(
+    const mri::MediaPerception& media_perception_proto) {
+  EventRouter* router = EventRouter::Get(browser_context_);
+  if (!router || !router->HasEventListener(
+                     media_perception::OnMediaPerception::kEventName)) {
+    LOG(WARNING) << "Router null or no event listener when receiving a "

[tbarzic, 2017/05/11 00:38:27]

Remove this log, no listener being set seems like a real possibility here, we
don't want to spew logs if that happens

I'd change this whole if to:
DCHECK(router);

[Luke Sorenson, 2017/05/11 23:57:59]

Done.

+                    "media perception signal.";
+    return;
+  }
+  media_perception::MediaPerception media_perception =
+      MediaPerceptionProtoToIdl(media_perception_proto);
+  std::unique_ptr<Event> event(
+      new Event(events::MEDIA_PERCEPTION_PRIVATE_ON_MEDIA_PERCEPTION,
+                media_perception::OnMediaPerception::kEventName,
+                media_perception::OnMediaPerception::Create(media_perception)));
+  router->BroadcastEvent(std::move(event));
+}
+
+}  // namespace extensions