[chrome.displaySource] Session notification improvements

extensions/renderer/display_source_custom_bindings.cc

 // Copyright 2015 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/renderer/display_source_custom_bindings.h"
 
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "content/public/child/v8_value_converter.h"
@@ skipping 53 matching lines @@
     }
   }
 
   return v8::Null(isolate);
 }
 
 }  // namespace
 
 void DisplaySourceCustomBindings::StartSession(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
-  CHECK_EQ(1, args.Length());
+  CHECK_EQ(2, args.Length());
   CHECK(args[0]->IsObject());
+  CHECK(args[1]->IsFunction());
+
   v8::Isolate* isolate = context()->isolate();
   v8::Local<v8::Object> start_info = args[0].As<v8::Object>();
 
   v8::Local<v8::Value> sink_id_val =
       GetChildValue(start_info, "sinkId", isolate);
   CHECK(sink_id_val->IsInt32());
   const int sink_id = sink_id_val->ToInt32(isolate)->Value();
   if (GetDisplaySession(sink_id)) {
     isolate->ThrowException(v8::Exception::Error(v8::String::NewFromUtf8(
         isolate, kSessionAlreadyStarted)));
@@ skipping 71 matching lines @@
       weak_factory_.GetWeakPtr());
   auto on_terminated_callback = base::Bind(
       &DisplaySourceCustomBindings::OnSessionTerminated,
       weak_factory_.GetWeakPtr());
   auto on_error_callback = base::Bind(
       &DisplaySourceCustomBindings::OnSessionError,
       weak_factory_.GetWeakPtr());
   session->SetCallbacks(on_started_callback,
                         on_terminated_callback,
                         on_error_callback);
+  v8::Global<v8::Function> callback(isolate, args[1].As<v8::Function>());
+  session_started_cb_map_.insert(std::make_pair(sink_id, std::move(callback)));
+
   session->Start();
   session_map_.insert(std::make_pair(sink_id, std::move(session)));
 }
 
 void DisplaySourceCustomBindings::TerminateSession(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
-  CHECK_EQ(1, args.Length());
+  CHECK_EQ(2, args.Length());
   CHECK(args[0]->IsInt32());
+  CHECK(args[1]->IsFunction());
 
   v8::Isolate* isolate = context()->isolate();
   int sink_id = args[0]->ToInt32(args.GetIsolate())->Value();
   DisplaySourceSession* session = GetDisplaySession(sink_id);
   if (!session) {
     isolate->ThrowException(v8::Exception::Error(v8::String::NewFromUtf8(
         isolate, kSessionNotFound)));
     return;
   }
 
   if (session->state() == DisplaySourceSession::Terminating) {
     isolate->ThrowException(v8::Exception::Error(v8::String::NewFromUtf8(
         isolate, kSessionAlreadyTerminating)));
     return;
   }
+  v8::Global<v8::Function> callback(isolate, args[1].As<v8::Function>());
+  session_finished_cb_map_.insert(std::make_pair(sink_id, std::move(callback)));
   // The session will get removed from session_map_ in OnSessionTerminated.
   session->Terminate();
 }
 
+void DisplaySourceCustomBindings::CallSessionStartedCallback(
+    int sink_id,
+    const std::string& error_message) {
+  auto iter = session_started_cb_map_.find(sink_id);
+  if (iter == session_started_cb_map_.end())
+    return;
+  CallCompletionCallback(iter->second, error_message);
+  session_started_cb_map_.erase(iter);
+}
+
+void DisplaySourceCustomBindings::CallSessionTerminatedCallback(
+    int sink_id,
+    const std::string& error_message) {
+  auto iter = session_finished_cb_map_.find(sink_id);
+  if (iter == session_finished_cb_map_.end())
+    return;
+  CallCompletionCallback(iter->second, error_message);
+  session_finished_cb_map_.erase(iter);
+}
+
+void DisplaySourceCustomBindings::CallCompletionCallback(
+    const v8::Global<v8::Function>& callback,
+    const std::string& error_message) {
+  v8::Isolate* isolate = context()->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Context::Scope context_scope(context()->v8_context());
+
+  v8::Local<v8::Value> callback_args[1];
+  if (error_message.empty())
+    callback_args[0] = v8::Null(isolate);
+  else
+    callback_args[0] = v8::String::NewFromUtf8(isolate, error_message.c_str());
+  context()->CallFunction(v8::Local<v8::Function>::New(isolate, callback), 1,
+                          callback_args);
+}
+
 void DisplaySourceCustomBindings::DispatchSessionStarted(int sink_id) const {
   v8::Isolate* isolate = context()->isolate();
   v8::HandleScope handle_scope(isolate);
   v8::Context::Scope context_scope(context()->v8_context());
   v8::Local<v8::Array> event_args = v8::Array::New(isolate, 1);
   event_args->Set(0, v8::Integer::New(isolate, sink_id));
   context()->DispatchEvent("displaySource.onSessionStarted", event_args);
 }
 
 void DisplaySourceCustomBindings::DispatchSessionTerminated(int sink_id) const {
@@ skipping 32 matching lines @@
 DisplaySourceSession* DisplaySourceCustomBindings::GetDisplaySession(
     int sink_id) const {
   auto iter = session_map_.find(sink_id);
   if (iter != session_map_.end())
     return iter->second.get();
   return nullptr;
 }
 
 void DisplaySourceCustomBindings::OnSessionStarted(int sink_id) {
   DispatchSessionStarted(sink_id);
+  CallSessionStartedCallback(sink_id);

[asargent_no_longer_on_chrome, 2016/02/09 00:07:56]

One question that came to mind seeing this - does it seem likely that in actual
programs, you'd want both the standalone sessionStarted event *and* the callback
argument to startSession?

For something like the chrome.tabs API, we have a create method that lets you
create new browser tabs, and that does have a callback to let you know when the
tab is created, and there is also a standalone tabs.onCreated event, but it's
useful to have both because many tabs are created by things like user action
outside of the context of the tabs API. In your case, I'm wondering if it would
be common for anywhere in a program to care about sessions being started
*except* for the code that just asked to start one. If it is, that's fine, but
if not you might consider getting rid of the standalone event.

[Mikhail, 2016/02/09 14:26:42]

Good point. Unlike session errors or termination, session start can only be
initiated from the API 'startSession' call, so 'sessionStarted' is duplicating
the method completion callback and can be dropped. However the
'chrome.displaySource' API proposal had been agreed. Would it be possible to
make such API changes now?

[asargent_no_longer_on_chrome, 2016/02/09 17:04:05]

Since the API is still only available in dev channel and not officially launched
in stable, you are still free to make breaking changes if you want. 

[Mikhail, 2016/02/10 15:58:51]

The 'chrome.displaySource' is removed now.

 }
 
 void DisplaySourceCustomBindings::OnSessionTerminated(int sink_id) {
   DisplaySourceSession* session = GetDisplaySession(sink_id);
   CHECK(session);
   session_map_.erase(sink_id);
   DispatchSessionTerminated(sink_id);
+  CallSessionTerminatedCallback(sink_id);
 }
 
 void DisplaySourceCustomBindings::OnSessionError(int sink_id,
                                                  DisplaySourceErrorType type,
                                                  const std::string& message) {
   DisplaySourceSession* session = GetDisplaySession(sink_id);
   CHECK(session);
-  if (session->state() != DisplaySourceSession::Established &&
-      session->state() != DisplaySourceSession::Terminating) {
-    // Error has occured before the session has actually started,
-    // no need to wait for session termination notification.
-    session_map_.erase(sink_id);
-  }
-
   DispatchSessionError(sink_id, type, message);
 }
 
 }  // extensions