[PM] Add a mechanism to change existing FramePriority votes.

VoteConsumers can implement this efficiently, or choose to invalidate
a vote and create new one.

BUG=971272

Change-Id: I47bf89b6abe223807a2a09b9ba47a2054d241e85
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1724701
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683183}

chrome/browser/performance_manager/frame_priority/frame_priority.cc

@@ -59,10 +59,25 @@ VoteConsumer* VoteReceipt::GetConsumer() const {
   return vote_->consumer();
 }
 
+VoterId VoteReceipt::GetVoterId() const {
+  return vote_->voter_id();
+}
+
 const Vote& VoteReceipt::GetVote() const {
   return vote_->vote();
 }
 
+void VoteReceipt::ChangeVote(base::TaskPriority priority, const char* reason) {
+  DCHECK(vote_);
+
+  // Do nothing if the vote hasn't actually changed.
+  const auto& vote = vote_->vote();
+  if (vote.priority() == priority && vote.reason() == reason)
+    return;
+
+  *this = vote_->ChangeVote(std::move(*this), priority, reason);
+}
+
 void VoteReceipt::Reset() {
   if (vote_) {
     vote_->InvalidateVote(this);
@@ -150,6 +165,13 @@ VoteReceipt AcceptedVote::IssueReceipt() {
   return VoteReceipt(this);
 }
 
+void AcceptedVote::UpdateVote(const Vote& vote) {
+  DCHECK_EQ(vote_.frame_node(), vote.frame_node());
+  DCHECK(vote_.priority() != vote.priority() ||
+         vote_.reason() != vote.reason());
+  vote_ = vote;
+}
+
 void AcceptedVote::SetReceipt(VoteReceipt* receipt) {
   // A receipt can only be set on a vote once in its lifetime.
   DCHECK(!receipt_);
@@ -173,6 +195,31 @@ void AcceptedVote::MoveReceipt(VoteReceipt* old_receipt,
   DCHECK(receipt_->HasVote(this));
 }
 
+VoteReceipt AcceptedVote::ChangeVote(VoteReceipt receipt,
+                                     base::TaskPriority priority,
+                                     const char* reason) {
+  DCHECK_EQ(receipt_, &receipt);
+  DCHECK(!invalidated_);
+  DCHECK(vote_.priority() != priority || vote_.reason() != reason);
+
+  // Explicitly save a copy of |vote_| as the consumer might overwrite it
+  // directly.
+  Vote old_vote = vote_;
+
+  // Notify the consumer of the new vote.
+  Vote new_vote = Vote(old_vote.frame_node(), priority, reason);
+  receipt = consumer_->ChangeVote(std::move(receipt), this, new_vote);
+
+  // Ensure that the returned receipt refers to a vote with the expected
+  // properties.
+  const Vote& returned_vote = receipt.GetVote();
+  DCHECK_EQ(new_vote.frame_node(), returned_vote.frame_node());
+  DCHECK_EQ(new_vote.priority(), returned_vote.priority());
+  DCHECK_EQ(new_vote.reason(), returned_vote.reason());
+
+  return receipt;
+}
+
 void AcceptedVote::InvalidateVote(VoteReceipt* receipt) {
   DCHECK(receipt);
   DCHECK_EQ(receipt_, receipt);
@@ -277,5 +324,25 @@ void VotingChannelFactory::OnVotingChannelDestroyed() {
 VoteConsumer::VoteConsumer() = default;
 VoteConsumer::~VoteConsumer() = default;
 
+/////////////////////////////////////////////////////////////////////
+// VoteConsumerDefaultImpl
+
+VoteConsumerDefaultImpl::VoteConsumerDefaultImpl() = default;
+VoteConsumerDefaultImpl::~VoteConsumerDefaultImpl() = default;
+
+VoteReceipt VoteConsumerDefaultImpl::ChangeVote(VoteReceipt receipt,
+                                                AcceptedVote* old_vote,
+                                                const Vote& new_vote) {
+  // The receipt and vote should be entangled, and the vote should be valid.
+  DCHECK(receipt.HasVote(old_vote));
+  DCHECK(old_vote->IsValid());
+
+  // Tear down the old vote before submitting a new one in order to prevent
+  // the voter from having 2 simultaneous votes for the same frame.
+  auto voter_id = receipt.GetVoterId();
+  receipt.Reset();
+  return SubmitVote(voter_id, new_vote);
+}
+
 }  // namespace frame_priority
 }  // namespace performance_manager

chrome/browser/performance_manager/frame_priority/frame_priority_unittest.cc

@@ -84,6 +84,26 @@ TEST(FramePriorityTest, VoteReceiptsWork) {
   ExpectEntangled(voter.receipts_[0], consumer.votes_[0]);
   ExpectEntangled(voter.receipts_[1], consumer.votes_[1]);
 
+  // Change a vote, but making no change.
+  ExpectEntangled(voter.receipts_[0], consumer.votes_[0]);
+  EXPECT_EQ(kDummyFrame1, consumer.votes_[0].vote().frame_node());
+  EXPECT_EQ(base::TaskPriority::LOWEST, consumer.votes_[0].vote().priority());
+  EXPECT_EQ(test::DummyVoter::kReason, consumer.votes_[0].vote().reason());
+  voter.receipts_[0].ChangeVote(base::TaskPriority::LOWEST,
+                                test::DummyVoter::kReason);
+  ExpectEntangled(voter.receipts_[0], consumer.votes_[0]);
+  EXPECT_EQ(kDummyFrame1, consumer.votes_[0].vote().frame_node());
+  EXPECT_EQ(base::TaskPriority::LOWEST, consumer.votes_[0].vote().priority());
+  EXPECT_EQ(test::DummyVoter::kReason, consumer.votes_[0].vote().reason());
+
+  // Change the vote and expect the change to propagate.
+  static const char kReason[] = "another reason";
+  voter.receipts_[0].ChangeVote(base::TaskPriority::HIGHEST, kReason);
+  ExpectEntangled(voter.receipts_[0], consumer.votes_[0]);
+  EXPECT_EQ(kDummyFrame1, consumer.votes_[0].vote().frame_node());
+  EXPECT_EQ(base::TaskPriority::HIGHEST, consumer.votes_[0].vote().priority());
+  EXPECT_EQ(kReason, consumer.votes_[0].vote().reason());
+
   // Cancel a vote.
   voter.receipts_[0].Reset();
   EXPECT_EQ(2u, voter.receipts_.size());

chrome/browser/performance_manager/frame_priority/unittest_util.cc

@@ -29,8 +29,23 @@ VoteReceipt DummyVoteConsumer::SubmitVote(VoterId voter_id, const Vote& vote) {
   return receipt;
 }
 
-void DummyVoteConsumer::VoteInvalidated(AcceptedVote* vote) {
+VoteReceipt DummyVoteConsumer::ChangeVote(VoteReceipt receipt,
+                                          AcceptedVote* old_vote,
+                                          const Vote& new_vote) {
   // We should own this vote and it should be valid.
+  EXPECT_TRUE(receipt.HasVote(old_vote));
+  EXPECT_LE(votes_.data(), old_vote);
+  EXPECT_LT(old_vote, votes_.data() + votes_.size());
+  EXPECT_TRUE(old_vote->IsValid());
+  EXPECT_LT(0u, valid_vote_count_);
+
+  // Just update the vote in-place, and return the existing receipt for it.
+  old_vote->UpdateVote(new_vote);
+  return receipt;
+}
+
+void DummyVoteConsumer::VoteInvalidated(AcceptedVote* vote) {
+  // We should own this vote.
   EXPECT_LE(votes_.data(), vote);
   EXPECT_LT(vote, votes_.data() + votes_.size());
   EXPECT_FALSE(vote->IsValid());
@@ -53,6 +68,9 @@ void DummyVoteConsumer::ExpectValidVote(size_t index,
   EXPECT_TRUE(vote.reason());
 }
 
+// static
+const char DummyVoter::kReason[] = "dummmy reason";
+
 DummyVoter::DummyVoter() = default;
 DummyVoter::~DummyVoter() = default;
 
@@ -61,11 +79,11 @@ void DummyVoter::SetVotingChannel(VotingChannel&& voting_channel) {
 }
 
 void DummyVoter::EmitVote(const FrameNode* frame_node,
-                          base::TaskPriority priority) {
-  static const char kReason[] = "dummmy reason";
+                          base::TaskPriority priority,
+                          const char* reason) {
   EXPECT_TRUE(voting_channel_.IsValid());
   receipts_.emplace_back(
-      voting_channel_.SubmitVote(Vote(frame_node, priority, kReason)));
+      voting_channel_.SubmitVote(Vote(frame_node, priority, reason)));
 }
 
 }  // namespace test

chrome/browser/performance_manager/frame_priority/unittest_util.h

@@ -23,6 +23,9 @@ class DummyVoteConsumer : public VoteConsumer {
 
   // VoteConsumer implementation:
   VoteReceipt SubmitVote(VoterId voter_id, const Vote& vote) override;
+  VoteReceipt ChangeVote(VoteReceipt receipt,
+                         AcceptedVote* old_vote,
+                         const Vote& new_vote) override;
   void VoteInvalidated(AcceptedVote* vote) override;
 
   // Checks that the vote at position |index| is valid, and has the
@@ -36,12 +39,15 @@ class DummyVoteConsumer : public VoteConsumer {
   std::vector<AcceptedVote> votes_;
   size_t valid_vote_count_ = 0;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(DummyVoteConsumer);
 };
 
 // A dummy voter that allows emitting votes and tracking their receipts.
 class DummyVoter {
  public:
+  static const char kReason[];
+
   DummyVoter();
   ~DummyVoter();
 
@@ -50,7 +56,8 @@ class DummyVoter {
   // Causes the voter to emit a vote for the given |frame_node| and with the
   // given |priority|. The receipt is pushed back onto |receipts_|.
   void EmitVote(const FrameNode* frame_node,
-                base::TaskPriority priority = base::TaskPriority::LOWEST);
+                base::TaskPriority priority = base::TaskPriority::LOWEST,
+                const char* reason = kReason);
 
   VotingChannel voting_channel_;
   std::vector<VoteReceipt> receipts_;

chrome/browser/performance_manager/public/frame_priority/frame_priority.h

@@ -16,8 +16,8 @@
 //     (by a VoteConsumer) and tracking (via VoteReceipt). This is a final
 //     concrete class.
 // (3) VoteReceipt - Counterpart to an AcceptedVote. Issued to a voter when a
-//     vote is accepted, and used to allow that voter to withdraw / cancel their
-//     vote. This is a final concrete class.
+//     vote is accepted, and used to allow that voter to change / withdraw /
+//     cancel their vote. This is a final concrete class.
 // (4) VoteConsumer - Destination for Votes (making them AcceptedVotes) and
 //     issuer of VoteReceipts. This is an interface.
 // (5) VotingChannel - A mechanism by which a voter can submit votes to
@@ -128,10 +128,19 @@ class VoteReceipt final {
   // HasVote returns true.
   VoteConsumer* GetConsumer() const;
 
+  // Returns the voter ID associated with this receipt. Can only be called if
+  // HasVote returns true.
+  VoterId GetVoterId() const;
+
   // Returns the vote corresponding to this receipt. Can only be called if
   // HasVote returns true.
   const Vote& GetVote() const;
 
+  // Changes the upstream vote associated with this vote receipt. Can only be
+  // called if HasVote returns true.
+  void ChangeVote(base::TaskPriority priority, const char* reason);
+
+  // Rests the vote receipt, canceling the upstream vote.
   void Reset();
 
  protected:
@@ -192,6 +201,9 @@ class AcceptedVote final {
   VoterId voter_id() const { return voter_id_; }
   const Vote& vote() const { return vote_; }
 
+  // Allows an accepted vote to be updated in place.
+  void UpdateVote(const Vote& vote);
+
  protected:
   // VoteReceipt and AcceptedVote are tightly intertwined, and maintain
   // back-pointers to each other as one or the other is moved. The friendship
@@ -205,6 +217,12 @@ class AcceptedVote final {
   // Allows a VoteReceipt to update its backpointer.
   void MoveReceipt(VoteReceipt* old_receipt, VoteReceipt* new_receipt);
 
+  // Allows a VoteReceipt to change this vote. The vote receipt gives up its
+  // receipt only to be returned a new one.
+  VoteReceipt ChangeVote(VoteReceipt receipt,
+                         base::TaskPriority priority,
+                         const char* reason);
+
   // Allows a VoteReceipt to invalidate this vote.
   void InvalidateVote(VoteReceipt* receipt);
 
@@ -325,6 +343,24 @@ class VoteConsumer {
   // Used by a VotingChannel to submit votes to this consumer.
   virtual VoteReceipt SubmitVote(VoterId voter_id, const Vote& vote) = 0;
 
+  // Used by an AcceptedVote to notify a consumer that a previously issued vote
+  // has been changed. Both the |new_vote| and the |receipt| are provided to the
+  // consumer, as it may be necessary for the consumer to create an entirely
+  // new vote and issue a new receipt for it (although ideally it does not do so
+  // for the sake of efficiency). Alternatively, the consumer can choose to
+  // update the |old_vote| in-place, using the data from |new_vote|. This is
+  // kept protected as it is part of a private contract between an AcceptedVote
+  // and a VoteConsumer. A naive implementation of this would be the following:
+  //
+  //   // Tear down the old vote before submitting a new one in order to prevent
+  //   // the voter from having 2 simultaneous votes for the same frame.
+  //   auto voter_id = receipt.GetVoterId();
+  //   receipt.Reset();
+  //   return SubmitVote(voter_id, new_vote);
+  virtual VoteReceipt ChangeVote(VoteReceipt receipt,
+                                 AcceptedVote* old_vote,
+                                 const Vote& new_vote) = 0;
+
   // Used by a AcceptedVote to notify a consumer that a previously issued
   // receipt has been destroyed, and the vote is now invalidated. This is kept
   // protected as it is part of a private contract between an AcceptedVote and a
@@ -335,6 +371,26 @@ class VoteConsumer {
   DISALLOW_COPY_AND_ASSIGN(VoteConsumer);
 };
 
+// Provides a default implementation of VoteConsumer that implements a naive
+// (less efficient) version of "ChangeVote".
+class VoteConsumerDefaultImpl : public VoteConsumer {
+ public:
+  VoteConsumerDefaultImpl();
+  ~VoteConsumerDefaultImpl() override;
+
+  // VoteConsumer implementation left to the derived class:
+  VoteReceipt SubmitVote(VoterId voter_id, const Vote& vote) override = 0;
+  void VoteInvalidated(AcceptedVote* vote) override = 0;
+
+  // VoteConsumer implementation provided by this class:
+  VoteReceipt ChangeVote(VoteReceipt receipt,
+                         AcceptedVote* existing_vote,
+                         const Vote& new_vote) override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(VoteConsumerDefaultImpl);
+};
+
 }  // namespace frame_priority
 }  // namespace performance_manager