Add RendererFactorySelector

media/base/renderer_factory_selector.h

+// Copyright 2017 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 MEDIA_BASE_RENDERER_FACTORY_SELECTOR_H_
+#define MEDIA_BASE_RENDERER_FACTORY_SELECTOR_H_
+
+#include "base/containers/flat_map.h"
+#include "media/base/renderer_factory.h"
+
+namespace media {
+
+// RendererFactorySelector owns RendererFactory instances used within WMPI.
+// Its purpose is to aggregate the signals and centralize the logic behind
+// choosing which RendererFactory should be used when creating a new Renderer.
+class MEDIA_EXPORT RendererFactorySelector {

[ncarter (slow), 2017/04/24 17:14:33]

https://memegen.googleplex.com/9189713

[tguilbert, 2017/04/24 19:31:16]

+1

+ public:
+  enum FactoryType {
+    DEFAULT,       // DefaultRendererFactory.
+    MOJO,          // MojoRendererFactory.
+    MEDIA_PLAYER,  // MediaPlayerRendererClientFactory.
+    ADAPTIVE,      // AdaptiveRendererFactory.
+    UNKNOWN,

[whywhat, 2017/04/22 15:52:50]

nit: you could make base_factory_type_ a base::Optional to further highlight it
must be set before GetCurrentFactory calls and remove the extra enum item that
must never be used by anything but the class itself IIUC.

[tguilbert, 2017/04/24 19:31:16]

Good idea, thanks!

+  };
+
+  RendererFactorySelector();
+  ~RendererFactorySelector();
+
+  // NOTE: There should be at most one factory per factory type.
+  void AddFactory(FactoryType type, std::unique_ptr<RendererFactory> factory);
+
+  // Sets the "default" factory to be returned in the absence explicit signals
+  // indicating that another factory type should be selected instead.
+  void SetBaseFactoryType(FactoryType type);

[whywhat, 2017/04/22 15:52:50]

nit: Base/Default to match the comment?

[tguilbert, 2017/04/24 19:31:16]

I wanted to avoid confusion between the "default/base factory" and the
"DefaultRendererFactory". I will clarify the comment.

+
+  // Note: This only returns the base factory type at the moment.
+  RendererFactory* GetCurrentFactory();
+
+ private:
+  FactoryType base_factory_type_ = FactoryType::UNKNOWN;
+  base::flat_map<FactoryType, std::unique_ptr<RendererFactory>> factories_;

[whywhat, 2017/04/22 15:52:50]

nit: as I understand flat_map, it's a sorted vector; given we have up to 5
factories, it could just be an array of pointers using the enum as the index?
probably not much of an improvement for a small set anyway but sorting the
vector seems unnecessary too.

[tguilbert, 2017/04/24 19:31:16]

I went with a flat_map instead of a regular map due to this guidance
https://cs.chromium.org/chromium/src/base/containers/flat_set.h?l=27

Until the flinging renderer comes along, there are only 2 factories used at the
same time (Default or Mojo, with Adaptive/Courier or Mediaplayer).

I prefer using flat_map since it has simple semantics. I will reconsider this if
someone feels strongly about it.

[whywhat, 2017/04/24 20:35:48]

Totally agree that it's not worth a map, just thought that something like
std::unique_ptr<RendererFactory> factories_[FACTORIES_TYPE_MAX + 1] could be
even simpler.

+  DISALLOW_COPY_AND_ASSIGN(RendererFactorySelector);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_RENDERER_FACTORY_H_