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/bad_message.h"
+#include "content/browser/renderer_host/media/media_stream_manager.h"
+#include "content/browser/renderer_host/media/media_stream_ui_proxy.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 {
+class AudioOutputAuthorizationHandler {

[Guido Urdaneta, 2016/10/28 15:17:29]

Is this class really necessary in the context of migrating to Mojo?
My first impression is that this is actually harder to read than the original,
with the new callback types, bad-message handler, and so on.

You should have a comment explaining what this class does (handle the logic for
the authorization IPC). The name of the class (and the IPC message) suggests
that this is only about authorization, but it actually also involves translating
the device ID and reading hardware parameters.

[Max Morin, 2016/11/01 11:26:20]

The class isn't strictly necessary, but I want to break AudioRendererHost into
smaller pieces, and this is one way to move a lot of self-contained logic out of
there. It could also be in the future OutputManager which will take more of ARHs
logic and own an instance of this class, but I think having smaller classes is
generally preferable. It's also true that it does many things, which is not
ideal, but the current alternative is to have it all in the ARH, so this change
makes concerns more separated, even if it's not completely separated. It also
eliminates some sources of errors (i.e. it's now easy to verify that
UMALogDeviceAuthorizationTime is called once, before it was called from seven
different places, and similarly it's nice to not have Send(new
AudioMsg_NotifyDeviceAuthorized...) in several places).

Added documentation.

[Guido Urdaneta, 2016/11/01 13:49:16]

Now that I understand it, I like it.
Maybe you should be more explicit in the comment about saying that this handles
all the logic related to the  authorization/device request IPC. 

[Max Morin, 2016/11/01 14:47:19]

Done.

+ public:
+  using AuthorizationCompletedCallback =

[Guido Urdaneta, 2016/10/28 15:17:29]

This callback type looks not trivial. You should have a comment explaining what
the semantics are, what each of the result parameters mean, etc.
I understand that this will be the replacement of the authorization reply
message in the mojo-based version.

Since you are using std::move for this callback, I think you should change it to
OnceCallback.

[Max Morin, 2016/11/01 11:26:20]

OnceCallback makes sense, but it is not possible to switch yet since many
dependencies (at least PostTaskAndReply and MediaStreamUIProxy::CheckAccess)
take RepeatableCallbacks and OnceCallbacks cannot be bound into
RepeatableCallbacks. I could maybe fix MediaStreamUIProxy, but I definitely
think PostTaskAndReply should be left to the people in crbug.com/554299.

Added documentation.

+      base::Callback<void(media::OutputDeviceStatus,
+                          const media::AudioParameters&,
+                          const std::string&)>;
+
+  using BadMessageHandler =

[Guido Urdaneta, 2016/10/28 15:17:28]

Why is a BadMessageHandler needed?
Can't you just call ReceivedBadMessage directly at the point where you detect
the bad message?
You only need the process ID, which you already have in a member variable.

[Max Morin, 2016/11/01 11:26:20]

Done.

+      base::Callback<void(content::bad_message::BadMessageReason)>;
+
+  AudioOutputAuthorizationHandler(MediaStreamManager* media_stream_manager,
+                                  int render_process_id_,
+                                  const std::string& salt);
+
+  ~AudioOutputAuthorizationHandler();
+
+  // Checks authorization of the device with device_id for the given
+  // render frame id and security origin, or uses session_id
+  // for authorization.
+  void RequestDeviceAuthorization(int render_frame_id,
+                                  int session_id,
+                                  const std::string& device_id,
+                                  const url::Origin& security_origin,
+                                  AuthorizationCompletedCallback cb);
+
+  // In case an invalid request is recieved, this handler will be called.

[Guido Urdaneta, 2016/10/28 15:17:29]

typo: recieved -> received

[Max Morin, 2016/11/01 11:26:20]

Done.

+  void SetBadMessageHandler(BadMessageHandler handler) {

[Guido Urdaneta, 2016/10/28 15:17:28]

If you're going to use a handler, why not pass it in the constructor?

[Max Morin, 2016/11/01 11:26:20]

Removed handler.

+    bad_message_handler_ = std::move(handler);
+  }
+
+ private:
+  void CheckOutputDeviceAccess(AuthorizationCompletedCallback cb,
+                               int render_frame_id,
+                               const std::string& device_id,
+                               const url::Origin& security_origin);
+
+  void AccessChecked(AuthorizationCompletedCallback cb,
+                     std::unique_ptr<MediaStreamUIProxy> ui_proxy,
+                     const std::string& device_id,
+                     const url::Origin& security_origin,
+                     bool have_access);
+
+  void TranslateDeviceID(AuthorizationCompletedCallback cb,
+                         const std::string& device_id,
+                         const url::Origin& security_origin,
+                         const MediaDeviceEnumeration& enumeration);
+
+  void DeviceParametersReceived(AuthorizationCompletedCallback cb,
+                                bool device_found,
+                                const std::string& unique_id,
+                                media::AudioParameters output_params);
+
+  MediaStreamManager* const media_stream_manager_;
+  const int render_process_id_;
+  const std::string salt_;
+  BadMessageHandler bad_message_handler_;
+  base::WeakPtrFactory<AudioOutputAuthorizationHandler> weak_factory;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioOutputAuthorizationHandler);
+};
+}  // namespace content
+#endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_AUTHORIZATION_HANDLER_H_