[chrome.displaySource] Session notification improvements

extensions/renderer/display_source_custom_bindings.cc

Index: extensions/renderer/display_source_custom_bindings.cc
diff --git a/extensions/renderer/display_source_custom_bindings.cc b/extensions/renderer/display_source_custom_bindings.cc
index 50752c0e0a18feb6207b1e77e4d9a115ee860c76..0e072324556b5257821ed93ae0243bc36df16f52 100644
--- a/extensions/renderer/display_source_custom_bindings.cc
+++ b/extensions/renderer/display_source_custom_bindings.cc
@@ -71,8 +71,10 @@ v8::Local<v8::Value> GetChildValue(v8::Local<v8::Object> value,
 
 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>();
 
@@ -164,14 +166,18 @@ void DisplaySourceCustomBindings::StartSession(
   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();
@@ -187,10 +193,48 @@ void DisplaySourceCustomBindings::TerminateSession(
         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);
@@ -243,6 +287,7 @@ DisplaySourceSession* DisplaySourceCustomBindings::GetDisplaySession(
 
 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) {
@@ -250,6 +295,7 @@ void DisplaySourceCustomBindings::OnSessionTerminated(int sink_id) {
   CHECK(session);
   session_map_.erase(sink_id);
   DispatchSessionTerminated(sink_id);
+  CallSessionTerminatedCallback(sink_id);
 }
 
 void DisplaySourceCustomBindings::OnSessionError(int sink_id,
@@ -257,13 +303,6 @@ void DisplaySourceCustomBindings::OnSessionError(int sink_id,
                                                  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);
 }