Factor out authorization from AudioRendererHost.

content/browser/renderer_host/media/audio_output_authorization_handler.h

+// Copyright 2016 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.
+
+// This class implements authorization checking for AudioRendererHost.
+#ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_AUTHORIZATION_HANDLER_H_
+#define CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_AUTHORIZATION_HANDLER_H_
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/callback_forward.h"
+#include "base/memory/weak_ptr.h"
+#include "content/browser/media/media_devices_permission_checker.h"
+#include "content/browser/renderer_host/media/media_stream_manager.h"
+#include "media/audio/audio_device_description.h"
+#include "media/audio/audio_manager.h"
+#include "media/base/audio_parameters.h"
+#include "media/base/output_device_info.h"
+
+namespace content {
+
+// This class, which lives on the IO thread, handles the logic of an IPC device
+// request from the renderer. It checks which device to use (in case of using
+// session_id to select device), verifies that the renderer is authorized to use
+// the device, and gets the default device parameters for the selected audio
+// device.
+class CONTENT_EXPORT AudioOutputAuthorizationHandler {
+ public:
+  // The result of an authorization check. In addition to the status,
+  // it indicates whether a device was found using the session_id in the
+  // variable should_send_id, in which case the renderer expects to get the id

[Guido Urdaneta, 2016/11/16 12:40:19]

should_send_id -> |should_send_id|. Also, why is this necessary?
The renderer always knows if it needs to read the device_id coming in the reply
or not.

|translated_device_id| is not a good name since it could be interpreted as
translated to raw or translated to hashed (as happened to me in a previous
round). Since this is intended to be raw, perhaps it's better to use
|raw_device_id|.

Finally, wouldn't it be more consistent for this class to receive and return
hashed IDs instead of receiving hashed IDs and returning raw IDs?
It would provide a better path for a mojo migration since this callback would be
the callback sent by the mojo client.

[Max Morin, 2016/11/16 14:12:19]

The ARH (and mojo replacement) needs to know if it should send the id to the
renderer. This is what |should_send_id| indicates. They also need the raw id for
the stream, since the raw id is the one that the audio manager expects. The
current plan is not to just send the callback from the renderer straight here,
there will be a class in-between (~taking the place of AudioRendererHost).

[Guido Urdaneta, 2016/11/16 14:28:33]

I see. You can't send an empty string for some of the cases where
|should_send_id| is false because you need the ID to create the streams in ARH.

+  // hash. It also has the default audio parameters for the device, and the id
+  // for the device, which is needed to open a stream for the device. This id
+  // is not hashed, so it must be hashed before sending it to the renderer.
+  // TODO(maxmorin): Change to OnceCallback once base:: code is ready for it.
+  using AuthorizationCompletedCallback =
+      base::Callback<void(media::OutputDeviceStatus status,
+                          bool should_send_id,
+                          const media::AudioParameters& params,
+                          const std::string& translated_device_id)>;
+
+  AudioOutputAuthorizationHandler(MediaStreamManager* media_stream_manager,
+                                  int render_process_id_,
+                                  const std::string& salt);
+
+  ~AudioOutputAuthorizationHandler();
+
+  // Checks authorization of the device with the hashed id device_id for the
+  // given render frame id and security origin, or uses session_id for
+  // authorization. Looks up device id (if session_id is used for device
+  // selection) and default device parameters.
+  void RequestDeviceAuthorization(int render_frame_id,
+                                  int session_id,
+                                  const std::string& device_id,
+                                  const url::Origin& security_origin,
+                                  AuthorizationCompletedCallback cb) const;
+
+  void OverridePermissionsForTesting(bool override_value);

[Guido Urdaneta, 2016/11/16 12:40:19]

Document what |override_value| means.

[Max Morin, 2016/11/16 14:12:19]

Done.

+
+ private:
+  // The comments give a simplified overview of implementation. The sequence of
+  // calls can be broken at various places, e.g. if access rights are missing or
+  // the device cannot be found. In this case, a non-ok status indicating the
+  // error is returned through the callback.
+
+  // RequestDeviceAuthorization will check if session_id should be used. If yes,

[Guido Urdaneta, 2016/11/16 12:40:19]

session_id -> |session_id|

I think such detailed comments for the private methods are overkill and will be
a maintenance burden as the implementation changes over time.
I would prefer shorter comments that explain what the methods do (or why) or no
comment at all, than this kind of detailed comment saying how the methods do
what they do.

[Max Morin, 2016/11/16 14:12:19]

I don't disagree, but I'm not sure how I would find a solution that both you and
Olga find acceptable, since she wanted more comments here.

[Guido Urdaneta, 2016/11/16 14:28:33]

I'm OK with comments that clarify what the method does if the name is not
sufficiently obvious. 
For example, I would leave the first sentence in the comment for
TranslateDeviceID, but remove the second.
Details about the internal implementation details are never as precise as the
code and can easily become obsolete.

+  // fetch saved device and parameters and skip to DeviceParametersRecieved.
+  // Otherwise, check whether the renderer has access to the output device
+  // (async in case of a nondefault device). Result goes to AccessChecked.
+
+  // Calls GetDeviceParameters for default devices. Nondefault devices first
+  // goes to TranslateDeviceId.
+  void AccessChecked(AuthorizationCompletedCallback cb,
+                     const std::string& device_id,
+                     const url::Origin& security_origin,
+                     bool have_access) const;
+
+  // Takes the hashed device id device_id and finds the corresponding raw device
+  // id. After that, it calls GetDeviceParameters with the result.
+  void TranslateDeviceID(AuthorizationCompletedCallback cb,
+                         const std::string& device_id,
+                         const url::Origin& security_origin,
+                         const MediaDeviceEnumeration& enumeration) const;
+
+  // Takes a raw device id and gets the default device parameters for the device
+  // from the OS. Calls DeviceParametersReceived.
+  void GetDeviceParameters(AuthorizationCompletedCallback cb,
+                           const std::string& translated_device_id) const;
+
+  // Returns by calling the callback. should_send_id indicates whether the
+  // renderer expects to get the chosen id back, which is the case if a device

[Guido Urdaneta, 2016/11/16 12:40:19]

The original contract was to let the renderer know which output device was going
to be used, not if a match was found. Therefore, the empty string (default
device) was sent when no device was found.
For the case when the renderer already knows the device ID, the empty string was
sent as a don't-care value.

[Max Morin, 2016/11/16 14:12:19]

Yes, I preserve this behavior by providing the AudioRendererHost with a bool
indicating if this is a request for which a device was found for a specific
session id.

+  // was found using a session id.
+  void DeviceParametersReceived(
+      AuthorizationCompletedCallback cb,
+      bool should_send_id,
+      const std::string& translated_device_id,
+      const media::AudioParameters& output_params) const;
+
+  MediaStreamManager* const media_stream_manager_;
+  std::unique_ptr<MediaDevicesPermissionChecker> permission_checker_;
+  const int render_process_id_;
+  const std::string salt_;
+  // All access is on the IO thread, and taking a weak pointer to const looks
+  // const, so this can be mutable.
+  mutable base::WeakPtrFactory<const AudioOutputAuthorizationHandler>
+      weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioOutputAuthorizationHandler);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_AUTHORIZATION_HANDLER_H_