Making DesktopEnvironment a factory class used by ClientSession to create audio/video capturers and…

remoting/host/desktop_environment.h

 // Copyright (c) 2012 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.
 
 #ifndef REMOTING_HOST_DESKTOP_ENVIRONMENT_H_
 #define REMOTING_HOST_DESKTOP_ENVIRONMENT_H_
 
-#include <string>
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 
-#include "base/basictypes.h"
-#include "base/callback_forward.h"
-#include "base/memory/scoped_ptr.h"
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base
 
 namespace remoting {
 
 class AudioCapturer;
 class EventExecutor;
 class VideoFrameCapturer;
 
-namespace protocol {
-class ClipboardStub;
-}
-
+// Provides factory methods for creation of audio/video capturers and event
+// executor for a given desktop environment.
 class DesktopEnvironment {
  public:
-  DesktopEnvironment(scoped_ptr<AudioCapturer> audio_capturer,
-                     scoped_ptr<EventExecutor> event_executor,
-                     scoped_ptr<VideoFrameCapturer> video_capturer);
-  virtual ~DesktopEnvironment();
+  DesktopEnvironment() {}

[Wez, 2013/01/11 02:23:41]

Do you need to declare a ctor explicitly here?

[alexeypa (please no reviews), 2013/01/11 19:59:46]

It is required because of DISALLOW_COPY_AND_ASSIGN(DesktopEnvironment);

+  virtual ~DesktopEnvironment() {}
 
-  AudioCapturer* audio_capturer() const { return audio_capturer_.get(); }
-  EventExecutor* event_executor() const { return event_executor_.get(); }
-  VideoFrameCapturer* video_capturer() const { return video_capturer_.get(); }
-
-  // Starts the desktop environment passing |client_jid| of the attached
-  // authenticated session. Registers |client_clipboard| to receive
-  // notifications about local clipboard changes. |disconnect_callback| can be
-  // invoked by the DesktopEnvironment to request the client session to be
-  // disconnected. |disconnect_callback| is invoked on the same thread Start()
-  // has been called on.
-  virtual void Start(
-      scoped_ptr<protocol::ClipboardStub> client_clipboard,
-      const std::string& client_jid,
-      const base::Closure& disconnect_callback);
+  // Factory methods used to create audio/video capturers and event executor for
+  // a particular desktop environment.
+  virtual scoped_ptr<AudioCapturer> CreateAudioCapturer(
+      scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner) = 0;

[Wez, 2013/01/11 02:23:41]

nit: If you pass AutoThreadTaskRunner here then calling code can't mistakenly
supply e.g. a MessageLoopProxy, and the capturers, etc, can then be written to
assume AutoThreadTaskRunner semantics.

[Wez, 2013/01/11 02:23:41]

Why do you prefer to pass the TaskRunners to the DesktopEnvironment rather than
to the DesktopEnvironmentFactory?

[alexeypa (please no reviews), 2013/01/11 19:59:46]

Agree, but it should be done as a separate CL.

[alexeypa (please no reviews), 2013/01/11 19:59:46]

To avoid using more task runners then needed. If CreateAudioCapturer is never
called, the called does not need to provide a task runner for audio for
instance.

[Wez, 2013/01/12 01:58:06]

SGTM!

[Wez, 2013/01/12 01:58:06]

Ah, so you're thinking we would spawn the audio thread only when the first
client needs it in future?

[alexeypa (please no reviews), 2013/01/14 20:43:09]

Potentially. Basically it strikes me that we keep a lot of references to task
runners only because we need to pass them to child object later. We could
provide some way of requesting threads/task runners on demand. Then, for
instance, ChromotingHost/ClientSession will only care about the network thread
and request other threads only when creating audio/video schedulers.

+  virtual scoped_ptr<EventExecutor> CreateEventExecutor(
+      scoped_refptr<base::SingleThreadTaskRunner> input_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) = 0;
+  virtual scoped_ptr<VideoFrameCapturer> CreateVideoCapturer(
+      scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> encode_task_runner) = 0;
 
  private:
-  // Used to capture audio to deliver to clients.
-  scoped_ptr<AudioCapturer> audio_capturer_;
-
-  // Executes input and clipboard events received from the client.
-  scoped_ptr<EventExecutor> event_executor_;
-
-  // Used to capture video to deliver to clients.
-  scoped_ptr<VideoFrameCapturer> video_capturer_;
-
   DISALLOW_COPY_AND_ASSIGN(DesktopEnvironment);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_DESKTOP_ENVIRONMENT_H_