[chrome.displaySource] further implementation of call completion callbacks

The API implementation determines when and with which
arguments the 'startSession'/'terminateSession' completion
callbacks are invoked.

The 'exceeded_session_limit_error' session error type is
eliminated and substituted with 'startSession' completion
callback call (which is more natural since this problem is
detected before a new session is started).

JS bindings code is simplified.

BUG=242107

Review URL: https://codereview.chromium.org/1730583002

Cr-Commit-Position: refs/heads/master@{#377864}

7 files changed, 171 insertions, 139 deletions

diff --git a/extensions/renderer/api/display_source/display_source_session.cc b/extensions/renderer/api/display_source/display_source_session.cc
index df83007..20b1442 100644
--- a/extensions/renderer/api/display_source/display_source_session.cc
+++ b/extensions/renderer/api/display_source/display_source_session.cc
@@ -22,15 +22,12 @@ DisplaySourceSession::DisplaySourceSession()
 
 DisplaySourceSession::~DisplaySourceSession() = default;
 
-void DisplaySourceSession::SetCallbacks(
-    const SinkIdCallback& started_callback,
-    const SinkIdCallback& terminated_callback,
+void DisplaySourceSession::SetNotificationCallbacks(
+    const base::Closure& terminated_callback,
     const ErrorCallback& error_callback) {
-  DCHECK(started_callback_.is_null());
   DCHECK(terminated_callback_.is_null());
   DCHECK(error_callback_.is_null());
 
-  started_callback_ = started_callback;
   terminated_callback_ = terminated_callback;
   error_callback_ = error_callback;
 }
diff --git a/extensions/renderer/api/display_source/display_source_session.h b/extensions/renderer/api/display_source/display_source_session.h
index 2c4053c..548958e 100644
--- a/extensions/renderer/api/display_source/display_source_session.h
+++ b/extensions/renderer/api/display_source/display_source_session.h
@@ -23,10 +23,10 @@ using DisplaySourceErrorType = api::display_source::ErrorType;
 // This class represents a generic display source session interface.
 class DisplaySourceSession {
  public:
-  using SinkIdCallback = base::Callback<void(int sink_id)>;
+  using CompletionCallback =
+      base::Callback<void(bool success, const std::string& error_description)>;
   using ErrorCallback =
-      base::Callback<void(int sink_id,
-                          DisplaySourceErrorType error_type,
+      base::Callback<void(DisplaySourceErrorType error_type,
                           const std::string& error_description)>;
 
   // State flow is ether:
@@ -47,36 +47,43 @@ class DisplaySourceSession {
   // Starts the session.
   // The session state should be set to 'Establishing' immediately after this
   // method is called.
-  virtual void Start() = 0;
+  // |callback| : Called with 'success' flag set to 'true' if the session is
+  //              successfully started (state should be set to 'Established')
+  //
+  //              Called with 'success' flag set to 'false' if the session
+  //              has failed to start (state should be set back to 'Idle').
+  //              The 'error_description' argument contains description of
+  //              an error that had caused the call falure.
+  virtual void Start(const CompletionCallback& callback) = 0;
 
   // Terminates the session.
   // The session state should be set to 'Terminating' immediately after this
   // method is called.
-  virtual void Terminate() = 0;
+  // |callback| : Called with 'success' flag set to 'true' if the session is
+  //              successfully terminated (state should be set to 'Idle')
+  //
+  //              Called with 'success' flag set to 'false' if the session
+  //              could not terminate (state should not be modified).
+  //              The 'error_description' argument contains description of
+  //              an error that had caused the call falure.
+  virtual void Terminate(const CompletionCallback& callback) = 0;
 
   State state() const { return state_; }
 
   // Sets the callbacks invoked to inform about the session's state changes.
   // It is required to set the callbacks before the session is started.
-  // |started_callback| : Called when the session was actually started (state
-  //                      should be set to 'Established')
-  // |terminated_callback| : Called when the session was actually started (state
+  // |terminated_callback| : Called when session was terminated (state
   //                         should be set to 'Idle')
   // |error_callback| : Called if a fatal error has occured and the session
-  //                    either cannot be started (if was invoked in
-  //                    'Establishing' state) or will be terminated soon for
-  //                    emergency reasons (if was invoked in 'Established'
-  //                    state).
-  void SetCallbacks(const SinkIdCallback& started_callback,
-                    const SinkIdCallback& terminated_callback,
-                    const ErrorCallback& error_callback);
+  //                    will be terminated soon for emergency reasons.
+  void SetNotificationCallbacks(const base::Closure& terminated_callback,
+                                const ErrorCallback& error_callback);
 
  protected:
   DisplaySourceSession();
 
   State state_;
-  SinkIdCallback started_callback_;
-  SinkIdCallback terminated_callback_;
+  base::Closure terminated_callback_;
   ErrorCallback error_callback_;
 
  private:
diff --git a/extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc b/extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc
index f6d1104..78fd285 100644
--- a/extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc
+++ b/extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc
@@ -38,70 +38,93 @@ WiFiDisplaySession::WiFiDisplaySession(
 WiFiDisplaySession::~WiFiDisplaySession() {
 }
 
-void WiFiDisplaySession::Start() {
-  DCHECK(state_ == DisplaySourceSession::Idle);
+void WiFiDisplaySession::Start(const CompletionCallback& callback) {
+  DCHECK_EQ(DisplaySourceSession::Idle, state_);
+  DCHECK(!terminated_callback_.is_null())
+      << "Should be set with 'SetNotificationCallbacks'";
+  DCHECK(!error_callback_.is_null())
+      << "Should be set with 'SetNotificationCallbacks'";
+
   service_->Connect(params_.sink_id, params_.auth_method, params_.auth_data);
   state_ = DisplaySourceSession::Establishing;
-  if (!started_callback_.is_null())
-    started_callback_.Run(params_.sink_id);
+  start_completion_callback_ = callback;
 }
 
-void WiFiDisplaySession::Terminate() {
-  DCHECK(state_ != DisplaySourceSession::Idle);
-  switch (state_) {
-  case DisplaySourceSession::Idle:
-  case DisplaySourceSession::Terminating:
-    // Nothing to do.
-    return;
-  case DisplaySourceSession::Establishing:
-  case DisplaySourceSession::Established:
-    service_->Disconnect();
-    state_ = DisplaySourceSession::Terminating;
-    break;
-  default:
-    NOTREACHED();
-  }
+void WiFiDisplaySession::Terminate(const CompletionCallback& callback) {
+  DCHECK_EQ(DisplaySourceSession::Established, state_);
+  service_->Disconnect();
+  state_ = DisplaySourceSession::Terminating;
+  teminate_completion_callback_ = callback;
 }
 
-void WiFiDisplaySession::OnEstablished(const mojo::String& ip_address) {
-  DCHECK(state_ != DisplaySourceSession::Established);
+void WiFiDisplaySession::OnConnected(const mojo::String& ip_address) {
+  DCHECK_EQ(DisplaySourceSession::Established, state_);
   ip_address_ = ip_address;
-  state_ = DisplaySourceSession::Established;
+  // TODO(Mikhail): Start Wi-Fi Display session control message exchange.
+}
+
+void WiFiDisplaySession::OnConnectRequestHandled(bool success,
+                                                 const mojo::String& error) {
+  DCHECK_EQ(DisplaySourceSession::Establishing, state_);
+  state_ =
+      success ? DisplaySourceSession::Established : DisplaySourceSession::Idle;
+  RunStartCallback(success, error);
 }
 
 void WiFiDisplaySession::OnTerminated() {
-  DCHECK(state_ != DisplaySourceSession::Idle);
+  DCHECK_NE(DisplaySourceSession::Idle, state_);
   state_ = DisplaySourceSession::Idle;
-  if (!terminated_callback_.is_null())
-    terminated_callback_.Run(params_.sink_id);
+  terminated_callback_.Run();
+}
+
+void WiFiDisplaySession::OnDisconnectRequestHandled(bool success,
+                                                    const mojo::String& error) {
+  RunTerminateCallback(success, error);
 }
 
 void WiFiDisplaySession::OnError(int32_t type,
                                  const mojo::String& description) {
   DCHECK(type > api::display_source::ERROR_TYPE_NONE
          && type <= api::display_source::ERROR_TYPE_LAST);
-  if (!error_callback_.is_null())
-    error_callback_.Run(params_.sink_id, static_cast<ErrorType>(type),
-                        description);
+  DCHECK_EQ(DisplaySourceSession::Established, state_);
+  error_callback_.Run(static_cast<ErrorType>(type), description);
 }
 
 void WiFiDisplaySession::OnMessage(const mojo::String& data) {
-  DCHECK(state_ == DisplaySourceSession::Established);
+  DCHECK_EQ(DisplaySourceSession::Established, state_);
 }
 
 void WiFiDisplaySession::OnConnectionError() {
-  if (!error_callback_.is_null()) {
-    error_callback_.Run(params_.sink_id,
-                        api::display_source::ERROR_TYPE_UNKNOWN_ERROR,
-                        kErrorInternal);
+  // We must explicitly notify the session termination as it will never
+  // arrive from browser process (IPC is broken).
+  switch (state_) {
+    case DisplaySourceSession::Idle:
+    case DisplaySourceSession::Establishing:
+      RunStartCallback(false, kErrorInternal);
+      break;
+    case DisplaySourceSession::Terminating:
+    case DisplaySourceSession::Established:
+      error_callback_.Run(api::display_source::ERROR_TYPE_UNKNOWN_ERROR,
+                          kErrorInternal);
+      state_ = DisplaySourceSession::Idle;
+      terminated_callback_.Run();
+      break;
+    default:
+      NOTREACHED();
   }
+}
 
-  if (state_ != DisplaySourceSession::Idle) {
-    // We must explicitly notify the session termination as it will never
-    // arrive from browser process (IPC is broken).
-    if (!terminated_callback_.is_null())
-      terminated_callback_.Run(params_.sink_id);
-  }
+void WiFiDisplaySession::RunStartCallback(bool success,
+                                          const std::string& error_message) {
+  if (!start_completion_callback_.is_null())
+    start_completion_callback_.Run(success, error_message);
+}
+
+void WiFiDisplaySession::RunTerminateCallback(
+    bool success,
+    const std::string& error_message) {
+  if (!teminate_completion_callback_.is_null())
+    teminate_completion_callback_.Run(success, error_message);
 }
 
 }  // namespace extensions
diff --git a/extensions/renderer/api/display_source/wifi_display/wifi_display_session.h b/extensions/renderer/api/display_source/wifi_display/wifi_display_session.h
index 5d10308..7eeb987 100644
--- a/extensions/renderer/api/display_source/wifi_display/wifi_display_session.h
+++ b/extensions/renderer/api/display_source/wifi_display/wifi_display_session.h
@@ -21,24 +21,34 @@ class WiFiDisplaySession: public DisplaySourceSession,
   ~WiFiDisplaySession() override;
 
  private:
+  using DisplaySourceSession::CompletionCallback;
   // DisplaySourceSession overrides.
-  void Start() override;
-  void Terminate() override;
+  void Start(const CompletionCallback& callback) override;
+  void Terminate(const CompletionCallback& callback) override;
 
   // WiFiDisplaySessionServiceClient overrides.
-  void OnEstablished(const mojo::String& ip_address) override;
+  void OnConnected(const mojo::String& ip_address) override;
+  void OnConnectRequestHandled(bool success,
+                               const mojo::String& error) override;
   void OnTerminated() override;
+  void OnDisconnectRequestHandled(bool success,
+                                  const mojo::String& error) override;
   void OnError(int32_t type, const mojo::String& description) override;
   void OnMessage(const mojo::String& data) override;
 
   // A connection error handler for the mojo objects used in this class.
   void OnConnectionError();
 
+  void RunStartCallback(bool success, const std::string& error = "");
+  void RunTerminateCallback(bool success, const std::string& error = "");
+
  private:
   WiFiDisplaySessionServicePtr service_;
   mojo::Binding<WiFiDisplaySessionServiceClient> binding_;
   std::string ip_address_;
   DisplaySourceSessionParams params_;
+  CompletionCallback start_completion_callback_;
+  CompletionCallback teminate_completion_callback_;
   base::WeakPtrFactory<WiFiDisplaySession> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(WiFiDisplaySession);
diff --git a/extensions/renderer/display_source_custom_bindings.cc b/extensions/renderer/display_source_custom_bindings.cc
index ab68553..1a021c1 100644
--- a/extensions/renderer/display_source_custom_bindings.cc
+++ b/extensions/renderer/display_source_custom_bindings.cc
@@ -67,13 +67,17 @@ v8::Local<v8::Value> GetChildValue(v8::Local<v8::Object> value,
   return v8::Null(isolate);
 }
 
+int32_t GetCallbackId() {
+  static int32_t sCallId = 0;
+  return ++sCallId;
+}
+
 }  // namespace
 
 void DisplaySourceCustomBindings::StartSession(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
-  CHECK_EQ(2, args.Length());
+  CHECK_EQ(1, 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>();
@@ -154,32 +158,28 @@ void DisplaySourceCustomBindings::StartSession(
     return;
   }
 
-  auto on_started_callback = base::Bind(
-      &DisplaySourceCustomBindings::OnSessionStarted,
-      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);
-
-  CallbackInfo cb_info = GetCallbackInfo(kStarted, sink_id);
-  args.GetReturnValue().Set(static_cast<int32_t>(cb_info.call_id));
-  callbacks_.push_back(cb_info);
-
-  session->Start();
+  auto on_terminated_callback =
+      base::Bind(&DisplaySourceCustomBindings::OnSessionTerminated,
+                 weak_factory_.GetWeakPtr(), sink_id);
+  auto on_error_callback =
+      base::Bind(&DisplaySourceCustomBindings::OnSessionError,
+                 weak_factory_.GetWeakPtr(), sink_id);
+  session->SetNotificationCallbacks(on_terminated_callback, on_error_callback);
+
+  int32_t call_id = GetCallbackId();
+  args.GetReturnValue().Set(call_id);
+
+  auto on_call_completed =
+      base::Bind(&DisplaySourceCustomBindings::OnSessionStarted,
+                 weak_factory_.GetWeakPtr(), sink_id, call_id);
+  session->Start(on_call_completed);
   session_map_.insert(std::make_pair(sink_id, std::move(session)));
 }
 
 void DisplaySourceCustomBindings::TerminateSession(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
-  CHECK_EQ(2, args.Length());
+  CHECK_EQ(1, args.Length());
   CHECK(args[0]->IsInt32());
-  CHECK(args[1]->IsFunction());
 
   v8::Isolate* isolate = context()->isolate();
   int sink_id = args[0]->ToInt32(args.GetIsolate())->Value();
@@ -190,39 +190,44 @@ void DisplaySourceCustomBindings::TerminateSession(
     return;
   }
 
-  if (session->state() == DisplaySourceSession::Terminating) {
+  DisplaySourceSession::State state = session->state();
+  DCHECK_NE(state, DisplaySourceSession::Idle);
+  if (state == DisplaySourceSession::Establishing) {
+    // 'session started' callback has not yet been invoked.
+    // This session is not existing for the user.
+    isolate->ThrowException(v8::Exception::Error(
+        v8::String::NewFromUtf8(isolate, kSessionNotFound)));
+    return;
+  }
+
+  if (state == DisplaySourceSession::Terminating) {
     isolate->ThrowException(v8::Exception::Error(v8::String::NewFromUtf8(
         isolate, kSessionAlreadyTerminating)));
     return;
   }
 
-  CallbackInfo cb_info = GetCallbackInfo(kTerminated, sink_id);
-  args.GetReturnValue().Set(static_cast<int32_t>(cb_info.call_id));
-  callbacks_.push_back(cb_info);
+  int32_t call_id = GetCallbackId();
+  args.GetReturnValue().Set(call_id);
 
+  auto on_call_completed =
+      base::Bind(&DisplaySourceCustomBindings::OnCallCompleted,
+                 weak_factory_.GetWeakPtr(), call_id);
   // The session will get removed from session_map_ in OnSessionTerminated.
-  session->Terminate();
+  session->Terminate(on_call_completed);
 }
 
-void DisplaySourceCustomBindings::CallCompletionCallback(
-    int sink_id,
-    CallbackType type,
+void DisplaySourceCustomBindings::OnCallCompleted(
+    int call_id,
+    bool success,
     const std::string& error_message) {
-  auto predicate = [sink_id, type](const CallbackInfo& info) -> bool {
-    return info.sink_id == sink_id && info.type == type;
-  };
-  auto it = std::find_if(callbacks_.begin(), callbacks_.end(), predicate);
-  if (it == callbacks_.end())
-    return;
-
   v8::Isolate* isolate = context()->isolate();
   ModuleSystem* module_system = context()->module_system();
   v8::HandleScope handle_scope(isolate);
   v8::Context::Scope context_scope(context()->v8_context());
 
   v8::Local<v8::Value> callback_args[2];
-  callback_args[0] = v8::Integer::New(isolate, it->call_id);
-  if (error_message.empty())
+  callback_args[0] = v8::Integer::New(isolate, call_id);
+  if (success)
     callback_args[1] = v8::Null(isolate);
   else
     callback_args[1] = v8::String::NewFromUtf8(isolate, error_message.c_str());
@@ -231,6 +236,19 @@ void DisplaySourceCustomBindings::CallCompletionCallback(
                                   callback_args);
 }
 
+void DisplaySourceCustomBindings::OnSessionStarted(
+    int sink_id,
+    int call_id,
+    bool success,
+    const std::string& error_message) {
+  CHECK(GetDisplaySession(sink_id));
+  if (!success) {
+    // Session has failed to start, removing it.
+    session_map_.erase(sink_id);
+  }
+  OnCallCompleted(call_id, success, error_message);
+}
+
 void DisplaySourceCustomBindings::DispatchSessionTerminated(int sink_id) const {
   v8::Isolate* isolate = context()->isolate();
   v8::HandleScope handle_scope(isolate);
@@ -272,32 +290,17 @@ DisplaySourceSession* DisplaySourceCustomBindings::GetDisplaySession(
   return nullptr;
 }
 
-void DisplaySourceCustomBindings::OnSessionStarted(int sink_id) {
-  CallCompletionCallback(sink_id, kStarted);
-}
-
 void DisplaySourceCustomBindings::OnSessionTerminated(int sink_id) {
-  DisplaySourceSession* session = GetDisplaySession(sink_id);
-  CHECK(session);
+  CHECK(GetDisplaySession(sink_id));
   session_map_.erase(sink_id);
   DispatchSessionTerminated(sink_id);
-  CallCompletionCallback(sink_id, kTerminated);
 }
 
 void DisplaySourceCustomBindings::OnSessionError(int sink_id,
                                                  DisplaySourceErrorType type,
                                                  const std::string& message) {
-  DisplaySourceSession* session = GetDisplaySession(sink_id);
-  CHECK(session);
+  CHECK(GetDisplaySession(sink_id));
   DispatchSessionError(sink_id, type, message);
 }
 
-DisplaySourceCustomBindings::CallbackInfo
-DisplaySourceCustomBindings::GetCallbackInfo(
-    DisplaySourceCustomBindings::CallbackType type,
-    int sink_id) const {
-  static int sCallId = 0;
-  return {type, sink_id, ++sCallId};
-}
-
 }  // extensions
diff --git a/extensions/renderer/display_source_custom_bindings.h b/extensions/renderer/display_source_custom_bindings.h
index 2a65445..72ff6a4 100644
--- a/extensions/renderer/display_source_custom_bindings.h
+++ b/extensions/renderer/display_source_custom_bindings.h
@@ -30,18 +30,20 @@ class DisplaySourceCustomBindings : public ObjectBackedNativeHandler {
   void TerminateSession(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   // Call completion callbacks.
-  enum CallbackType { kStarted, kTerminated };
-  void CallCompletionCallback(int sink_id,
-                              CallbackType type,
-                              const std::string& error_message = "");
+  void OnCallCompleted(int call_id,
+                       bool success,
+                       const std::string& error_message);
+  void OnSessionStarted(int sink_id,
+                        int call_id,
+                        bool success,
+                        const std::string& error_message);
   // Dispatch events
   void DispatchSessionTerminated(int sink_id) const;
   void DispatchSessionError(int sink_id,
                             DisplaySourceErrorType type,
                             const std::string& message) const;
 
-  // DisplaySession callbacks.
-  void OnSessionStarted(int sink_id);
+  // DisplaySession notification callbacks.
   void OnSessionTerminated(int sink_id);
   void OnSessionError(int sink_id,
                       DisplaySourceErrorType type,
@@ -50,16 +52,6 @@ class DisplaySourceCustomBindings : public ObjectBackedNativeHandler {
   DisplaySourceSession* GetDisplaySession(int sink_id) const;
 
   std::map<int, scoped_ptr<DisplaySourceSession>> session_map_;
-  // Data of a call completion callback.
-  struct CallbackInfo {
-    CallbackType type;
-    int sink_id;
-    int call_id;  // Each call has a unique Id.
-  };
-
-  CallbackInfo GetCallbackInfo(CallbackType type, int sink_id) const;
-
-  std::vector<CallbackInfo> callbacks_;
   base::WeakPtrFactory<DisplaySourceCustomBindings> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DisplaySourceCustomBindings);
diff --git a/extensions/renderer/resources/display_source_custom_bindings.js b/extensions/renderer/resources/display_source_custom_bindings.js
index a1345ff..38d7e3c 100644
--- a/extensions/renderer/resources/display_source_custom_bindings.js
+++ b/extensions/renderer/resources/display_source_custom_bindings.js
@@ -18,7 +18,7 @@ function callbackWrapper(callback, method, message) {
 
   try {
     if (message !== null)
-      lastError.set('displaySource.startSession', message, null, chrome);
+      lastError.set(method, message, null, chrome);
     callback();
   } finally {
     lastError.clear(chrome);
@@ -40,7 +40,7 @@ binding.registerCustomHook(function(bindingsAPI, extensionId) {
   apiFunctions.setHandleRequest(
       'startSession', function(sessionInfo, callback) {
         try {
-          var callId = natives.StartSession(sessionInfo, callbackWrapper);
+          var callId = natives.StartSession(sessionInfo);
           callbacksInfo[callId] = {
             callback: callback,
             method: 'displaySource.startSession'
@@ -52,7 +52,7 @@ binding.registerCustomHook(function(bindingsAPI, extensionId) {
   apiFunctions.setHandleRequest(
       'terminateSession', function(sink_id, callback) {
         try {
-          var callId = natives.TerminateSession(sink_id, callbackWrapper);
+          var callId = natives.TerminateSession(sink_id);
           callbacksInfo[callId] = {
             callback: callback,
             method: 'displaySource.terminateSession'