Add MediaExperimentManager::ScopedPlayerState.

To allow clients to change the player state, this CL adds
MediaExperimentManager::ScopedPlayerState.  This class provides
access to MediaExperimentManager::PlayerState via the -> operator,
and notifies the manager when it goes out of scope.

This is mostly a convenience to remove the need for accessor
methods on the manager.  However, it also lets the client modify
multiple properties of a player atomically, before the manager
tries to check for changes to trigger experiment notifications.

Change-Id: Icc80f17598aeccfd3d660df869281fabfc7545cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1595115Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657137}

content/browser/media/media_experiment_manager.cc

@@ -4,8 +4,28 @@
 
 #include "content/browser/media/media_experiment_manager.h"
 
+#include <utility>
+
+#include "base/bind_helpers.h"
+
 namespace content {
 
+MediaExperimentManager::ScopedPlayerState::ScopedPlayerState(
+    base::OnceClosure destruction_cb,
+    PlayerState* state)
+    : state_(state), destruction_cb_(std::move(destruction_cb)) {}
+
+MediaExperimentManager::ScopedPlayerState::ScopedPlayerState(
+    ScopedPlayerState&& rhs) {
+  destruction_cb_ = std::move(rhs.destruction_cb_);
+  state_ = rhs.state_;
+}
+
+MediaExperimentManager::ScopedPlayerState::~ScopedPlayerState() {
+  if (destruction_cb_)
+    std::move(destruction_cb_).Run();
+}
+
 MediaExperimentManager::MediaExperimentManager() = default;
 
 MediaExperimentManager::~MediaExperimentManager() = default;
@@ -23,6 +43,15 @@ void MediaExperimentManager::PlayerDestroyed(const MediaPlayerId& player_id) {
   ErasePlayersInternal({player_id});
 }
 
+MediaExperimentManager::ScopedPlayerState
+MediaExperimentManager::GetPlayerState(const MediaPlayerId& player_id) {
+  auto iter = players_.find(player_id);
+  DCHECK(iter != players_.end());
+  // TODO(liberato): Replace this callback with something that checks the
+  // experiment state, to see if an experiment has started / stopped.
+  return ScopedPlayerState(base::DoNothing(), &iter->second);
+}
+
 void MediaExperimentManager::ClientDestroyed(Client* client) {
   auto by_client = player_ids_by_client_.find(client);
   // Make a copy, since ErasePlayers will modify the original.

content/browser/media/media_experiment_manager.h

@@ -9,6 +9,7 @@
 #include <set>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "content/common/content_export.h"
@@ -46,10 +47,24 @@ class CONTENT_EXPORT MediaExperimentManager {
   struct PlayerState {
     Client* client = nullptr;
     bool is_playing = false;
-    bool is_full_screen = false;
+    bool is_fullscreen = false;
     bool is_pip = false;
   };
 
+  class CONTENT_EXPORT ScopedPlayerState {
+   public:
+    ScopedPlayerState(base::OnceClosure destruction_cb, PlayerState* state);
+    ScopedPlayerState(ScopedPlayerState&&);
+    ~ScopedPlayerState();
+
+    PlayerState* operator->() { return state_; }
+
+   private:
+    PlayerState* state_;
+    base::OnceClosure destruction_cb_;
+    DISALLOW_COPY_AND_ASSIGN(ScopedPlayerState);
+  };
+
   MediaExperimentManager();
   virtual ~MediaExperimentManager();
 
@@ -70,7 +85,10 @@ class CONTENT_EXPORT MediaExperimentManager {
   // error if |client| has no active players.
   virtual void ClientDestroyed(Client* client);
 
-  // TODO(liberato): Allow clients to update the player's state.
+  // Update the player state.  When the returned ScopedMediaPlayerState is
+  // destroyed, we will process the changes.  One may not create or destroy
+  // players while the ScopedMediaPlayerState exists.
+  virtual ScopedPlayerState GetPlayerState(const MediaPlayerId& player);
 
   // Return the number of players total.
   size_t GetPlayerCountForTesting() const;

content/browser/media/media_experiment_manager_unittest.cc

@@ -15,6 +15,8 @@
 
 namespace content {
 
+using ScopedPlayerState = MediaExperimentManager::ScopedPlayerState;
+
 class MockExperimentClient : public MediaExperimentManager::Client {
  public:
   MOCK_METHOD1(OnExperimentStarted, void(const MediaPlayerId& player));
@@ -81,4 +83,70 @@ TEST_F(MediaExperimentManagerTest, CreateTwoClientsAndDestroyOneClient) {
   EXPECT_EQ(manager_->GetPlayerCountForTesting(), 1u);
 }
 
+TEST_F(MediaExperimentManagerTest, ScopedPlayerStateModifiesState) {
+  MockExperimentClient client_1;
+  MockExperimentClient client_2;
+
+  manager_->PlayerCreated(player_id_1_, &client_1);
+  manager_->PlayerCreated(player_id_2_, &client_2);
+  // Set the player state differently for each player.  We set two things so
+  // that each player has a non-default value.
+  {
+    ScopedPlayerState state = manager_->GetPlayerState(player_id_1_);
+    state->is_fullscreen = true;
+    state->is_pip = false;
+  }
+
+  {
+    ScopedPlayerState state = manager_->GetPlayerState(player_id_2_);
+    state->is_fullscreen = false;
+    state->is_pip = true;
+  }
+
+  // Make sure that the player state matches what we set for it.
+  {
+    ScopedPlayerState state = manager_->GetPlayerState(player_id_1_);
+    EXPECT_TRUE(state->is_fullscreen);
+    EXPECT_FALSE(state->is_pip);
+  }
+
+  {
+    ScopedPlayerState state = manager_->GetPlayerState(player_id_2_);
+    EXPECT_FALSE(state->is_fullscreen);
+    EXPECT_TRUE(state->is_pip);
+  }
+}
+
+TEST_F(MediaExperimentManagerTest, ScopedPlayerStateCallsCallback) {
+  bool cb_called = false;
+  base::OnceClosure cb = base::BindOnce([](bool* flag) { *flag = true; },
+                                        base::Unretained(&cb_called));
+  MediaExperimentManager::PlayerState state;
+  state.is_fullscreen = false;
+
+  // Normally, these would not by dynamically allocated.  However, this makes
+  // it much easier to control when they're destroyed.  This is also why we
+  // reference the underying state as (*scoped_1)-> ; we want tp use the
+  // overloaded -> operator on ScopedPlayerState, not unique_ptr.
+  std::unique_ptr<ScopedPlayerState> scoped_1 =
+      std::make_unique<ScopedPlayerState>(std::move(cb), &state);
+  (*scoped_1)->is_fullscreen = true;
+  EXPECT_TRUE(state.is_fullscreen);
+  EXPECT_FALSE(cb_called);
+
+  // Moving |scoped_1| and deleting it should not call the callback.
+  std::unique_ptr<ScopedPlayerState> scoped_2 =
+      std::make_unique<ScopedPlayerState>(std::move(*scoped_1));
+  scoped_1.reset();
+  EXPECT_FALSE(cb_called);
+
+  // |scoped_2| should now modify |state|.
+  (*scoped_2)->is_fullscreen = false;
+  EXPECT_FALSE(state.is_fullscreen);
+
+  // Deleting |scoped_2| should call the callback.
+  scoped_2.reset();
+  EXPECT_TRUE(cb_called);
+}
+
 }  // namespace content